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

Expectations for aria-hidden and focused elements #2181

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

scottaohara
Copy link
Member

@scottaohara scottaohara commented May 20, 2024

This PR closes #1765 and is related to work that was done in #2037, but scoped only to the original issue I filed.

The intent of this PR is to identify not only how user agents would need to handle focusable elements that are aria-hidden (explicitly or due to being a descendant of an aria-hidden container) - but for the case where a focusable element is within an aria-hidden container, that the entire subtree would need to be re-exposed so that any other relevant information to the user could be made available. (e.g., so as to not just expose a "learn more" link, with no way to determine what someone would be learning about)

a simple example being like:

<div aria-hidden=true>
  <h3>Something or other</h3>
   some details about said something, or other.
  <a href=#>Learn more!</a>
</div>
  • Related Core AAM Issue/PR:
  • Related AccName Issue/PR:
  • Any other dependent changes?

Test, Documentation and Implementation tracking

Once this PR and all related PRs have been approved by the working group, tests
should be written and issues should be opened on browsers. Add N/A and check when not
applicable.

  • Related APG Issue/PR:
  • MDN Issue/PR:
  • "author MUST" tests:
  • "user agent MUST" tests:
  • Browser implementations (link to issue or when done, link to commit):
    • WebKit:
    • Gecko:
    • Blink:
  • Does this need AT implementations?

Preview | Diff

This PR closes #1765 and is related to work that was done in #2037, but scoped only to the original issue I filed.

The intent of this PR is to identify not only how user agents would need to handle focusable elements that are aria-hidden (explicitly or due to being a descendant of an aria-hidden container) - but for the case where a focusable element is within an aria-hidden container, that the entire subtree would need to be re-exposed so that any other relevant information to the user could be made available.  (e.g., so as to not just expose a "learn more" link, with no way to determine what someone would be learning about)

a simple example being like:

```
<div aria-hidden=true>
  <h3>Something or other</h3>
   some details about said something, or other.
  <a href=#>Learn more!</a>
</div>
```
index.html Outdated Show resolved Hide resolved
@aleventhal
Copy link
Contributor

aleventhal commented May 20, 2024 via email

@rahimabdi
Copy link
Contributor

This may be WPT-testable; should I create a separate issue if you'd like tests for this (the MUST around the subtree being re-exposed)?

@scottaohara
Copy link
Member Author

talking with @smhigley about this... but should this clarify that if an element is already focused and its container becomes aria-hidden, then the aria-hidden state should remain unless focus is then moved to another element within that hidden subtree.

for instance, think of a custom modal dialog that was built where the order of events are:

  • button invokes dialog (focus is on button)
  • element containing button set to aria-hidden=true
  • dialog is rendered (outside aria-hidden=true container)
  • focus moves to dialog

since focus moving is the last thing that occurs, we do not want browsers to immediately undo the aria-hidden that was dynamically added (though focus is still on the button inside the aria-hidden container)

@spectranaut spectranaut requested a review from adampage May 23, 2024 17:07
@jnurthen jnurthen self-requested a review May 23, 2024 17:08
aarongable pushed a commit to chromium/chromium that referenced this pull request May 28, 2024
Using aria-hidden on content that is actually visible can cause problems
if a node inside receives focus. In the past, nothing was announced,
and the fix was to expose all focusable nodes inside of an aria-hidden,
but with the invisible state. This gave the AT a chance to decide to
make the announcement. However, this fix did not work with VoiceOver
or NVDA, and only partially worked with JAWS.

The new approach is to assume the aria-hidden is correct and to
not expose anything in the subtree, unless the user somehow is able
to focus something in subtree. In that case, it is assumed to be
an authoring error and misuse of aria-hidden. As such, browsers will
now consider the bad aria-hidden on the containing element
to be invalid and ignore it from that point forward, causing the
entire subtree to now be exposed as visible. See this PR for
the ARIA spec: w3c/aria#2181.

This also helps enable a downstream CL that removes 125 lines.

Fixed: 40833533
Change-Id: Ib0d7eed10fada5ae7799c5af1257e5a66fb0c034
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5544894
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1306971}
@scottaohara
Copy link
Member Author

need to add to this that user agents need to make this repair if the element focused is in the same document - but say if the element that is focused is within an iframe, but the iframe is hidden from an aria-hidden surrounding it in the parent doc, then browsers may not be able to reasonably make this repair.

@smhigley
Copy link
Contributor

I have one other question, but mostly for browser implementors; I'm not sure if this amount of detail can be in the spec --

Would the removal of aria-hidden occur after the focus event on an element, or after document.activeElement is set to point to a hidden element? I'm making the distinction because in a lot of our focus management logic for things like dialogs, we look for a focus event on a hidden element (either a focus bumper, or looking for focus moving to the background page), and immediately send focus back into the dialog in the event handler. The result is that document.activeElement never points to a hidden element, but the focus event does fire. If the change happened as a result of the event, it would break quite a lot of things at least in our library, and IIRC in others as well 😅.

If that's not the case, and if the scenario that @scottaohara already mentioned is also handled, I think I wouldn't have any more concerns about this.

@aleventhal
Copy link
Contributor

aleventhal commented May 28, 2024 via email

@smhigley
Copy link
Contributor

smhigley commented May 29, 2024

@aleventhal I think I may not have communicated my use case well enough 😅.

The distinction between doing a .removeAttribute() and ignoring the attribute through some other method wasn't really material to the question I had in mind, though bringing it up does make me worry that this may make it difficult for developers to debug when an element hidden in markup is exposed.

For my original question, I made this JS Fiddle to help: https://jsfiddle.net/sjc89htb/. It uses logic similar to what we use at Microsoft, and what I've seen for focus traps elsewhere, where focus is immediately sent elsewhere when it lands on a hidden element. In practice this works perfectly in all ATs I've tested up till now. Would you be able to clarify whether it will continue working with this change, or whether the change will result in the hidden content being exposed?

Thanks for taking the time to investigate and weigh in!

index.html Outdated Show resolved Hide resolved
@aleventhal
Copy link
Contributor

@aleventhal I think I may not have communicated my use case well enough 😅.

Thanks for explaining... good chance the issue was on the receiver's end :)

The distinction between doing a .removeAttribute() and ignoring the attribute through some other method wasn't really material to the question I had in mind, though bringing it up does make me worry that this may make it difficult for developers to debug when an element hidden in markup is exposed.

We could log something like an error to the JS console, warning the developer that bad aria-hidden markup had been discarded. I thought we had similar guidance for other user agent repairs, but I can't find that in the spec after a quick search.

For my original question, I made this JS Fiddle to help: https://jsfiddle.net/sjc89htb/. It uses logic similar to what we use at Microsoft, and what I've seen for focus traps elsewhere, where focus is immediately sent elsewhere when it lands on a hidden element. In practice this works perfectly in all ATs I've tested up till now. Would you be able to clarify whether it will continue working with this change, or whether the change will result in the hidden content being exposed?

It seems to work! Our proposed change is currently implemented in Chrome Canary from this morning, version 127.0.6508.0. I ran with and without the focus trap. In the version without the focus trap, once you tab into the button, the button is revealed in the a11y tree (I used the devtools Inspector full page a11y view for the test). In the version with the focus trap, focus never goes to the button, and the aria-hidden is never discarded. I think this is an important test that we should add to WPT tests. @scottaohara maybe we can update the spec text to say if focus is not forwarded somewhere else? Perhaps it should mentioned as a valuable technique, although maybe authors should be using inert here.

Anyway, go ahead and play with Canary if you like. I'll wait to hear back what people think of a console message to developers. Maybe something like, console.error("The following element had aria-hidden markup that was discarded, because focus went to it or an element within it. See WAI-ARIA rules on aria-hidden:", bad_aria_hidden_element);

@smhigley
Copy link
Contributor

smhigley commented May 29, 2024

@aleventhal awesome! I love both the idea of putting that scenario in WPT, and the idea of logging a warning to the console.

I'm all in on this now, thanks again for investigating!

@aleventhal
Copy link
Contributor

aleventhal commented May 30, 2024

Landing a patch to Chromium that adds this console error:

  element.AddConsoleMessage(
      mojom::blink::ConsoleMessageSource::kRendering,
      mojom::blink::ConsoleMessageLevel::kError,
      String::Format(
          "Blocked aria-hidden on a <%s> element because the element that just "
          "received focus must not be hidden from assistive technology users. "
          "Avoid using aria-hidden on a focused element or its ancestor. "
          "Consider using the inert attribute instead, which will also prevent "
          "focus. For more details, see the aria-hidden section of the "
          "WAI-ARIA specification at https://w3c.github.io/aria/#aria-hidden.",
          element.TagQName().ToString().Ascii().c_str()));

Should we add text to this PR saying that user agents may provide a helpful console error message?
Also, can we update this PR to say how developers can fix the issue? (Either by forwarding focus via focusin, by not putting aria-hidden on focusable objects, by using inert etc.)

@pkra pkra added the spec:aria label Jun 14, 2024
index.html Outdated Show resolved Hide resolved
pulling in aaron's suggested edits
Copy link

netlify bot commented Aug 7, 2024

Deploy Preview for wai-aria ready!

Name Link
🔨 Latest commit 8d575af
🔍 Latest deploy log https://app.netlify.com/sites/wai-aria/deploys/66e1b3613ff2c90008bcff3f
😎 Deploy Preview https://deploy-preview-2181--wai-aria.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

adds in a reference to the tree exclusion section, and updates bits of content here to include reference to html's `inert` attribute, and to try and acknowledge the change to aria-hidden=false by also referencing aria-hidden=undefined.
@scottaohara
Copy link
Member Author

made updates per the provided feedback, so re-requesting reviews.

index.html Outdated Show resolved Hide resolved
@aleventhal
Copy link
Contributor

Landing a patch to Chromium that adds this console error:

  element.AddConsoleMessage(
      mojom::blink::ConsoleMessageSource::kRendering,
      mojom::blink::ConsoleMessageLevel::kError,
      String::Format(
          "Blocked aria-hidden on a <%s> element because the element that just "
          "received focus must not be hidden from assistive technology users. "
          "Avoid using aria-hidden on a focused element or its ancestor. "
          "Consider using the inert attribute instead, which will also prevent "
          "focus. For more details, see the aria-hidden section of the "
          "WAI-ARIA specification at https://w3c.github.io/aria/#aria-hidden.",
          element.TagQName().ToString().Ascii().c_str()));

Should we add text to this PR saying that user agents may provide a helpful console error message? Also, can we update this PR to say how developers can fix the issue? (Either by forwarding focus via focusin, by not putting aria-hidden on focusable objects, by using inert etc.)

@scottaohara

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider further calling out aria-hidden expectations for focusable elements
7 participants