Implement minimal accessibility snapshots #236
Conversation
calebeby
requested review from
spaceninja,
gerardo-rodriguez and
Paul-Hebert
| InaccessibleSelfAndDescendents, | ||
| } | ||
|
|
||
| // TODO in PR: what about role="presentation" or role="none"? Should they be excluded? |
There was a problem hiding this comment.
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
The following elements are not exposed via the accessibility API and user agents MUST NOT include them in the accessibility tree:
...
- Elements with none or presentation as the first role in the role attribute. However, their exclusion is conditional. In addition, the element's descendants and text content are generally included. These exceptions and conditions are documented in the presentation (role) section.
https://www.w3.org/TR/wai-aria-1.2/#presentation
The intended use is when an element is used to change the look of the page but does not have all the functional, interactive, or structural relevance implied by the element type, or may be used to provide for an accessible fallback in older browsers that do not support WAI-ARIA.
For any element with a role of presentation and which is not focusable, the user agent MUST NOT expose the implicit native semantics of the element (the role and its states and properties) to accessibility APIs. However, the user agent MUST expose content and descendant elements that do not have an explicit or inherited role of presentation. Thus, the presentation role causes a given element to be treated as having no role or to be removed from the accessibility tree, but does not cause the content contained within the element to be removed from the accessibility tree.
There was a problem hiding this comment.
Oh yeah, weird. MDN makes it even more complicated:
The presentation role is used to remove semantic meaning from an element and any of its related child elements. For example, a table used for layout purposes could have the presentation role applied to the table element to remove any semantic meaning from the table element and any of its table related children elements, such as table headers and table data elements. Non-table related elements should retain their semantic meaning, however.
Happy to chat more about this, though your quotes made me realize I've been misunderstanding role="presentation" (and made me grateful for just using aria-hidden instead 😅 )
There was a problem hiding this comment.
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.
FWIW I'm planning to research/ask about this and address it in a follow-up PR
There was a problem hiding this comment.
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.
@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:
region
presentation
heading "hello"
button "click"
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.
FWIW I'm planning to research/ask about this and address it in a follow-up PR
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.
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.
Yeah I think a chat would be good. I could do tomorrow afternoon anytime?
There was a problem hiding this comment.
This is fantastic @calebeby ! Really, really nice work.
This all looks good to me. I left a few nits inline but nothing major. I'd encourage you to get reviews from other team members on this feature as well since they may have other helpful feedback.
| InaccessibleSelfAndDescendents, | ||
| } | ||
|
|
||
| // TODO in PR: what about role="presentation" or role="none"? Should they be excluded? |
There was a problem hiding this comment.
You may want to flag this in our internal #a11y channel or the A11y slack to get some more expertise
| .toMatchInlineSnapshot(` | ||
| button "click me" | ||
| ↳ description: "extended description" | ||
| text "click me" |
There was a problem hiding this comment.
I wonder if this should be included when includeText is true since the text is already displayed in the accessible name? I'm not sure what the right answer is. Curious what you and others think.
There was a problem hiding this comment.
I thin you probably have it right as-is. Things could get confusing quickly if you start omitting text that's used as a name/description.
There was a problem hiding this comment.
Good question! Ideally I would like it not to be shown. But dom-accessibility-api doesn't yet have a way to see which nodes it already used when computing the name/description: eps1lon/dom-accessibility-api#728. I'll open a PR there so we can have that feature.
Also, FWIW chrome's accessibility tree shows the text nodes in cases like this, even if they are already used in computing the name/description.
There was a problem hiding this comment.
Yeah that makes sense. I think Chrome might be doing it right. I'd rather be verbose than remove things in a way that could be confusing (and I could see this becoming confusing with aria-labelledby in the mix.)
There was a problem hiding this comment.
I think there are some cases where the text is "used up" in computing the name, and the screen reader can't read the text separately from the name. Ideally in those cases we wouldn't show text. I'll think about it more to see if there is a consistent way to make this work. (outside of this PR)
|
cc @gerardo-rodriguez or @spaceninja do you mind taking a look as well? |
I can help review as well! I probably won't be able to get to this until late today or Monday, FYI. |
|
@gerardo-rodriguez @Paul-Hebert I think I've gotten to all the feedback, except for the role="presentation" conversation which can be a follow up PR |
Cool, my approval still stands. This is looking great 🙂 |
I am leaving the feature undocumented until it is no longer experimental