w3c / accname

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

Ambiguity in AccName LabelledBy section: "[if] current node is not already 'part of' [sic]…traversal" #209

Open cookiecrook opened 9 months ago

cookiecrook commented 9 months ago

As part of WPT PR #42522, I wrote the following test…

<nav aria-labelledby="r1" class="ex" data-expectedlabel="inception cubed" data-testname="nav with straight recursion">
  <h2 id="r1" aria-labelledby="r2">1st FAIL IF INCLUDED</h2>
  <span id="r2" aria-labelledby="r3">2nd FAIL IF INCLUDED</span>
</nav>
<span id="r3" aria-label="inception cubed">3rd FAIL IF INCLUDED</span>

…which all three implementations fail in exactly the same way:

Gecko, WebKit, Chromium all agree on the computedlabel here:

FAIL message: expected "inception cubed" but got "1st FAIL IF INCLUDED"

While I hope no one will write code like that in production, I believe I am reading the labelledby portion of the algorithm correctly, and that the recursion step allows chaining together aria-labelledby references like this. I asked in the Interop issue:

Do the editors and ARIA implementors agree that the implementations are not to spec? Do implementors have concerns this recursion step of the algorithm?

And received an answer from @twilco that leads me to believe this is an AccName spec ambiguity:

LabelledBy: Otherwise, if the current node has an aria-labelledby attribute that contains at least one valid IDREF, and the current node is not already part of an ongoing aria-labelledby or aria-describedby traversal, process its IDREFs in the order they occur:

Specifically in:

…[if] the current node is not already part of an ongoing aria-labelledby or aria-describedby traversal…

What does "already part of" mean here? Apparently I've interpreted it differently than the implementations. I think it's intended to reference this other (normative? informative?) comment:

Important: Each node in the subtree is consulted only once. If text has been collected from a descendant, but is referenced by another IDREF in some descendant node, then that second, or subsequent, reference is not followed. This is done to avoid infinite loops.

So I interpreted "already part of" to mean "already been computed/used once" in the overall traversal... "to avoid infinite loops."

A more permissive reading seems to match the implementations, and would mean chaining together aria-labelledby to another downstream aria-labelledby is not allowed.

So which is it? Thanks.

JAWS-test commented 9 months ago

Related: https://github.com/w3c/accname/issues/98 and https://github.com/w3c/accname/issues/1

Your example is not correct, because span must not be labeled with aria-labelledby

The example in spec shows that the more permissive reading is correct: "chaining together aria-labelledby to another downstream aria-labelledby is not allowed."

spectranaut commented 9 months ago

@accdc if you agree with the implementation then James says perhaps we should just update the spec

cookiecrook commented 9 months ago

Your example is not correct, because span must not be labeled with aria-labelledby

okay, I'll update that to use some non-generic, but the first heading can be labelled by the span, so if the implementations were following that path, the label would be "2nd FAIL IF INCLUDED" not "1st FAIL IF INCLUDED"

So this issue is still valid, despite my authoring error.

cookiecrook commented 9 months ago

Will add the examples in https://w3c.github.io/accname/#mapping_additional_nd_te

accdc commented 9 months ago

This is going to take a bit of explaining, but what the browsers are doing is correct in that the expected outcome should be "1st FAIL IF INCLUDED".

The reason why goes back to what we decided at the FTF meeting we had years ago at SF when we met at Level Access, when the same ambiguity was discussed.

Basically, the statement "the current node is not already part of an ongoing aria-labelledby or aria-describedby traversal" was decided to mean that there can only be one traversal allowed as part of a root node process, after which it will progress no further down any additional chaining after that. As such, the labelledby process can only ever go 1 level deep.

Weirdly enough, we also decided that this first traversal did not have to start on the root node, but could occur on a child node of that process. E.G. If you had a root node of a button that included a child element that had aria-labelledby on it, this would count as being traversable because there was no prior labelledby iteration currently in action at that time, so all referenced ids would need to be processed 1 level deep as per the spec. Here too though, since no chaining is allowed, it could only do this once.

<button id="test"><span aria-labelledby="l1"></span></button>
<span id="l1">Accessible Name</span>

The issue of not processing the same node twice complicates things, mainly because browsers don't appear to be doing this.

E.G. This should return "Accessible Name" as the button name, but instead returns "Accessible Name Accessible Name" in Chrome.

<button id="test" aria-labelledby="l1 l1"></button>
<span id="l1">Accessible Name</span>

Nobody has given a convincing argument why this should be allowable though, which is why the spec still says what it does.

JAWS-test commented 9 months ago

@accdc:

I agree with @cookiecrook that there is an ambiguity in the wording. I propose to adjust Accname to remove this ambiguity. So far, clarification on this point is provided only via the example. However, the specification should be sufficiently understandable even without the examples.

Your second example (aria-labelledby="l1 l1") is, in my opinion, a different case that is not captured by the phrase given here. I'm not aware that any wording in Accname prohibits multiple reference via aria-labelledby, and don't think it's necessary or useful (e.g., a delete button could be labeled "Delete element1 because element1 is obsolete" vs. "Delete element2 because element2 is sold out" - where aria-labelledby refers 2x to the element name). However, if the second example is to be as you write it, I think this also needs to be added to Accname.

accdc commented 9 months ago

If it's not clear, we can certainly discuss what needs changing in the spec.

There appears to be 2 issues brought up here that would need to be broken out.

  1. Should AccName allow daisy chaining of aria-labelledby and aria-describedby.
  2. Should AccName remove the clause that the same node cannot be processed more than once.

If this is true, can this issue be broken up and added to the agenda so we can go over it in the next meeting?

Also, as an FYI, it is actually allowed to put aria-label or aria-labelledby on a span or div if that element is part of the child recursion process within AccName. This is only disallowed when putting these attributes on a div or span and expecting them to be processed as the root node.

JAWS-test commented 9 months ago

@accdc:

it is actually allowed to put aria-label or aria-labelledby on a span or div if that element is part of the child recursion process within AccName. This is only disallowed when putting these attributes on a div or span and expecting them to be processed as the root node.

Thank you very much for this information. Unfortunately, I was not aware of this and it does not seem very logical to me that this is the case. Is this somewhere in the specification? If not, this must also definitely be added. So, @cookiecrook, your example was correct, it can stay as it is.

accdc commented 9 months ago

Also, I forgot to mention as a bit of context, the reason why daisy chaining was disallowed in prior years is because it would be too easy to daisy chain yourself into a circle by self-referencing the same container which had a child that would reference the parent and so on infinitely.

Combined with browsers not enforcing the omission of nodes that have already been processed would result in the browser crashing as a result.

This last scenario actually happened years ago when AccName 1.0 was being implemented, and is what led to these clauses being added to the spec.

JAWS-test commented 9 months ago

@accdc:

The reason why chaining was disallowed is clear as the spec says

Important: Each node in the subtree is consulted only once. If text has been collected from a descendant, but is referenced by another IDREF in some descendant node, then that second, or subsequent, reference is not followed. This is done to avoid infinite loops.

Avoiding infinite loops is understandable and absolutely necessary.

But the problem is,

Thus, on the one hand, the ambiguity that @cookiecrook addresses remains here, and on the other hand, the rationale for prohibiting aria-labelledby="el1 el1" is wrong.

That is why I suggest that it should be formulated more clearly:

cookiecrook commented 9 months ago

Thanks for the explanation... It seems like there are several ambiguities in the AccName spec.

Among other things, reviewers should have stopped me from naming the step "LabelledBy Recursion" since ~"only do one level deep," is not the definition of recursion. But it is easier to implement...

As a follow-on question for @accdc and @MelSumner, what is the expectation for this name computation:

<dialog aria-labelledby="h">
  <h2 id="h">
    All about <span aria-labelledby="i">➡<img href="#" alt="Bananas" id="i">⬅</span>
  </h2>
  <p>dialog contents</p>
</dialog>

I think by Bryan's explanation, the dialog's computed label should be "All about ➡⬅" even though it's clear the author intended it to be "All about Bananas."

cookiecrook commented 9 months ago

The spec ambiguities as I see it so far...

  1. The OP ambiguity, which can be clarified in prose to more explicitly state it'd not recursive or allowed to follow labelledby more than once (for any reason)...
  2. LabelledBy Recursion (and the WPT test file) should be renamed.
  3. That label/labelledby is allowed on a generic as part of a traversal may be in conflict with "name prohibited", so we have a joint issue between the AccName and ARIA specs...
cookiecrook commented 9 months ago

My understanding of the intention (now problems) was the same as @JAWS-test listed above... That said, I'm not eager to increase the complexity of the implementations to allow ~infinite depth recursion since:

  1. the ARIA WG previously decided it was unnecessary
  2. the implementations all agree on max depth of 1

So I'd rather just fix the AccName prose to match Bryan's understanding, which aligns with the implementations and the prior working group decisions.

JAWS-test commented 9 months ago

I think by Bryan's explanation, the dialog's computed label should be "All about ➡⬅" even though it's clear the author intended it to be "All about Bananas."

And that would be strange in that the heading would then have the correct name, but the dialog referencing the heading would not. Also strange would be that the dialog would have the correct description when referenced by aria-describedby to the heading. The result would thus be different between aria-describedby and aria-labelledby.

@cookiecrook I think, in your example the dialog's name would be "All about ➡ Bananas ⬅" because there is no reason for the alt attribute of the graphic to be ignored.

accdc commented 9 months ago

I think by Bryan's explanation, the dialog's computed label should be "All about ➡⬅" even though it's clear the author intended it to be "All about Bananas."

Actually no, it would be: "All about ➡Bananas⬅"

The reason is for the following:

  1. The first aria-labelledby traversal starts on the dialog and references the element with id="h".
  2. The algorithm then traverses all child nodes normally, regardless of additional aria-labelledby references which are ignored if encountered.

In your example, the aria-labelledby="i" is ignored and the content of the span is traversed in the order that the nodes appear as expected, starting with the unicode character then the image alt and then the second unicode character.

accdc commented 9 months ago

As an FYI, this is already what Chrome is doing, returning "All about ➡ Bananas ⬅" as the accessible name for the container.

accdc commented 9 months ago

I flagged this for the agenda so we can go over it at the next meeting to discuss next steps.

spectranaut commented 8 months ago

Will discuss Nov 9th, or when @accdc can attend

smhigley commented 8 months ago

I realize I'm late to this, but FWIW our production code relies heavily in various places on not daisy-chaining aria-labelledby (i.e. it relies on the spec describing the behavior @accdc wrote, and the current browser behavior)