dequelabs / axe-core

Accessibility engine for automated Web UI testing
https://www.deque.com/axe/
Mozilla Public License 2.0
5.94k stars 773 forks source link

aria-hidden-focus trigger false positive #1482

Closed luarmr closed 4 years ago

luarmr commented 5 years ago

Expectation: I expect aria-hidden-focus only show as an error when the content is in the viewport.

Actual: aria-hidden-focus is showing as an error in elements that are outside the viewport (intentionally not showing in the screen) or behind another layer.

Motivation: I am getting this error in carousels or modal window for example: https://react-bootstrap.github.io/components/modal/#modals-live or code like (nonsense code but I think made a point): https://codepen.io/luarmr/pen/ROoMWQ


axe-core version: 3.2.2
axe-puppeteer version:1.0.0 (but the relevant is axe-core)
WilcoFiers commented 5 years ago

@luarmr I am not sure why you would want axe-core to exclude visually hidden content. If you have a link or button or something else focusable inside an aria-hidden=true region, that element can still be focused, and it can be pulled up in things like link lists. When you put an element in an aria-hidden=true region, its accessibility name is removed. This results in something that's effectively a link/button/whatever without a name.

ktsangop commented 5 years ago

I am facing a similar problem, working with ng-bootstrap library https://ng-bootstrap.github.io/#/components/modal/examples

I believe that when a modal has the role="dialog" and aria-modal="true" then AXE could omit showing this error because those attributes explicitly set the modal as the only area of focus.

https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html https://www.w3.org/TR/wai-aria-practices/#dialog_roles_states_props

The fact that in ng-bootstrap modals, removing the aria-hidden="true" from the body/root app element, is fixing the AXE error, without breaking screen reader support, shows that this could be an issue of ng-bootstrap. But, since not all browsers/screen readers currently support the above attributes, i believe that aria-hidden="true" is intentionally left, to prevent screen readers from focusing out of the dialog.

In fact another related example here with the same error, which is i believe from AXE's own documentation website :

https://dequeuniversity.com/library/aria/popups-dialogs/sf-simple-dialog

FYI: The react-bootstrap library indeed seems to have another bug. Except for adding aria-hidden="true" to the body, it also adds it to the modal container, along with role="dialog" and aria-modal="true". That's why AXE shows 2 errors. On the second case i think that AXE is correct.

In my opinion, until role="dialog" and aria-modal="true" are widely supported by browsers/screen readers, this AXE rule should be changed.

luarmr commented 5 years ago

@WilcoFiers Thanks for the answer. For example in a carousel when you are preloading the content, and you don't want for any user to interact with the content. For web browser, you hide the content from the viewport and form screen readers you use aria-hidden. And this content is not focusable even with tab because you add the aria-hidden. I understand that this is a problem if a visible button is visible in the screen and a user with his keyboard is pretending to access it but if this element is not visible in the screen.

As well the example with the modals propose for @ktsangop is the second scenario.

gubagu commented 5 years ago

Similar problem here with angular material mat dialog. It does have aria-modal="true" but elements behind the modal are still getting evaluated while the focus is bound to the modal dialog only. The result is a lot of false positives.

stevenyxu commented 5 years ago

This continues to be a noisy false positive throughout our code base and in Angular Material: https://github.com/angular/components/pull/14644.

Having tabindex traps is a widely accepted way to implementing modal dialogs on web (with the addition of aria-hidden on all content outside of the dialog to also trap the virtual cursor) because there is no other way to trap tab focus from hitting the content behind the backdrop. The ARIA best practices page uses this https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html. aria-modal is not sufficient in practice to hide content behind the modal and definitely not sufficient to prevent tabbing from going to it.

These must be aria-hidden (the ARIA best practices page is incorrect in this regard) because many screen readers (iOS VO, Android TalkBack, NVDA/JAWS under some configurations) can navigate to the tabstop without focusing on it, causing a nuisance unlabelled element.

The approach of using aria-hidden tabindex=0 anchors at the bookends of a modal dialog is the best approach we have today (though we're open to new ones recommended by Deque if any exist with broad support), and axe falsely trips on it. We encourage feature teams implementing pages on top of our modal frameworks to use axe, but this nuisance is very annoying and reduces confidence in axe tooling. We are appreciative of the axe tooling overall and are big boosters, and resolving this to accommodate the a11y best practice would be a useful improvement.

dylanb commented 5 years ago

Despite everything in those other tickets, it is still possible to get keyboard focus into the content behind the modal dialog using the rotor in Voiceover and similar functionality in other SRs.

image

In other words @stevenyxu @gubagu and @ktsangop - this is not a false positive

dylanb commented 5 years ago

@stevenyxu the technique to use is to set tabindex=-1 on all naturally focusable content inside the aria-hidden="true" region when the modal displays and then remove that attribute when it is closed

Alternatively (for elements that support it), you can set disabled on the element to remove focusability while it is in the background

stevenyxu commented 5 years ago

@dylanb are you testing in Chrome? Production Chrome is currently affected by https://bugs.chromium.org/p/chromium/issues/detail?id=971880. Does it reproduce in Safari?

@dylanb thanks for the suggestions. I worry that the changes you propose are onerous and don't play well with many data binding frameworks without additional abstraction to allow a global override in this way. Do we really want to encourage people to have to make potentially hundreds of DOM changes all across the page just to show a single modal popup?

dylanb commented 5 years ago

@stevenyxu I was testing in Safari

As far as the onerousness is concerned, you could try to use the inert polyfill https://github.com/GoogleChrome/inert-polyfill

ktsangop commented 5 years ago

Despite everything in those other tickets, it is still possible to get keyboard focus into the content behind the modal dialog using the rotor in Voiceover and similar functionality in other SRs.

image

In other words @stevenyxu @gubagu and @ktsangop - this is not a false positive

This is interesting. So what is the best solution? Add aria-hidden="true" for all dom elements outside the modal?

Also could anyone explain how this example which seems to use the same techniques does not trigger a similar error?

https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html

mfairchild365 commented 5 years ago

Consider the following test and results.

Test:

  1. Navigate to https://material.angular.io/components/dialog/examples
  2. Click "pick one" and the modal will open
  3. Run axe and observe this violation being thrown.
  4. Try to access content outside of the open model with the following methods
    • Using the elements list
    • Using other AT short cuts (jump to the next form control, speak "click your name", etc).
    • Using the tab key
    • Using arrow keys or gestures to navigate to the next line/item

My testing results indicate that all supported combinations by axe pass (excluding the one I couldn't test). I don't have access to an android device to test. I'm inclined to say that this is a false positive.

related issue in ACT-Rules.

AT Browser Able to focus content outside of the modal using elements list Able to focus content outside of the modal using other shortcuts (next form control, etc) Able to focus content outside of the modal using tab key Able to focus content outside of the modal using arrow keys or gestures All tests pass
VO/MacOS (10.14.5) Safari (12.1.1) No No No No Yes
VO/iOS (12.3.1) Safari (12.3.1) No No NA No Yes
NVDA (2019.1.1) Firefox (67) No No No No Yes
JAWS (2019.1906.10) IE (11) No No No No Yes
Talkback Chrome unknown unknown unknown unknown
Dragon (15.30) Firefox (67) NA No No NA Yes

@dylanb It looks like we got different results for VO on MacOS. What version of MacOS and Safari are you testing with? Could also be a difference in implementation.

straker commented 5 years ago

Here's a CodePen of a simple test case verifying @mfairchild365's findings https://codepen.io/straker/pen/MNojdp. I even testing JAWS+Chrome and the results are the same.

However, this only works if javascript is trapping the tab focus while inside the modal. If not, then the elements outside of the modal are still focusable via the tab key. However, there's no easy way to test if the user has implemented tab trapping on the modal and thus cannot tab to the elements.

Perhaps instead of marking these as a violation, we mark them as Needs Review and let the user know of a potential problem but let them manual test it for errors.

timbomckay commented 5 years ago

We're discussing this rule at our organization as well. We have dialog & popover components that apply aria-hidden="true" on the root elements that are siblings to the displayed modal. A focus trap prevents anyone from navigating out of the modal and an overlay prevents clicking an element outside of the modal. This rule would require us to query the aria-hidden="true" elements to apply tabindex="-1" to any focusable elements, which just seems like overkill if we're already providing a solution.

Perhaps instead of marking these as a violation, we mark them as Needs Review and let the user know of a potential problem but let them manual test it for errors.

We provide a report to our higher ups and marking aria-hidden-focus as a "Serious" issue is misleading to them. "Needs Review" seems much more applicable as it's an issue that should be considered but something axe-core may not be able to check.

Found this issue after reading a related issue in ACT-Rules. Responded in here as it seems to be the original with more activity.

padmavemulapati commented 4 years ago

for Link/Button elements, aria-hidden-focus is not on viewport but still giving incomplete issue details, verified for the given test file - `

` for the test files like having model/popups we can see the incomplete details after clicking on model (once appearing on viewport). Which is expected