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

"role=text": no focusable descendants #2934

Closed
tujoworker opened this issue May 15, 2021 · 8 comments
Closed

"role=text": no focusable descendants #2934

tujoworker opened this issue May 15, 2021 · 8 comments
Labels
fix Bug fixes pr A pr has been created for the issue rules Issue or false result from an axe-core rule
Milestone

Comments

@tujoworker
Copy link

tujoworker commented May 15, 2021

I really love that "role=text" made it in to axe-core. So I would like to thank everyone involved at first 🙏

PR: #2702

Product: axe-core

Expectation: Allow to have "tabindex=-1" as a descendants of "role=text"

Actual: Ensures "role=text" is used on elements with no focusable descendants

Motivation: I use "tabindex=-1" to let JavaScript set the focus to a descendant to make a copy to the clipboard. I use "role=text" to let VoiceOver (and others) read the content without destruction.

Simple example (fails):

<p>
  some text
  <span role="text">
    more text
    <span tabindex="-1">JavaScript is able to focus this</span>
  </span>
</p>

Second example (fails):

<p>
  some text
  <span role="text">
    more text
    <span tabindex="-1" aria-hidden="true" class="visually-hidden">JavaScript is able to focus this</span>
  </span>
</p>

axe-core version: 4.2

@WilcoFiers
Copy link
Contributor

Interesting use case. Can't say I've ever seen it used that way. We'll have a look. I'm not sure about the case where it is visible. A screen reader user using touch screen can still focus elements with tabindex="-1", that's kind of an edge case though. And it wouldn't be relevant if the element is off screen.

@WilcoFiers WilcoFiers added fix Bug fixes rules Issue or false result from an axe-core rule labels Aug 12, 2021
@WilcoFiers
Copy link
Contributor

I think we need to change this rule to not consider elements with negative tabindex to be focusable. It is technically true, but difficult to explain, and a very unusual edge case to handle.

@tujoworker
Copy link
Author

This would be wonderful 🙏🏻✨

@dan-tripp
Copy link
Contributor

dan-tripp commented Sep 11, 2021

I would like to fix this issue. It seems to me that there are two ways to fix it:

  1. change only the "aria-text" rule to not consider elements with negative tabindex to be focusable, or
  2. change all axe-core code to not consider elements with negative tabindex to be focusable.

(2) would have more side effects, such as in lib/commons/aria/get-role.js: the hasConflictResolution(vNode) function. Any guidance, @WilcoFiers ?

@WilcoFiers
Copy link
Contributor

@dan-tripp I think the simplest solve would be to test the tabindex in the focusable-element-evaluate check, similar to what we do in focusable-element-evaluate.js

const isFocusable = virtualNode.isFocusable;
let tabIndex = parseInt(virtualNode.attr('tabindex'), 10);
tabIndex = !isNaN(tabIndex) ? tabIndex : -1

if (isFocusable && tabindex >= 0) {

@dan-tripp
Copy link
Contributor

Right on. I'm working on it.

@dan-tripp
Copy link
Contributor

I must confess that I strayed from your advice @WilcoFiers - my PR uses "isFocusable(vNode)" rather than "vNode.isFocusable" because the latter seemed to break "nested-interactive virtual-rule". I don't know why. Anyway, I hope my PR helps.

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>
@WilcoFiers WilcoFiers added this to the Axe-core 4.4 milestone Nov 10, 2021
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>
dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Nov 28, 2021
WilcoFiers added a commit that referenced this issue Nov 29, 2021
* test commit 1

* Revert "test commit 1"

This reverts commit 9f996bc.

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

Closes issue #2934

* feat(rule): add new rule color-contrast-enhanced (WCAG AAA)

Work in progress.

* Work in progress.

* Work in progress.

* Updating shadow-dom test for WCAG AAA contrast.

* Rearranging shadow-dom test.

* Adding new rule color-contrast-enhanced to code-highlighting test.

* adding new 'color-contrast-enhanced' rule to sticky-header test.

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

This reverts commit 30f0e01.

* update rule-descriptions.md

* fix whitespace

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

* work in progress: splitting tests for contrast and the new contrast-enhanced.

* work in progress: tests

* work in progress: tests

* work in progress: tests

* adding separate tests for color-contrast-enhanced.  work in progress.

* update rule-descriptions.md

* add test: test/integration/full/contrast-enhanced/simple.html

* remove redundant tests for contrast-enhanced

* remove temporary code

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
dan-tripp added a commit to dan-tripp/axe-core that referenced this issue Nov 30, 2021
WilcoFiers pushed a commit that referenced this issue Dec 6, 2021
…th no role (#3165)

* 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

* work in progress

* work in progress

* test commit 1

* Revert "test commit 1"

This reverts commit 9f996bc.

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

Closes issue #2934

* work in progress

* work in progress

* fix whitespace

* add new case to test test/checks/keyboard/no-focusable-content.js

* change "disabled" test case in test/checks/keyboard/no-focusable-content.js

* fix merge problem
@WilcoFiers WilcoFiers added the pr A pr has been created for the issue label Dec 14, 2021
@padmavemulapati
Copy link

Verified with the latest dev branch code , role=text is allowing to have "tabindex=-1" as a descendants

image

straker added a commit that referenced this issue Feb 22, 2022
…s as disabled (#3393)

* 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

* work in progress

* work in progress

* test commit 1

* Revert "test commit 1"

This reverts commit 9f996bc.

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

Closes issue #2934

* work in progress

* work in progress

* fix whitespace

* add new case to test test/checks/keyboard/no-focusable-content.js

* change "disabled" test case in test/checks/keyboard/no-focusable-content.js

* fix merge problem

* fix(commons/dom): focusDisabled() behavior

Now checks "disabled" attribute

Closes issue #3315

* Update lib/commons/dom/focus-disabled.js

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

* fix typo

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
straker pushed a commit that referenced this issue Feb 28, 2022
…les, add role to message (#3395)

* 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

* work in progress

* work in progress

* test commit 1

* Revert "test commit 1"

This reverts commit 9f996bc.

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

Closes issue #2934

* work in progress

* work in progress

* fix whitespace

* add new case to test test/checks/keyboard/no-focusable-content.js

* change "disabled" test case in test/checks/keyboard/no-focusable-content.js

* fix merge problem

* fix(rules): unsupportedrole check bugs

Closes issue #3282

* lint fix

* add assertion on checkContext._data to unit tests

* refactor(checks/unsupportedrole): change this.data from using array to single string

to match lib/checks/aria/deprecatedrole-evaluate.js

* Revert "refactor(checks/unsupportedrole): change this.data from using array to single string"

This reverts commit e7c757f9330b635b1ed16385b3526f2e71786488.

* refactor(checks/unsupportedrole): change this.data from using array to single string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes pr A pr has been created for the issue rules Issue or false result from an axe-core rule
Projects
None yet
Development

No branches or pull requests

4 participants