Skip to content
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

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

Open
cookiecrook opened this issue Oct 13, 2023 · 19 comments
Assignees

Comments

@cookiecrook
Copy link
Contributor

cookiecrook commented Oct 13, 2023

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
Copy link

JAWS-test commented Oct 16, 2023

Related: #98 and #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
Copy link
Contributor

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

@cookiecrook
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@accdc
Copy link
Contributor

accdc commented Oct 19, 2023

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
Copy link

JAWS-test commented Oct 20, 2023

@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
Copy link
Contributor

accdc commented Oct 20, 2023

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
Copy link

@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
Copy link
Contributor

accdc commented Oct 20, 2023

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
Copy link

@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,

  • that this paragraph does not say that no multiple aria-labelledby chaining is forbidden (as you say and the example in the spec), but only a chaining that leads to the same target being reached again, because only then would infinite loops arise
  • that the paragraph does say that per aria-labelledby it is not allowed to reference 2x to the same target (in the sense of aria-labelledby="el1 el1"), although this does not result in infinite loops.

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:

  • If another element has already been referenced by aria-labelledby (or label etc), step B is aborted and step C or the following is executed. Important: The hint with the infinite loops is currently at step F, but rather belongs to step B.
  • if aria-labelledby (or aria-describedby etc.) refers to the same target more than once, only the first reference is followed (this is not necessary to avoid loops, but can be a useful rule. Possibly the rule can also be abolished because browsers do not respect it.)

@cookiecrook
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

cookiecrook commented Oct 22, 2023

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
Copy link

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
Copy link
Contributor

accdc commented Oct 23, 2023

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
Copy link
Contributor

accdc commented Oct 23, 2023

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

@accdc accdc added the Agenda label Oct 23, 2023
@accdc
Copy link
Contributor

accdc commented Oct 23, 2023

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

@spectranaut
Copy link
Contributor

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

@smhigley
Copy link

smhigley commented Nov 9, 2023

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)

@cookiecrook cookiecrook self-assigned this Nov 9, 2023
aarongable pushed a commit to chromium/chromium that referenced this issue May 31, 2024
In [1] the change was made to use a copy of the visited set for
the aria-labeledby traversal, so that nodes added in that traversal
would not persist in the visited set used later in name
computation. This was done for the sake of the name sources reported
to the inspector protocol: the intent seems to have been that if a
node was encountered in the labeledby traversal, it should still
be able to show up as a superseded name source later in the traversal.

This is causing functionality issues however, since it can cause the
text of a node to be used twice, for example in the test
comp_name_from_content.html [2] which is part of Interop2024.

Hence in this CL, stop using a thrown-away copy of the visited set
for the aria-labeledby traversal.

Note, this affects some tests in
comp_name_from_content.tentative.html, where there is not yet
consensus on the correct behavior. See discussion in [3]. Depending
on how [3] is settled, we may eventually need to maintain two
"visited" sets:
- One to track the elements we've visited so far in a given labeledby
  traversal, which gets thrown away after that traversal.
- One to track the elements whose text we've used (including in a
  labeledby traversal), which is maintained for the entire name
  calculation.

But until there's consensus on that behavior, I want to avoid this
further complexity. So, this CL aims to do the simplest thing that
aligns us with the subset of behavior agreed on for Interop2024.

[1] https://codereview.chromium.org/1939303002
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/accname/name/comp_name_from_content.html;l=221;drc=bd2cb163ac4cbeb1676c8399c37b92cb7a3db6be
[3] w3c/accname#209


Bug: 338398669
Change-Id: I4fd5e86169fe417a5d99fd1cf37f3ad358aa9c52
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5467872
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1308792}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants