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

How should labeledby calculation handle whitespace-only content? #233

Open
dandclark opened this issue Mar 15, 2024 · 5 comments
Open

How should labeledby calculation handle whitespace-only content? #233

dandclark opened this issue Mar 15, 2024 · 5 comments
Assignees

Comments

@dandclark
Copy link

I'm looking at the test case "button's hidden referenced name (visibility:hidden) with hidden aria-labelledby traversal falls back to aria-label" in the WPT accname/name/comp_label.html.

<button aria-labelledby="span4" aria-label="foo" data-expectedlabel="foo" data-testname="button's hidden referenced name (visibility:hidden) with hidden aria-labelledby traversal falls back to aria-label" class="ex">
  <span id="span4">
    <span id="span5" style="visibility:hidden;">label</span>
  </span>
  x
</button>

One problem is that the span4 element ID is duplicated from the previous test; I'm fixing that in web-platform-tests/wpt#45117.

In Chromium, the test still fails because Blink includes the whitespace under span4 as span4's a contribution to the LabeledBy Recursion in step 2B LabeledBy. So we end up using that whitespace as the computed name, instead of falling back to the aria-label, and the test fails.

If I delete the whitespace under span4 then the test passes.

The spec language doesn't say anything here about not counting whitespace:

  • i. Set the accumulated text to the empty string.
  • ii. For each IDREF:
    • a. Set the current node to the node referenced by the IDREF.
    • b. LabelledBy Recursion: Compute the text alternative of the current node beginning with the overall Computation step. Set the result to that text alternative.
    • c. Append a space character and the result to the accumulated text.
  • iii. Return the accumulated text if it is not the empty string ("").

So per the spec, Chromium's behavior seems to be correct and the test incorrect, but I don't think that's the intent. Should the spec have a step to strip whitespace from the accumulated text in step iii before deciding whether not to return it?

aarongable pushed a commit to chromium/chromium that referenced this issue May 17, 2024
… empty

This subtest is failing because some of the whitespace in the
aria-labelledby traversal is non-collapsed and shows up in the result,
which prevents the aria-label from being applied.

Fix this by only using the aria-labelledby result only if it is
non-empty and non-whitespace-only.

Note, the spec [1] does not explicitly state that whitespace should
be ignored here. However, other engines do ignore the whitespace; see
test results at [2]. And the spec *does* say that whitespace should
be ignored in aria-label [3]. So I think there's a good chance the
spec will be changed for this.

I filed a spec issue on this at [4]. That may take some time to
resolve, so in the meantime I think the right thing to do is to align
with the behavior of other browsers; hence this CL.

[1] https://w3c.github.io/accname/#comp_labelledby
[2] https://wpt.fyi/results/accname/name/comp_label.html?label=experimental&label=master&aligned&view=interop&q=label%3Ainterop-2024-accessibility
[3] https://w3c.github.io/accname/#comp_label
[4] w3c/accname#233

Bug: 338348441
Change-Id: Iffb623023e5ee3b903f2c243ae985efa5179135e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5540421
Reviewed-by: Aaron Leventhal <[email protected]>
Reviewed-by: Benjamin Beaudry <[email protected]>
Commit-Queue: Aaron Leventhal <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1302913}
@MelSumner
Copy link
Contributor

If adding a step to strip the whitespace from the accumulated text so that it will correctly fall back to the aria-label, then we should add it.

Do we think this is the case?

@aleventhal
Copy link
Contributor

aleventhal commented Jul 11, 2024 via email

@aleventhal
Copy link
Contributor

aleventhal commented Jul 11, 2024 via email

@MelSumner
Copy link
Contributor

I wonder if, because the whitespace is the result of formatted code (linebreaks and tabs/spaces), that kind of whitespace should be removed.

Formatting whitespace (is there an existing formal/more correct name?) shouldn't add semantic meaning to anything.

(FWIW, I've run into issues before just with HTML where code-formatting whitespace issues can make things not render, or not render correctly, in the browser, and it's super frustrating).

Also, aria-label already references trimming whitespace (to respond to @aleventhal comment):

AriaLabel: Otherwise, if the current node has an aria-label attribute whose value is not undefined, not the empty string, nor, when trimmed of whitespace, is not the empty string:

@aleventhal
Copy link
Contributor

IMO Chromium is correct and should not be failing a test here.

The browser engine is what decides which whitespace from the source should be rendered. The a11y implementation faithfully represents what's rendered.

You can see this if you change the button to a <div style="display:inline-block" contenteditable> (the button is inline-block by default in the user agent style sheet). Because a space shows up there, it's clear that representing the space is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs triage
Development

No branches or pull requests

4 participants