fix(color-contrast): add special case for new sr-only technique #2985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WilcoFiers
previously requested changes
Jun 10, 2021
|
Score +1 for proper testing; in firefox the false positive described in the ticket doesn't show up at all due to differences in the bounding rect calculations, leading me to think my code fixed the issue. Pushed a better fix that adds a new special case to the sr-only detection, and added a test for it. |
clottman
changed the title
WilcoFiers
previously requested changes
Jun 14, 2021
lib/commons/dom/is-visible.js
Outdated
| if ( | ||
| !screenReader && | ||
| (isClipped(style) || | ||
| style.getPropertyValue('opacity') === '0' || | ||
| (getScroll(el) && parseInt(style.getPropertyValue('height')) === 0)) | ||
| (getScroll(el) && elHeight === 0) || |
There was a problem hiding this comment.
Is there a reason you left this at 0?
There was a problem hiding this comment.
Yes, turns out it wasn't relevant to the case I was trying to address so I left it alone. Would you rather it be bumped up too?
lib/commons/dom/is-visible.js
Outdated
Comment on lines
164
to
167
| (getScroll(el) && elHeight === 0) || | ||
| (style.getPropertyValue('position') === 'absolute' && | ||
| elHeight <= 1 && | ||
| style.getPropertyValue('overflow') === 'hidden')) |
There was a problem hiding this comment.
6 line if statement's a bit much. Can you isolate some of the logic on this?
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
WilcoFiers
approved these changes
Jun 15, 2021
WilcoFiers
added a commit
that referenced
this pull request
Jun 17, 2021
* bump up height requirement for sr-only to 1px * add special case for sr-only method * use less than 2 Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com> * make if smaller Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
clottman
added a commit
that referenced
this pull request
Jun 17, 2021
* bump up height requirement for sr-only to 1px * add special case for sr-only method * use less than 2 Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com> * make if smaller Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
straker
pushed a commit
that referenced
this pull request
Jun 22, 2021
* bump up height requirement for sr-only to 1px * add special case for sr-only method * use less than 2 Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com> * make if smaller Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The minimum CSS that makes the element from the example visually hidden is a combination of:
So if the user is doing that, they are probably trying to hide the element visually and axe-core should recognize that.
Note that the repro case will not fail in Firefox even without this code.
Closes #2962