Skip to content

Remove aria-hidden from FocusTrapZone's bumpers#9019

Merged
Jahnp merged 3 commits into
microsoft:masterfrom
atneik:panel-focus
May 9, 2019
Merged

Remove aria-hidden from FocusTrapZone's bumpers#9019
Jahnp merged 3 commits into
microsoft:masterfrom
atneik:panel-focus

Conversation

@atneik

@atneik atneik commented May 9, 2019

Copy link
Copy Markdown
Contributor

Pull request checklist

Description of changes

As bumpers are focus-able, having aria-hidden: true is violating a rule. Having no aria-hidden is better in this case.

I noticed the implementation of trapping focus in a W3 aria examples, they didn't require any aria-hidden for bumpers. Windows Narrator doesn't inform their presence either.

Focus areas to test

Any accessibility regressions

Microsoft Reviewers: Open in CodeFlow

@atneik
atneik requested a review from JasonGore May 9, 2019 00:56
@atneik
atneik requested review from Jahnp, Vitalius1 and kkjeer as code owners May 9, 2019 00:56
@atneik atneik changed the title Remove aria-hidden from FocusZoneTrap's bumpers Remove aria-hidden from FocusTrapZone's bumpers May 9, 2019
@msft-github-bot

Copy link
Copy Markdown
Contributor

Component perf results:

Scenario Target branch avg total (ms) PR avg total (ms) Target branch avg per item (ms) PR avg per item (ms) Is significant change Is regression
PrimaryButton 79.475 78.454 0.795 0.785 false false
BaseButton 30.503 30.045 0.305 0.301 false false
NewButton 110.611 113.117 1.106 1.131 false false
button 5.202 5.099 0.052 0.051 false false
DetailsRows without styles 172.372 169.173 1.724 1.692 false false
DetailsRows 196.510 197.760 1.965 1.978 false false
Toggles 48.633 48.313 0.486 0.483 false false
NewToggle 64.636 66.090 0.646 0.661 false false
DocumentCardTitle with truncation 25.328 23.969 0.253 0.240 false false

@size-auditor

size-auditor Bot commented May 9, 2019

Copy link
Copy Markdown
Bundle test Size (minified) Diff from master
HoverCard 99.124 kB BelowBaseline     -17 bytes
Dropdown 215.081 kB BelowBaseline     -17 bytes
DatePicker 203.737 kB BelowBaseline     -17 bytes
Callout 83.468 kB BelowBaseline     -17 bytes
FocusTrapZone 17.709 kB BelowBaseline     -17 bytes
Panel 184.159 kB BelowBaseline     -17 bytes
Dialog 194.095 kB BelowBaseline     -17 bytes
Modal 93.874 kB BelowBaseline     -17 bytes
Coachmark 97.624 kB BelowBaseline     -17 bytes

ExceedsTolerance  Exceeds Tolerance     ExceedsBaseline  Exceeds Baseline     BelowBaseline  Below Baseline     1 kB = 1000 bytes

@cliffkoh cliffkoh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be worth checking who added aria-hidden in the first place and get sign off from the original author.

@JasonGore JasonGore left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not see bumpers anywhere in the ARIA examples. Can you please show in detail where you see bumpers in the ARIA example?

There should be no need for bumpers when using aria-modal because aria-modal implicitly handles this behavior.

We want behavior similar aria-modal in FocusTrapZone but there is no browser analog. As a result, we have to use "virtual" bumpers. They are technically focusable in that they receive focus, but they immediately pass it on. They are not and should never be focusable from the users' perspective, and they should be completely hidden from an accessibility perspective.

@atneik

atneik commented May 9, 2019

Copy link
Copy Markdown
Contributor Author

@JasonGore They get added in JS here:
image

Resulting in:

image

Though not necessarily used for the same functionality, they do highlight how some element that needs to be "tabbable but not visible" could be implemented. I don't see the side effects of removing aria-hidden, as won't screen readers ignore them due to lack of any label or description and focus forwarding? What would be your recommended solution?

Also as per my understanding aria-modal would just hide other layers for readers without having any effecting on keyboarding.

@JasonGore JasonGore left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There it is! Sorry I missed that.

The concern I have is that the bumpers get focused in scan mode, but that seems to be the case even for the ARIA sample (at least the top bumper does.) Seeing as their implementation is so close in concept to FocusTrapZone, I guess it's ok to remove aria-hidden.

@Jahnp
Jahnp merged commit 85b43c2 into microsoft:master May 9, 2019
@msft-github-bot

Copy link
Copy Markdown
Contributor

🎉office-ui-fabric-react@v6.180.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

panels violates rule: aria-hidden-focus

5 participants