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

feat(rule): Scrollable region focusable #1396

Merged
merged 17 commits into from
May 14, 2019
Merged

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Mar 1, 2019

Rule which checks if scrollable region has keyboard access.

Closes issue: #96

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: Steve

straker
straker previously requested changes Mar 4, 2019
lib/checks/keyboard/focusable-element.js Outdated Show resolved Hide resolved
test/checks/keyboard/focusable-content.js Outdated Show resolved Hide resolved
test/checks/keyboard/focusable-element.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy changed the title feat(rule): Scrollable region focusable [WIP] feat(rule): Scrollable region focusable Mar 5, 2019
@jeeyyy jeeyyy changed the title [WIP] feat(rule): Scrollable region focusable feat(rule): Scrollable region focusable Mar 5, 2019
@jeeyyy jeeyyy marked this pull request as ready for review March 5, 2019 13:23
@jeeyyy jeeyyy requested a review from a team as a code owner March 5, 2019 13:23
@jeeyyy jeeyyy requested a review from straker March 5, 2019 13:24
straker
straker previously approved these changes Mar 5, 2019
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

LGTM

lib/checks/keyboard/focusable-element.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy requested a review from WilcoFiers April 3, 2019 16:29
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

I'm concerned about false positives in this one. Even a single pixel over overflow causes this rule to match the scrollable region. We're not actually looking to see if any of content is hidden. I think that's important. Thinking about this a little more, I'm wondering if we shouldn't exclude elements with overflow:hidden, and elements with the overflow area is small, maybe anything less than 1em or something. Edges get clipped all the time. Please come up with a proposal for this problem.

lib/rules/scrollable-region-focusable.json Outdated Show resolved Hide resolved
lib/rules/scrollable-region-focusable-matches.js Outdated Show resolved Hide resolved
lib/core/utils/get-scroll.js Show resolved Hide resolved
lib/rules/scrollable-region-focusable-matches.js Outdated Show resolved Hide resolved
lib/checks/keyboard/focusable-content.json Outdated Show resolved Hide resolved
lib/rules/scrollable-region-focusable.json Outdated Show resolved Hide resolved
test/checks/keyboard/focusable-content.js Outdated Show resolved Hide resolved
test/checks/keyboard/focusable-element.js Outdated Show resolved Hide resolved
test/core/utils/get-scroll.js Show resolved Hide resolved
@dylanb
Copy link
Contributor

dylanb commented Apr 12, 2019

@JKODU who has the ball on this PR?

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Apr 23, 2019

@dylanb it has been reviewed by @WilcoFiers. I am picking up with the changes requested. Should be ready for another round soon.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

FYI, I didn't review the test cases yet. But I'm done for today, so I figured I'd share what I've got.

lib/core/utils/get-scroll.js Outdated Show resolved Hide resolved
lib/core/utils/get-scroll.js Outdated Show resolved Hide resolved
lib/rules/scrollable-region-focusable-matches.js Outdated Show resolved Hide resolved
lib/rules/scrollable-region-focusable-matches.js Outdated Show resolved Hide resolved
lib/rules/scrollable-region-focusable-matches.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy changed the title feat(rule): Scrollable region focusable [WIP] feat(rule): Scrollable region focusable Apr 30, 2019
@jeeyyy jeeyyy requested a review from WilcoFiers May 1, 2019 14:42
@jeeyyy jeeyyy changed the title [WIP] feat(rule): Scrollable region focusable feat(rule): Scrollable region focusable May 1, 2019
@jeeyyy jeeyyy changed the title feat(rule): Scrollable region focusable [WIP] feat(rule): Scrollable region focusable May 1, 2019
@jeeyyy jeeyyy changed the title [WIP] feat(rule): Scrollable region focusable feat(rule): Scrollable region focusable May 2, 2019
@jeeyyy jeeyyy merged commit 861371a into develop May 14, 2019
@jeeyyy jeeyyy deleted the scrollable-region-focusable branch May 14, 2019 16:57
@scurker scurker mentioned this pull request May 15, 2019
4 tasks
@kensgists
Copy link

This new rule is causing standard accessible text clipping techniques to fail. For example, this CSS:

.accessible-text { position: absolute !important;
clip: rect(0.0625rem 0.0625rem 0.0625rem 0.0625rem);
clip: rect(0.0625rem, 0.0625rem, 0.0625rem, 0.0625rem);
padding: 0 !important;
border: 0 !important;
height: 0.0625rem !important;
width: 0.0625rem !important;
overflow: hidden;
}`

is marked as a violation in axe-coconut.

@WilcoFiers pointed out above that elements with overflow:hidden might be exempted. I think this is a good idea.

We use axe-coconut for testing because of some of the useful experimental rules. But now we are getting vast numbers of false positives due to this new rule.

MichaelKetting added a commit to re-motion/Framework that referenced this pull request Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants