Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLAT-115535: Wrap the created portal with div having aria-owns #2840

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ybsung
Copy link
Contributor

@ybsung ybsung commented Aug 4, 2020

Checklist

  • I have read and understand the contribution guide
  • A CHANGELOG entry is included
  • At least one test case is included for this feature or bug fix
  • Documentation was added or is not needed
  • This is an API breaking change

Issue Resolved / Feature Added

When moving focus from the component in the panel to the component in the floatLayer and moving back,
the screen reader doesn't read out the panel header because the panel has aria-owns="floatLayer" prop.

But when the panel is in the floatLayer like PopupTabLayout, the aria-owns won't work. Then the screen reader reads the panel title first, then a focused component when moving focus from out of the panel to in the panel.

Resolution

When creating a component in the floatLayer, the component has the id and is wrapped with div including aria-owns prop of the id.

Additional Considerations

Links

PLAT-115535

Comments

Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])

Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected]
@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@a95419f). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #2840   +/-   ##
==========================================
  Coverage           ?   44.72%           
==========================================
  Files              ?      163           
  Lines              ?     8244           
  Branches           ?     2009           
==========================================
  Hits               ?     3687           
  Misses             ?     3422           
  Partials           ?     1135           
Impacted Files Coverage Δ
packages/ui/FloatingLayer/FloatingLayer.js 72.36% <100.00%> (ø)
packages/i18n/$L/$L.js 0.00% <0.00%> (ø)
packages/ui/BodyText/BodyText.js 100.00% <0.00%> (ø)
...ges/ui/internal/localized-fonts/localized-fonts.js 0.00% <0.00%> (ø)
packages/ui/Touchable/Hold.js 21.34% <0.00%> (ø)
packages/ui/Scrollable/Scrollable.js 0.00% <0.00%> (ø)
packages/core/util/util.js 45.09% <0.00%> (ø)
packages/ui/Toggleable/Toggle.js 100.00% <0.00%> (ø)
packages/i18n/src/zoneinfo.js 7.60% <0.00%> (ø)
... and 154 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a95419f...5792a47. Read the comment docs.

Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])
Copy link
Contributor

@MikyungKim MikyungKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

{scrimType !== 'none' ? <Scrim type={scrimType} onClick={this.handleClick} /> : null}
{React.cloneElement(children, {onClick: this.stopPropagation})}
</div>,
this.node
);

return <div aria-owns={id}>{portal}</div>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding DOM here could be problematic. Since the portal is rendering in another subtree, this <div /> ends up being dropped wherever FloatingLayer was used and those component might not be expecting it.

I like the instinct to build this into ui but I'm not sure this is the right way. It might not be feasible to do since some outer component needs the id in order to set aria-owns correctly.

An alternative might be to add some Context to this to help auto-wire IDs up since they are required (instead of something generic like CSS classes) for ARIA to behave correctly. I'm not sure what that would be yet but something to consider.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants