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

Change aria-hidden=false to synonym of undefined #2090

Merged
merged 5 commits into from
May 2, 2024
Merged

Conversation

scottaohara
Copy link
Member

@scottaohara scottaohara commented Dec 14, 2023

resolves #1256

This PR modifies the previous note about aria-hidden=false to instead indicate that as of ARIA 1.3 is serves as a synonym for the 'undefined' value.

It also includes a minor editorial change to remove the mention of 'screen readers' from the last paragraph of the normative text. It seemed to state, especially when the sentence ends with referring to 'assistive technologies' in general.

See last comment

Question: assuming this PR is approved, would we also want to add the following sentence to the note?

In future versions of ARIA, the undefined value will be deprecated in favor of the false value.

PR tracking

Check these when the relevant issue or PR has been made, OR after you have confirmed the
related change is not necessary (add N/A). Leave unchecked if you are unsure. Read the
Process Document or
Test Overview for more information.

  • Related Core AAM Issue/PR: N/A
  • Related AccName Issue/PR: N/A
  • Related APG Issue/PR: N/A
  • Any other dependent changes? N/A
    • WPT tests (multiple implementations passing before merge)

Implementation tracking


Preview | Diff

resolves #1256

This PR modifies the previous note about aria-hidden=false to instead indicate that as of ARIA 1.3 is serves as a synonym for the 'undefined' value.

It also includes a minor editorial change to remove the mention of 'screen readers' from the last paragraph of the normative text.  It seemed to state, especially when the sentence ends with referring to 'assistive technologies' in general.

See [last comment](#1256 (comment))

Chrome issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1500299

Question: assuming this PR is approved, would we also want to add the following sentence to the note?
>In future versions of ARIA, the undefined value will be deprecated in favor of the false value.
index.html Outdated Show resolved Hide resolved
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.

@aleventhal approved this PR, but he was also proposing that aria-hidden=false be allowed on visibly rendered items, a partial equivalent to the inert attribute. What changed, Aaron?

If there's a possibility we'll ever want to bring this value back, we should not change it now, and instead wait to change it once WICG/aom#203 (or some other testing approach) gives us access to inspect whether the node is "hidden from accessibility"

@cookiecrook
Copy link
Contributor

cookiecrook commented Jan 5, 2024

@jnurthen there were other related issues in the ARIA-hidden project that have been moved elsewhere. Should a keyword be added instead? Is there an easy way to find those issues? I'm not sure why they were put in a 1.3/1.4 Projects when we already had 1.3/1.4 Milestones. Apologies if that explanation was covered in one of the meetings I missed.

@aleventhal
Copy link
Contributor

@aleventhal approved this PR, but he was also proposing that aria-hidden=false be allowed on visibly rendered items, a partial equivalent to the inert attribute. What changed, Aaron?

I wanted aria-hidden="false" to be useful, but unfortunately, as was pointed out to me, it's misused by a lot of JS toolkits which treat it as a boolean, and they end up setting "false" all over the place where they really should have removed the attribute.

If there's a possibility we'll ever want to bring this value back, we should not change it now, and instead wait to change it once WICG/aom#203 (or some other testing approach) gives us access to inspect whether the node is "hidden from accessibility"

Because of the problem in js toolkits, and because browsers all implement it differently, I think we should go ahead and deprecate it as a possible value.

@cookiecrook
Copy link
Contributor

Because of the problem in js toolkits, and because browsers all implement it differently, I think we should go ahead and deprecate [aria-hidden=false] as a possible value.

Okay. I'm going to ask @twilco and @minorninth to review as well in case there are implementation concerns with its removal.

I'm hopeful it goes without saying that this should not be merged until we have extensively passing WPT tests. Since this one wasn't in the initial Interop 2024 Focus Area proposal, that may not happen this year, but we can treat it as a tentative area for inclusion if there is time to fix it before the Fall releases.

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.

Updating my "Changes Requested" review to "Comment" tentatively pending review from others.

@cookiecrook cookiecrook self-requested a review January 5, 2024 20:56
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.

Second time's the charm?

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

I support removing the semantics of aria-hidden=false, but I'd like to improve the wording. I left some comments. Thanks.

Co-authored-by: James Craig <cookiecrook@users.noreply.github.com>
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@jnurthen jnurthen merged commit 6205740 into main May 2, 2024
5 checks passed
github-actions bot added a commit that referenced this pull request May 2, 2024
SHA: 6205740
Reason: push, by jnurthen

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
<p class="note">Authors are advised to use extreme caution and consider a wide range of disabilities when hiding visibly rendered content from assistive technologies. For example, a sighted, dexterity-impaired individual might use voice-controlled assistive technologies to access a visual interface. If an author hides visible link text "Go to checkout" and exposes similar, yet non-identical link text "Check out now" to the accessibility API, the user might be unable to access the interface they perceive using voice control. Similar problems can also arise for screen reader users. For example, a sighted telephone support technician might attempt to have the blind screen reader user click the "Go to checkout" link, which they might be unable to find using a type-ahead item search ("Go to…").</p>
<p class="note">At the time of this writing, <code><sref>aria-hidden</sref>="false"</code> is known to work inconsistently in browsers. As future implementations improve, use caution and test thoroughly before relying on this approach.</p>
<p class="note">As of ARIA 1.3, <code><sref>aria-hidden</sref>="false"</code> is now synonymous with <code>aria=hidden="undefined"</code>.</p>
Copy link

Choose a reason for hiding this comment

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

nit: aria-hidden*

hoffmanjoshua pushed a commit to hoffmanjoshua/WebKit that referenced this pull request Aug 12, 2024
https://bugs.webkit.org/show_bug.cgi?id=277958
rdar://problem/133693990

Reviewed by NOBODY (OOPS!).

Per spec changes (see w3c/aria#2090), we should treat `aria-hidden=false` as undefined.

This patch removes support for aria-hidden=false, updating places where we relied on the behaviors of isNodeARIAVisible
with the new behavior. For example, we need to check if a child node is focused before deciding whether to skip it if it is
aria-hidden (tested by accessibility/datetime/input-date-field-labels-and-value-changes.html).

Tests that explicitly validate aria-hidden false were removed, and a new test to check that we are ignoring this property
has been added.

* LayoutTests/accessibility/aria-hidden-false-ignored-expected.txt: Added.
* LayoutTests/accessibility/aria-hidden-false-ignored.html: Added.
* LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html: Removed.
* LayoutTests/accessibility/aria-hidden-negates-no-visibility.html: Removed.
* LayoutTests/accessibility/aria-modal-expected.txt:
* LayoutTests/accessibility/aria-modal.html:
* LayoutTests/accessibility/aria-visible-element-roles.html: Removed.
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::isNodeVisible const):
(WebCore::AXObjectCache::getOrCreate):
(WebCore::isNodeFocused):
(WebCore::isNodeAriaVisible): Deleted.
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::textUnderElement const):
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::defaultObjectInclusion const):
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::addNodeOnlyChildren):
hoffmanjoshua pushed a commit to hoffmanjoshua/WebKit that referenced this pull request Aug 12, 2024
https://bugs.webkit.org/show_bug.cgi?id=277958
rdar://problem/133693990

Reviewed by NOBODY (OOPS!).

Per spec changes (see w3c/aria#2090), we should treat `aria-hidden=false` as undefined.

This patch removes support for aria-hidden=false, updating places where we relied on the behaviors of isNodeARIAVisible
with the new behavior. For example, we need to check if a child node is focused before deciding whether to skip it if it is
aria-hidden (tested by accessibility/datetime/input-date-field-labels-and-value-changes.html).

Tests that explicitly validate aria-hidden false were removed, and a new test to check that we are ignoring this property
has been added.

* LayoutTests/accessibility/aria-hidden-false-ignored-expected.txt: Added.
* LayoutTests/accessibility/aria-hidden-false-ignored.html: Added.
* LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html: Removed.
* LayoutTests/accessibility/aria-hidden-negates-no-visibility.html: Removed.
* LayoutTests/accessibility/aria-modal-expected.txt:
* LayoutTests/accessibility/aria-modal.html:
* LayoutTests/accessibility/aria-visible-element-roles.html: Removed.
* LayoutTests/accessibility/datetime/input-date-field-labels-and-value-changes.html:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::isNodeVisible const):
(WebCore::AXObjectCache::getOrCreate):
(WebCore::isNodeFocused):
(WebCore::isNodeAriaVisible): Deleted.
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::textUnderElement const):
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::defaultObjectInclusion const):
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::addNodeOnlyChildren):
hoffmanjoshua pushed a commit to hoffmanjoshua/WebKit that referenced this pull request Aug 15, 2024
https://bugs.webkit.org/show_bug.cgi?id=277958
rdar://problem/133693990

Reviewed by NOBODY (OOPS!).

Per spec changes (see w3c/aria#2090), we should treat `aria-hidden=false` as undefined.

This patch removes support for aria-hidden=false, updating places where we relied on the behaviors of isNodeARIAVisible
with the new behavior. For example, we need to check if a child node is focused before deciding whether to skip it if it is
aria-hidden (tested by accessibility/datetime/input-date-field-labels-and-value-changes.html).

Tests that explicitly validate aria-hidden false were removed, and a new test to check that we are ignoring this property
has been added.

* LayoutTests/accessibility/aria-hidden-false-ignored-expected.txt: Added.
* LayoutTests/accessibility/aria-hidden-false-ignored.html: Added.
* LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html: Removed.
* LayoutTests/accessibility/aria-hidden-negates-no-visibility.html: Removed.
* LayoutTests/accessibility/aria-hidden-subtree-expected.txt: Added.
* LayoutTests/accessibility/aria-hidden-subtree.html: Added.
* LayoutTests/accessibility/aria-modal-expected.txt:
* LayoutTests/accessibility/aria-modal.html:
* LayoutTests/accessibility/aria-visible-element-roles.html: Removed.
* LayoutTests/accessibility/datetime/input-date-field-labels-and-value-changes.html:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::isNodeVisible const):
(WebCore::AXObjectCache::getOrCreate):
(WebCore::isNodeFocused):
(WebCore::isNodeAriaVisible): Deleted.
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::textUnderElement const):
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::defaultObjectInclusion const):
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::addNodeOnlyChildren):
hoffmanjoshua pushed a commit to hoffmanjoshua/WebKit that referenced this pull request Aug 15, 2024
https://bugs.webkit.org/show_bug.cgi?id=267150
rdar://120557669

Reviewed by NOBODY (OOPS!).

Per spec changes (see w3c/aria#2090), we should treat `aria-hidden=false` as undefined.

This patch removes support for aria-hidden=false, updating places where we relied on the behaviors of isNodeARIAVisible
with the new behavior. For example, we need to check if a child node is focused before deciding whether to skip it if it is
aria-hidden (tested by accessibility/datetime/input-date-field-labels-and-value-changes.html).

Tests that explicitly validate aria-hidden false were removed, and a new test to check that we are ignoring this property
has been added.

* LayoutTests/accessibility/aria-hidden-false-ignored-expected.txt: Added.
* LayoutTests/accessibility/aria-hidden-false-ignored.html: Added.
* LayoutTests/accessibility/aria-hidden-false-works-in-subtrees.html: Removed.
* LayoutTests/accessibility/aria-hidden-negates-no-visibility.html: Removed.
* LayoutTests/accessibility/aria-hidden-subtree-expected.txt: Added.
* LayoutTests/accessibility/aria-hidden-subtree.html: Added.
* LayoutTests/accessibility/aria-modal-expected.txt:
* LayoutTests/accessibility/aria-modal.html:
* LayoutTests/accessibility/aria-visible-element-roles.html: Removed.
* LayoutTests/accessibility/datetime/input-date-field-labels-and-value-changes.html:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::isNodeVisible const):
(WebCore::AXObjectCache::getOrCreate):
(WebCore::isNodeFocused):
(WebCore::isNodeAriaVisible): Deleted.
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::textUnderElement const):
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::defaultObjectInclusion const):
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::addNodeOnlyChildren):
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

Successfully merging this pull request may close these issues.

Consider allowing aria-hidden=false on visibly rendered elements inside an ancestor with aria-hidden=true
7 participants