-
Notifications
You must be signed in to change notification settings - Fork 765
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
fix(region): Decorative images ignored by region rule #4412
Changes from 2 commits
ace25cd
7c810e2
70b6ddb
f914665
be633af
d515329
bc6dc71
b29a90d
53782f2
8938496
738e9e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -45,14 +45,19 @@ function getRegionlessNodes(options) { | |||||
*/ | ||||||
function findRegionlessElms(virtualNode, options) { | ||||||
const node = virtualNode.actualNode; | ||||||
// End recursion if the element is a landmark, skiplink, or hidden content | ||||||
// End recursion if the element is... | ||||||
// - a landmark | ||||||
// - a skiplink | ||||||
// - hidden content | ||||||
// - a presentation graphic | ||||||
if ( | ||||||
getRole(virtualNode) === 'button' || | ||||||
isRegion(virtualNode, options) || | ||||||
['iframe', 'frame'].includes(virtualNode.props.nodeName) || | ||||||
(dom.isSkipLink(virtualNode.actualNode) && | ||||||
dom.getElementByReference(virtualNode.actualNode, 'href')) || | ||||||
!dom.isVisibleToScreenReaders(node) | ||||||
!dom.isVisibleToScreenReaders(node) || | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This brings up an interesting question. Should a presentational img using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You know I was wondering that, it was a path I started diving down and then uncertainty got the best of me. It's a function leveraged in a lot of places. If we think that's the right path I'm happy to try it out and see if any other test failures come up along the way that make us second guess it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair question, but no I don't think so. Conceptually, there's a difference between a hidden element and an element that's ignored. It matters for example on |
||||||
isPresentationGraphic(virtualNode) | ||||||
) { | ||||||
// Mark each parent node as having region descendant | ||||||
let vNode = virtualNode; | ||||||
|
@@ -86,6 +91,10 @@ function findRegionlessElms(virtualNode, options) { | |||||
} | ||||||
} | ||||||
|
||||||
function isPresentationGraphic(virtualNode) { | ||||||
return virtualNode.props.nodeName === 'img' && virtualNode.attr('alt') === ''; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a little incomplete. There are other graphics, and there are more ways for an image to be decorative. We have methods we can use to figure this stuff out:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I agree, alas I'm a bit unsure which existing methods are the most appropriate.
Think I'll post asking for pairing assistance tomorrow to learn how to fish for the right answer on this one :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WilcoFiers I paired with Steve and we generally agree that it'd be nice to make this check more broad, however We would both love to circle back with you on Monday @WilcoFiers about what we'd like to do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a call we discussed we wanted to check for the tags My findings copied from Slack below: It (
I tried adjusting all of these places but it got pretty hairy and makes me think this was all set up intentionally the way it was, and it may be the wrong thing to use. So this PR is stuck assuming on this comment thread currently without a clear path forward. I believe Steve and Wilco are busy so, hi @dbjorge any thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the real question is the following:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thoughts on the problem
I think this ought to come down to "is the image in question going to be exposed to ATs or not"; I think the result of that is that I agree with the other comment chain that it's confusing that the use of The difficulty with updating However, that's not the case here! The mechanism by which
What I suggest to resolve itI lean towards updating const role = getRole(virtualNode, { noPresentational: true });
const isPresentational = role === null
// The element itself is not visible to screen readers, but its descendants might be
const isShallowlyHidden = isPresentational && !hasChildTextNodes(elm) ...and then update the conditional that checks for You could make a reasonable argument that the better fix would be to add Footnotes1: This has a few caveats:
2: axe-core has a few places where this logic comes up, including the caveats from note 1; see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly addressed in 524f0be I played with adding some manual checks for the caveats of global aria attributes and focusable, |
||||||
} | ||||||
|
||||||
// Check if the current element is a landmark | ||||||
function isRegion(virtualNode, options) { | ||||||
const node = virtualNode.actualNode; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a few more tests here about other elements:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We intentionally allow svg's outside of regions fab58d4bf#diff-b11ab1ec32c4f5c0e17de0c9a1f3c5a9835bcc133f673b5da3ecaaf66a932077 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the other tests as both unit and integration tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an integration test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added :)