w3c / accname

Accessible Name and Description Computation
https://w3c.github.io/accname/
61 stars 23 forks source link

Add steps for shadow roots and slots #167

Closed nolanlawson closed 6 months ago

nolanlawson commented 1 year ago

Fixes #20 and #93 (related: #51).

Clarifies how accessible names are calculated for shadow roots and slots.

I believe this is accurate in terms of what UAs actually do to calculate the accessible name inside of shadow roots and slots, although WPT tests are probably needed.

Implementation


Preview | Diff

nolanlawson commented 1 year ago

Just chatted with @spectranaut – the suggestion was that "displayed child nodes" is probably the wrong phrase here, and maybe "rendered child nodes" is more accurate. I'm happy with either one; naming things is hard. πŸ™‚

jnurthen commented 1 year ago

@jcsteh can you please look at this

nolanlawson commented 1 year ago

I'm not sure this addresses all of https://github.com/w3c/accname/issues/51 since the submitter asked about aria-labelledby, which would presumably use a static element array when pointing into the shadow root.

The current accname spec says:

if computing a name, and the current node has an aria-labelledby attribute that contains at least one valid IDREF […]

I think potentially there is nothing accname needs to do here. If an IDREF crosses a shadow boundary, then by definition it is not valid.

Perhaps this should go into the definition of an IDREF? I see that in the aria spec, it says:

ID reference Reference to the ID of another element in the same document

It seems to me that this should read "in the same document or shadow root."

nolanlawson commented 1 year ago

OTOH there is the related question of what happens if a <slot> element itself has an aria-labelledby or aria-describeddby (or aria-label, for that matter).

(Previous, erroneous comment) Whipping up [a quick CodePen](https://codepen.io/nolanlawson-the-selector/pen/eYrMBWj), and looking at the Chrome/Firefox Accessibility DevTools and VoiceOver+Safari, it looks like: - Firefox ignores the `aria-labelledby` - Chrome and Safari consider it a valid label ~~This _feels_ like a bug to me (in which case, Firefox's behavior is correct and the language in my PR is correct). But I admit I'm not sure.~~ On second thought, I think my current PR language matches Chrome's and Safari's behavior – ``s are not treated as special, except that their assigned nodes supersede their child nodes for the purposes of traversing the DOM. Maybe this behavior is fine; it seems best not to treat ``s as special w.r.t. `aria-labelledby`/`aria-label`/etc.

I've opened a separate issue to track this: https://github.com/w3c/accname/issues/173

nolanlawson commented 1 year ago

which would presumably use a static element array when pointing into the shadow root

Just grokked this point. For IDREF reflection (https://github.com/whatwg/html/pull/7934) – i.e. ariaLabelledByElements, ariaDescribedByElements, and friends – it looks like yes, we need to add explicit steps for that.

I think potentially this could be handled in a separate PR, since it's not strictly about shadow DOM. E.g. with el1.ariaLabelledByElements = [el2], both el1 and el2 could be in light DOM.

spectranaut commented 1 year ago

Also can you confirm, this algorithm represents what Safari and Firefox already do, but Chrome does not?

And here is the chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1374358

nolanlawson commented 1 year ago

@spectranaut

Also can you confirm, this algorithm represents what Safari and Firefox already do, but Chrome does not? And here is the chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1374358

Correct, based on my testing (https://github.com/w3c/accname/issues/173), Safari and Firefox ignore the aria-label on the <slot>, whereas Chrome does not.

Based on https://github.com/w3c/html-aam/issues/440 though, it seems like we want to cover "elements that cannot be named" (e.g. <slot>) outside of the accname spec, in the html-aam spec instead. So I didn't handle it in this PR.

FWIW I did also open a PR to add some basic accname tests for shadow DOM and slots: https://github.com/web-platform-tests/wpt/pull/36541 . It broadly covers what this PR covers (although not aria-description).

rniwa commented 1 year ago

As discussed during AOM meeting, it's important that there is no JS API that exposes the computed accessibility names that emanate out of a shadow DOM since that would violate shadow DOM's encapsulation. And so far we've concluded there is none so we're good here.

nolanlawson commented 1 year ago

I did another read-through of the comments. I think most of the feedback has been addressed, and the PR on WPT should cover this PR: https://github.com/web-platform-tests/wpt/pull/36541

ariaLabelledByElements/ariaDescribedByElements is trickier, and is being handled in #170. There is also the related question of ElementInternals (https://github.com/w3c/core-aam/issues/152) and whether <slot>s can be named (https://github.com/w3c/html-aam/issues/440). But I don't believe these need to be solved to solve traversal for shadow roots / <slot>s (this PR).

jnurthen commented 1 year ago

@MelSumner @accdc Does this need anything else to get this merged? It seems like all the review comments have been addressed.

accdc commented 1 year ago

My vote is to merge. I'll wait for Melanie to confirm and then we are good to go.

nolanlawson commented 1 year ago

@cookiecrook No worries, thank you for the review!

jnurthen commented 1 year ago

@spectranaut are your review comments addressed?

Jym77 commented 1 year ago

(semi-randomly finding that now) Isn't this trying to redefine the flat tree of CSS? Shouldn't we just change step 2.F.iii from

For each child node of the current node

to

For each child node of the current node in the flat tree

that should already pretty much take care of all the shadow DOMβ€―magic πŸ€”

nolanlawson commented 11 months ago

@Jym77 I wasn't even aware that there was a concept of a "flattened tree." The link you provide is from the CSS spec – if there's something in the HTML spec, maybe we could reference it here to simplify the logic?

nolanlawson commented 11 months ago

BTW the WPT tests for this PR have already been merged: https://github.com/web-platform-tests/wpt/pull/36541

Maybe we can merge and then refactor later as necessary?

Jym77 commented 11 months ago

@Jym77 I wasn't even aware that there was a concept of a "flattened tree." The link you provide is from the CSS spec – if there's something in the HTML spec, maybe we could reference it here to simplify the logic?

I'm not aware of anything like that in HTML (or DOM) specs. I agree it would be better to have this definition there rather than in CSS... From what I gather, CSS needs the concept for inheritance (second paragraph), but DOM or HTML never directly need that concept 😞
In ACT rules, we've been using "flat tree" with link to CSS for some times, e.g. in the Applicability of Text has minimum contrast (and many other rules).

jcsteh commented 11 months ago

Realistically, accessibility implementations depend on layout rendering to some extent; e.g. display: none, visibility: hidden. Given that, it's probably not unreasonable to refer to a CSS concept here. FWIW, Gecko's accessibility engine internally uses the flat tree to build the accessibility tree and I assume other browsers might do similar.

alice commented 11 months ago

Gecko's accessibility engine internally uses the flat tree to build the accessibility tree and I assume other browsers might do similar.

Blink does the same (technically the "layout tree builder traversal" which includes pseudo-elements, but otherwise just uses the flat tree traversal), since the whole display: contents incident. (Previously it walked the layout tree directly, which mostly amounts to the same thing anyway.)

MelSumner commented 7 months ago

I am not sure what we want to do about this so I'm going to mark it for the agenda, and read through it all in the meantime.

spectranaut commented 6 months ago

This looks read to land, @jnurthen will resolve conflicts