fix(nested-interactive): add message for negative tabindex #3194
Conversation
Make `internal-link-present-evaluate` work with virtualNode rather than actualNode. Closes issue dequelabs#2466
This reverts commit 9f996bc.
…ractive" Closes issue dequelabs#2934
|
Did I use messageKey the right way? If so, how do I localize it? |
There was a problem hiding this comment.
Thanks for taking on this pr! It's a great start but I think we need to apply the message to the aria-text rule as well.
Testing out how aria-hidden=true and taxindex=-1 works with aria-text and nested-interactive elements:
<div role="text" tabindex="0">
<span>hello</span>
<button aria-hidden="true" tabindex="-1">Bob</button>
</div>
<div role=checkbox aria-checked=true tabindex=0 aria-label=foo>
<button aria-hidden="true" tabindex="-1">Bob</button>
</div>For the role=text element, focusing the element and then pressing down in VoiceOver and Safari results in the letter B being read (even though the text node text is only read as hello). Pressing down again moves off the element, but pressing up goes to the button and reads bob
For the role=checkbox element, focusing the element then pressing down reads the button text.
So for both of these rules we want to inform the user that the element is still reachable, so they can share the same evaluate function. I agree that the frame-focusable rule doesn't need to inform the user of this as what it's checking is different (that a frame with tabindex=1 doesn't have focusable children).
lib/checks/keyboard/no-focusable-content-for-nested-interactive-evaluate.js
Outdated
Show resolved
Hide resolved
lib/checks/keyboard/no-focusable-content-for-nested-interactive-evaluate.js
Outdated
Show resolved
Hide resolved
lib/checks/keyboard/no-focusable-content-for-nested-interactive.json
Outdated
Show resolved
Hide resolved
|
Nice! Thank you for these comments. I'll get started on fixing these issues now. |
|
@straker I think that I've implemented all of your suggestions, in my commits today. Let me know. Thanks again for taking the time to make these suggestions. I've been trying to get up to speed and they helped me a lot. |
|
Thank you again. I think you're doing the hard part here. I hope my latest commit fits the bill. |
Recombine checks "no-focusable-content-for-aria-text" and "no-focusable-content-for-nested-interactive" back into "no-focusable-content".
no-focusable content now consults only tabindex (for the 'not a reliable way of hiding interactive elements' messageKey check)
…lements with negative tabindex values
Was broken by recent rename of this check.
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
|
@straker Thank you for your patience. I'm new to git, and rebase. I think I fixed the merge problems, and that this PR is ready for your consideration again. |
|
But I'm still trying to figure out if I should take any action based on @WilcoFiers' 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." I've tried to come up with a test case for that which is matched by these rules - nested-interactive and aria-text. So far I can't think of such a test case. I wonder if "a second message for role=none|presentation and negative tabindex" would fall under another rule. |
|
We can worry about the presentation / none message in a different pr since this one's already large. |
|
Reviewed for security |
straker
changed the title
|
Great! Thank you again for all the help. |
…#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>
First I split the "no-focusable-content" check into three checks, one for each rule that uses it. That was my best guess on how to do it. If not for #3163, then for #3163 and #2934 later.
Then I added a messageKey for "nested-interactive".
Closes issue: #3163