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

prevent use of aria-hidden=true on document root elements #1880

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

Conversation

scottaohara
Copy link
Member

@scottaohara scottaohara commented Mar 3, 2023

closes #1254

adds user agent and author requirements regarding the use of aria-hidden=true on root-level elements of a document.

@cookiecrook I took a first stab at drafting up the author and UA must nots. Please revise however you see fit.

  • Related Core AAM Issue/PR:
  • Related AccName Issue/PR:
  • Related APG Issue/PR:
  • ARIA in HTML 447

Implementation tracking

  • validator
  • WPT tests: LINK
  • Browser implementations (link to issue or when done, link to commit):
  • Does this need AT implementations?

Preview | Diff

closes #1254

@cookiecrook I took a first stab at drafting up the author and UA must nots.  Please revise however you see fit.
Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start. Raising some questions that I don't currently know the answer to. Thanks for taking this one on!

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link

@jcsteh jcsteh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

Does this conflict with #1256? That proposal has its own set of challenges, but as I understand it, it could be used to emulate dialog.showModal(). That is, an author might set aria-hidden=true on the root and then set aria-hidden=false on a (non-native) modal dialog.

I tend to think the risk of abuse with aria-hidden on the root is higher than the advantages of supporting the theoretical emulation of dialog.showModal(), but I know there's been some push to have parallel support in ARIA for native HTML features, which is why I'm raising it here.

scottaohara and others added 2 commits March 24, 2023 13:52
Co-authored-by: James Craig <cookiecrook@users.noreply.github.com>
Co-authored-by: James Craig <cookiecrook@users.noreply.github.com>
@scottaohara
Copy link
Member Author

thanks a ton for those edits, @cookiecrook

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@pkra pkra added this to the ARIA 1.3 milestone Jun 28, 2023
index.html Outdated Show resolved Hide resolved
@jnurthen jnurthen added Agenda and removed Agenda labels Nov 7, 2023
scottaohara added a commit to w3c/html-aam that referenced this pull request Nov 15, 2023
@spectranaut spectranaut added the waiting for implementations add to traking issue when PR is merged into main, but can't be merged into stable label Feb 22, 2024
JonWBedard pushed a commit to JonWBedard/WebKit that referenced this pull request Apr 15, 2024
https://bugs.webkit.org/show_bug.cgi?id=269523
rdar://123049663

Reviewed by Chris Fleizach.

To prevent authors from hiding all content from assistive technologies, aria-hidden should not be respected on document
root elements (`html`, `body`, or top-level `svg`s).

w3c/aria#1880

* LayoutTests/accessibility/body-element-aria-hidden-expected.txt: Added.
* LayoutTests/accessibility/body-element-aria-hidden.html: Added.
* LayoutTests/accessibility/html-element-aria-hidden-expected.txt: Added.
* LayoutTests/accessibility/html-element-aria-hidden.html: Added.
* LayoutTests/platform/ios/TestExpectations: Enable new tests.
* Source/WebCore/accessibility/AXCoreObject.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::isARIAHidden const):
(WebCore::AccessibilityObject::tagName const):
(WebCore::AccessibilityObject::setIsIgnoredFromParentDataForChild):
* Source/WebCore/accessibility/AccessibilityObject.h:
* Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp:
(WebCore::AccessibilityObjectAtspi::attributes const):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::tagName const): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Apr 18, 2024
https://bugs.webkit.org/show_bug.cgi?id=269523
rdar://123049663

Reviewed by Chris Fleizach.

To prevent authors from hiding all content from assistive technologies, aria-hidden should not be respected on document
root elements (`html`, `body`, or top-level `svg`s).

w3c/aria#1880

* LayoutTests/accessibility/body-element-aria-hidden-expected.txt: Added.
* LayoutTests/accessibility/body-element-aria-hidden.html: Added.
* LayoutTests/accessibility/html-element-aria-hidden-expected.txt: Added.
* LayoutTests/accessibility/html-element-aria-hidden.html: Added.
* LayoutTests/platform/ios/TestExpectations: Enable new tests.
* Source/WebCore/accessibility/AXCoreObject.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::isARIAHidden const):
(WebCore::AccessibilityObject::tagName const):
(WebCore::AccessibilityObject::setIsIgnoredFromParentDataForChild):
* Source/WebCore/accessibility/AccessibilityObject.h:
* Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp:
(WebCore::AccessibilityObjectAtspi::attributes const):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::tagName const): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:

Canonical link: https://commits.webkit.org/277677@main
Copy link
Contributor

@aleventhal aleventhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully it is clear to readers what a primary document is. Do we need to define that in the glossary or is it really obvious to the intended audience?

Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottaohara, in an earlier comment you said the embedded view (iframe, img, etc) could be hidden by the containing context, so on a per-document level, this rule need not be aware of its embeddedness, or whether it's the "primary" document.

the root element or the host language element that represents the contents of the primary document in view.

I agree with you, so it's possible this ~"root element or the host language element representing contents" is sufficient without the undefined "primary document" term.

@aleventhal
Copy link
Contributor

What if the parent document doesn't have as much context as the iframe's contents about whether it should be hidden or not? They might not even be from the same domain. Is it difficult for any of the implementations to determine whether the document is top level?

Copy link
Contributor

@aleventhal aleventhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing my +1 until we get consensus on whether the top level document's documentElement/body is treated differently from the iframe's.

@aleventhal aleventhal self-requested a review April 26, 2024 14:32
@cookiecrook
Copy link
Contributor

Removing my +1 until we get consensus on whether the top level document's documentElement/body is treated differently from the iframe's.

If that's a deal-breaker for Aaron, I'd say let's keep "primary." It does allow more permissive authoring flexibility, albeit adding a bit more implementation complexity.

@aleventhal
Copy link
Contributor

I think it just adds a single condition to the 'if' statement. Not too complex IMO. I'd rather not deal with the bug that gets filed with a legit use case. But perhaps Scott has a good reason to disallow in iframe html/body elements.

@scottaohara
Copy link
Member Author

Allowing an html/body element to have aria-hidden=true on it in an iframe wouldn’t do anything to prevent the exposure of the iframe in the parent doc

However is the iframe in the parent doc could be consistently set to be hidden / taken out of the tab order, then that could prevent the nested iframe content from being accessible

I fail to understand why there should be any distinction whether aria-hidden is set to the html/body elements within an iframe or not, since it’d be unnecessary if the iframe in the parent doc was properly hidden, and if the iframe was not properly hidden, then respecting the aria-hidden attribute in that context would result in the problem we are trying to solve with this change

@spectranaut
Copy link
Contributor

Adding agenda to "get consensus" re: Aaron's concerns so we can move this one forward!

@spectranaut
Copy link
Contributor

discussed in https://www.w3.org/2024/05/09-aria-minutes.html#t10

Decision to move forward

@aleventhal
Copy link
Contributor

aleventhal commented May 10, 2024

Ok, let's go with disallowing aria-hidden on <body> and any document element <html>, <svg>.

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

@aleventhal aleventhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. I can't find anything that would be confusing, vague or isn't a clear improvement.

@jnurthen jnurthen removed the Agenda label May 21, 2024
pkra pushed a commit that referenced this pull request May 23, 2024
@pkra pkra added the spec:aria label Jun 14, 2024
@aleventhal
Copy link
Contributor

In terms of "waiting for implementations". Chrome has implemented this.

@pkra
Copy link
Member

pkra commented Jul 10, 2024

@scottaohara the WebKit issue also seems to have been resolved, so this might be ready to merge.

@scottaohara
Copy link
Member Author

related HTML AAM PR has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:aria waiting for implementations add to traking issue when PR is merged into main, but can't be merged into stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aria-hidden error case: disallow aria-hidden on root or document element (e.g. <html> or <body>)
7 participants