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.
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
Implement minimal accessibility snapshots #236
Implement minimal accessibility snapshots #236
Changes from 3 commits
8d172f7
7b8ba5c
7415492
1437e99
3f10604
cdde2fe
890a8e2
0dda349
cfa31c2
b900e92
1dcaf7a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Flagging this for review.
My brain has only parsed this like 20% so I don't really understand it yet. The current implementation in this PR is definitely wrong. I will either make it match the spec in this PR or in a follow-up.
If somebody understands this well (w.r.t. how to treat child elements) feel free to explain it to me!
https://www.w3.org/TR/wai-aria-1.2/#tree_exclusion
https://www.w3.org/TR/wai-aria-1.2/#presentation
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.
Oh yeah, weird. MDN makes it even more complicated:
Happy to chat more about this, though your quotes made me realize I've been misunderstanding
role="presentation"
(and made me grateful for just usingaria-hidden
instead 😅 )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.
You may want to flag this in our internal #a11y channel or the A11y slack to get some more expertise
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.
FWIW I'm planning to research/ask about this and address it in a follow-up PR
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.
A question I have is what is the goal here?
For example, is the goal of the accessibility tree test to validate I purposefully added a
presentation
role
to an HTML element? If yes, then it seems it'd be helpful to have this information included in the snapshot. 🤔Did I make sense?
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.
@gerardo-rodriguez Good point. Here is how I think about it:
If role="presentation" worked the same way as role="none" (which I don't know if they are but just for the sake of explanation), I would consider which one I was using to be an implementation detail, so they snapshots should be the same regardless of which I'm using. So this makes me reluctant to display a presentation role for
role="presentation"
like we do for other roles:It's unclear to me exactly what role="presentation" does to descendent elements. From the MDN page Paul quoted, it seems like descendents that are related to the element with role="presentation" get their roles removed? And maybe other things happen too? Ideally the snapshot would encompass all the changes to the accessibility tree that are described in the spec
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.
I am wondering if we should have a quick Slack call to better understand and/or resolve this with @cloudfour/dev? Or, @calebeby, do you feel your original request/questions have been answered?
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.
I'd be happy to chat. I'm also curious how the presentation role affects its children, and how we should display that info
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.
Yeah I think a chat would be good. I could do tomorrow afternoon anytime?
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.
Same 👍