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

Add custom message for negative tabindex in nested-interactive #3163

Closed
WilcoFiers opened this issue Sep 23, 2021 · 10 comments
Closed

Add custom message for negative tabindex in nested-interactive #3163

WilcoFiers opened this issue Sep 23, 2021 · 10 comments
Labels
good first issue For first-time contributors rule metadata Issues in the rule metadata code (lib/rules)

Comments

@WilcoFiers
Copy link
Contributor

WilcoFiers commented Sep 23, 2021

See #3159

Even though they seem like they should, things like this just don't work well:

  <div role="button" id="pass7" tabindex="0">
    <button aria-hidden="true" tabindex="-1">hidden</button> text
  </div>
  <div role=checkbox aria-checked=true tabindex=0 aria-label=foo>
    <input type=checkbox aria-hidden=true tabindex=-1> <!-- visually hidden -->
  </div>

The button / input inside will still be packed up by various AT, that are trying very hard to ensure they don't miss things. This is by no means obvious, and so it would be good to add a specific message key to the no-focusable-content check that reports that using aria-hidden and negative tabindex is not a reliable way of hiding interactive elements. We should have a second message for role=none|presentation and negative tabindex too, just so we don't try to combine too many things.

See ariaErrormessageEvaluate for an example of how to use messageKey.

@WilcoFiers WilcoFiers added rule metadata Issues in the rule metadata code (lib/rules) good first issue For first-time contributors labels Sep 23, 2021
@dan-tripp
Copy link
Contributor

I will try to implement this. Wish me luck.

@straker
Copy link
Contributor

straker commented Oct 1, 2021

Awesome. Let me know if you have any questions.

@dan-tripp
Copy link
Contributor

I'm trying to reconcile these two statements:
Wilco's statement "We should have a second message for role=none|presentation and negative tabindex too, just so we don't try to combine too many things."
and straker's statement "Thinking about this more, I don't think it matters if the element has aria-hidden or not. So long as the element has tabindex=-1 we should flag it"

In my PR now, the messageKey doesn't mention aria-hidden any more. Only negative tabindex. So does straker's statement render Wilco's statement moot?

@iamrafan
Copy link
Contributor

@WilcoFiers Below are the screen reader test results from testing the examples in this demo: https://codepen.io/iamrafan/pen/OJjRqLg

  Browser/AT Example 1 announcement Example 2 announcement Example 3 announcement Example 4 announcement
1 Edge/NVDA Text-1 button Foo-123 checkbox checked hidden-name-3 button text-3 button Foo-456 checkbox checked
2 Chrome/NVDA Text-1 button Foo-123 checkbox checked hidden-name-3 button text-3 button Foo-456 checkbox checked
3 Firefox/NVDA Text-1 button Foo-123 checkbox checked hidden-name-3 button text-3 button Foo-456 checkbox checked
4 Edge/JAWS Virtual mode off: 'Text -1 button' Virtual mode on: 'Text-1 button'. Then press arrow: nothing is announced Virtual mode off: 'Foo-123 checkbox checked'   Virtual mode on: 'Foo-123 checkbox checked' Then press arrow: nothing is announced Virtual mode off: 'hidden-name-3 text-3 button'   Virtual mode on: 'hidden-name-3 text-3 button' Then press arrow: nothing is announced Virtual mode off:  'Foo-456 checkbox checked'   Virtual mode on:  'Foo-456 checkbox checked' Then press arrow: nothing is announced
5 Chrome/JAWS Virtual mode off: 'Text -1 button'   Virtual mode on: 'Text-1 button'. Then press arrow: nothing is announced Virtual mode off: 'Foo-123 checkbox checked'   Virtual mode on: 'Foo-123 checkbox checked' Then press arrow: nothing is announced Virtual mode off: 'hidden-name-3 text-3 button'   Virtual mode on: 'hidden-name-3 text-3 button' Then press arrow: nothing is announced Virtual mode off:  'Foo-456 checkbox checked'   Virtual mode on:  'Foo-456 checkbox checked' Then press arrow: nothing is announced
6 Edge/Narrator Scan mode off: 'Text -1 button'   Scan mode on: 'Text-1 button'. Then press arrow: 'text-1' Scan mode off: 'Foo-123 checkbox checked'   Scan mode on: 'Foo-123 checkbox checked' Then press arrow: nothing is announced Scan mode off: 'hidden-name-3 text-3 button'   Scan mode on: 'hidden-name-3 text-3 button' Then press arrow: 'text-3' Scan mode off:  'Foo-456 checkbox checked'   Scan mode on:  'Foo-456 checkbox checked' Then press arrow: 'bar-789 checkbox unchecked'
7 Chrome/Narrator Scan mode off: 'Text -1 button'   Scan mode on: 'Text-1 button'. Then press arrow: 'text-1' Scan mode off: 'Foo-123 checkbox checked'   Scan mode on: 'Foo-123 checkbox checked' Then press arrow: 'checkbox unchecked' Scan mode off: 'hidden-name-3 text-3 button'   Scan mode on: 'hidden-name-3 text-3 button' Then press arrow: 'text-3' Scan mode off:  'Foo-456 checkbox checked'   Scan mode on:  'Foo-456 checkbox checked' Then press arrow: 'bar-789 checkbox unchecked'
8 Safari/Voiceover Text-1 button Foo-123, ticked, tick box hidden-name-3 text-3 button Foo-456, ticked, tick box

If I run axe-core on the demo page after pulling in the changes from PR #3194

I would see:

  • Failure description as: Nested interactive controls are not announced by screen readers
  • Fix the following as: Using negative tabindex is not a reliable way of hiding interactive elements

Based on the above AT test results I think it is safe to conclude that the nested elements are not accessible to screen readers, which would make the fix the following message wrong

If we conclude that the nested elements are accessible to screen readers based on the AT tests, the Failure description above would be wrong.

@straker
Copy link
Contributor

straker commented Oct 22, 2021

@iamrafan In VoiceOver and Safari (I haven't tested others), having aria-hidden=true and tabindex=-1 (but specifically the tabindex=-1) can causes issues when reading line by line and letter by letter. I demonstrated this in #3227 (comment), but reading line by line does not cause the button to become focused like it should when you get to the button. Same with letter by letter. Instead it reads the nested child and does not focus the interactive parent. Worse still, it announces that you are on a button even though your focus remains on the last element (such as the checkbox).

@iamrafan
Copy link
Contributor

@straker Sure. Considering the nested element is announced by VoiceOver on Safari, wouldn't the Failure description stating that Nested interactive controls are not announced by screen readers be misleading?

@straker
Copy link
Contributor

straker commented Oct 22, 2021

I see what you mean. Maybe something more along the lines of:

  • help: "Ensure interactive controls are not nested"
  • description: "Nested interactive controls are not always announced by screen readers or can cause focus problems for screen readers"
  • tabindex message: "Using tabindex=-1 on an element inside an interactive control does not prevent a screen reader from focusing the element (even with 'aria-hidden=true')"

@iamrafan
Copy link
Contributor

iamrafan commented Oct 22, 2021

@straker yes, those messages look good. Can you also please update the default message to something like Element has focusable descendants which can cause focus problems for Assistive Technology(screen reader or speech recognition software) users. Then this message would also cover #3232

dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Oct 23, 2021
dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Oct 27, 2021
dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Nov 1, 2021
straker added a commit that referenced this issue Nov 4, 2021
* refactor(checks/navigation): improve `internal-link-present-evaluate`

Make `internal-link-present-evaluate` work with virtualNode rather than actualNode.

Closes issue #2466

* test commit 1

* test commit 2

* test commit 3

* Revert "Merge branch 'dan-test-branch-1' into develop"

This reverts commit 428e015, reversing
changes made to 9f996bc.

* Revert "test commit 1"

This reverts commit 9f996bc.

* fix(rule): allow "tabindex=-1" for rules "aria-text" and "nested-interactive"

Closes issue #2934

* Revert "fix(rule): allow "tabindex=-1" for rules "aria-text" and "nested-interactive""

This reverts commit 30f0e01.

* refactor(check): split no-focusable-content rule into three.

That rule is now:
no-focusable-content-for-aria-text,
no-focusable-content-for-nested-interactive, and
no-focusable-content-for-frame

These three are all copy-and-pastes of each other, so far.

* fix(rule): add custom message for nested-interactive.

aria-hidden and negative tabindex will trigger messageKey.

* style(rule): fix lint errors

Errors were in parent commit.

* fix(no-focusable-content-for-frame): fix locale files

Was broken by recent rename of this check.

* refactor(check): undo recent check rename

Rename check "no-focusable-content-for-frame" back to "frame-focusable-content".

* refactor(check): undo recent split of checks

Recombine checks "no-focusable-content-for-aria-text" and "no-focusable-content-for-nested-interactive" back into "no-focusable-content".

* refactor(check): rename local variable

* fix(rule): make no-focusable-content not consult aria-hidden

no-focusable content now consults only tabindex (for the 'not a reliable way of hiding interactive elements' messageKey check)

* fix(checks/keyboard): make no-focusable-content use messageKey the right way

* refactor(check): misc. renaming and refactoring

* Updating description / messageKey text as per #3163 (comment)

* add unit test for frame-focusable-content check

* update the no-focusable-content unit tests to add a test to capture negative tabindex

* update the nested-interactive integration test to add a failure for elements with negative tabindex values

* fix(no-focusable-content-for-frame): fix locale files

Was broken by recent rename of this check.

* Update lib/rules/nested-interactive.json

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>

* Update lib/checks/keyboard/no-focusable-content.json

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>

* fixing botched merge

* update rule-descriptions.md

* fix locale files

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Nov 20, 2021
…#3194)

* refactor(checks/navigation): improve `internal-link-present-evaluate`

Make `internal-link-present-evaluate` work with virtualNode rather than actualNode.

Closes issue dequelabs#2466

* test commit 1

* test commit 2

* test commit 3

* Revert "Merge branch 'dan-test-branch-1' into develop"

This reverts commit 428e015, reversing
changes made to 9f996bc.

* Revert "test commit 1"

This reverts commit 9f996bc.

* fix(rule): allow "tabindex=-1" for rules "aria-text" and "nested-interactive"

Closes issue dequelabs#2934

* Revert "fix(rule): allow "tabindex=-1" for rules "aria-text" and "nested-interactive""

This reverts commit 30f0e01.

* refactor(check): split no-focusable-content rule into three.

That rule is now:
no-focusable-content-for-aria-text,
no-focusable-content-for-nested-interactive, and
no-focusable-content-for-frame

These three are all copy-and-pastes of each other, so far.

* fix(rule): add custom message for nested-interactive.

aria-hidden and negative tabindex will trigger messageKey.

* style(rule): fix lint errors

Errors were in parent commit.

* fix(no-focusable-content-for-frame): fix locale files

Was broken by recent rename of this check.

* refactor(check): undo recent check rename

Rename check "no-focusable-content-for-frame" back to "frame-focusable-content".

* refactor(check): undo recent split of checks

Recombine checks "no-focusable-content-for-aria-text" and "no-focusable-content-for-nested-interactive" back into "no-focusable-content".

* refactor(check): rename local variable

* fix(rule): make no-focusable-content not consult aria-hidden

no-focusable content now consults only tabindex (for the 'not a reliable way of hiding interactive elements' messageKey check)

* fix(checks/keyboard): make no-focusable-content use messageKey the right way

* refactor(check): misc. renaming and refactoring

* Updating description / messageKey text as per dequelabs#3163 (comment)

* add unit test for frame-focusable-content check

* update the no-focusable-content unit tests to add a test to capture negative tabindex

* update the nested-interactive integration test to add a failure for elements with negative tabindex values

* fix(no-focusable-content-for-frame): fix locale files

Was broken by recent rename of this check.

* Update lib/rules/nested-interactive.json

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>

* Update lib/checks/keyboard/no-focusable-content.json

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>

* fixing botched merge

* update rule-descriptions.md

* fix locale files

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
@DanielTripp
Copy link

Should this issue be closed? I think I fully addressed it with #3194

@WilcoFiers
Copy link
Contributor Author

Yup, a little behind on my grooming work. Thanks Daniel!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue For first-time contributors rule metadata Issues in the rule metadata code (lib/rules)
Projects
None yet
Development

No branches or pull requests

5 participants