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

AX: align AccessibilityRole::Switch with ::Checkbox #18938

Conversation

annevk
Copy link
Contributor

@annevk annevk commented Oct 11, 2023

5ce55c6

AX: align AccessibilityRole::Switch with ::Checkbox
https://bugs.webkit.org/show_bug.cgi?id=263008
rdar://116805287

Reviewed by Tyler Wilcock.

A switch should essentially be a checkbox except that it doesn't
support the intermediate/mixed state.

As such, this makes the following changes:

- We make switch support the required attribute.
- We make interaction regions support switches.

* LayoutTests/accessibility/aria-required-expected.txt:
* LayoutTests/accessibility/aria-required.html:
* LayoutTests/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:
* LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html:
* LayoutTests/interaction-region/aria-roles-expected.txt:
* LayoutTests/interaction-region/aria-roles.html:
* LayoutTests/platform/gtk/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::supportsRequiredAttribute const):
* Source/WebCore/page/InteractionRegion.cpp:
(WebCore::shouldAllowAccessibilityRoleAsPointerCursorReplacement):

Canonical link: https://commits.webkit.org/269311@main

179d374

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@annevk annevk self-assigned this Oct 11, 2023
@annevk annevk added the Accessibility For bugs related to accessibility. label Oct 11, 2023
@webkit-early-warning-system

This comment was marked as outdated.

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.

Will you also confirm computedrole with a new subtest in WPT?
https://github.com/web-platform-tests/wpt/blob/master/html-aam/roles.html

@cookiecrook
Copy link
Contributor

Oh the tests PR is already in. Commented there.

Comment on lines 272 to 273
if (isCheckboxOrRadio() || isSwitch())
return checkboxOrRadioRect();
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose checkboxOrRadioRect() is to unify the rect of the control with the rect of its associated label. But right now this only works with native <label for="id-of-control">Foo label</label>, or containing labels:

<label>
   Foo bar label for switch
   <div role="switch"></div>
</label>

But I'm wondering if either of these methods of pairing labels are "allowed" or recommended patterns for a role="switch"? Certainly for <input type=checkbox switch>, but role="switch" I'm less confident in.

@cookiecrook what do you think?

Copy link
Contributor Author

@annevk annevk Oct 12, 2023

Choose a reason for hiding this comment

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

Hmm, but the logic here also does this for <label><span role=checkbox></span></label>, right? Perhaps it shouldn't be using isCheckboxOrRadio() to begin with?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you're right, it would behave that way for role=checkbox, but arguably shouldn't. Let's remove this part and we'll address it separately later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes label for/id should only work on native inputs, not ARIA controls.

@annevk
Copy link
Contributor Author

annevk commented Oct 12, 2023

@cookiecrook this PR is mainly to align role=switch with role=checkbox. <input type=checkbox switch> is a bigger change that will take several PRs tracked as dependencies of https://bugs.webkit.org/show_bug.cgi?id=259380. And yeah, I'll make sure to add a computed role test there.

@annevk
Copy link
Contributor Author

annevk commented Oct 12, 2023

Update here:

  1. It seems that checkboxOrRadioRect() should only apply to genuine <input> checkboxes and radio buttons. This is probably best tackled separately as it no longer is about aligning role=switch with role=checkbox.
  2. I'd still appreciate some guidance with respect to testing the "required" attribute (aria-required presumably?) and shouldAllowAccessibilityRoleAsPointerCursorReplacement(). I didn't find any observable differences of these changes myself, which makes that a bit tricky.

@twilco
Copy link
Contributor

twilco commented Oct 12, 2023

  1. I'd still appreciate some guidance with respect to testing the "required" attribute (aria-required presumably?) and shouldAllowAccessibilityRoleAsPointerCursorReplacement(). I didn't find any observable differences of these changes myself, which makes that a bit tricky.

It would be great to add a testcase to LayoutTests/accessibility/aria-required.html. It's interesting that there are no VoiceOver behavior differences. supportsRequiredAttribute() is used by VoiceOver to determine whether it should bother asking for the required state of the element in future requests. But that doesn't seem to be the case here. Regardless, I'd still make your change as-is.

One place there will be a behavior difference is in inspector. Please add a role="switch" aria-required="true" testcase to inspector/dom/getAccessibilityPropertiesForNode.html. Without your patch, inspector will not report the required status.

@annevk annevk force-pushed the eng/Align-AccessibilityRoleSwitch-with-CheckBox branch from 8b8ad41 to c0555f0 Compare October 13, 2023 15:00
@webkit-early-warning-system

This comment was marked as outdated.

@annevk
Copy link
Contributor Author

annevk commented Oct 13, 2023

@twilco the changes I made to accessibility/aria-required.html pass with or without this change. inspector/dom/getAccessibilityPropertiesForNode.html is impacted by the supportsRequiredAttribute() change.

And interaction regions is a visionOS-specific fix it seems. @etiennesegonzac helpfully provided the test expectations there (that EWS will ignore, but hopefully that's good enough for everyone here to get r+).

@annevk annevk marked this pull request as ready for review October 13, 2023 15:30
@annevk annevk force-pushed the eng/Align-AccessibilityRoleSwitch-with-CheckBox branch from c0555f0 to c4a86cb Compare October 13, 2023 15:34
@webkit-early-warning-system

This comment was marked as outdated.

@annevk annevk added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Oct 13, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 13, 2023
@annevk annevk removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks merging-blocked Applied to prevent a change from being merged labels Oct 13, 2023
@annevk annevk changed the title Align AccessibilityRole::Switch with ::CheckBox AX: align AccessibilityRole::Switch with ::Checkbox Oct 13, 2023
@annevk annevk force-pushed the eng/Align-AccessibilityRoleSwitch-with-CheckBox branch from c4a86cb to 179d374 Compare October 13, 2023 17:15
@annevk annevk added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Oct 13, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Oct 13, 2023
https://bugs.webkit.org/show_bug.cgi?id=263008
rdar://116805287

Reviewed by Tyler Wilcock.

A switch should essentially be a checkbox except that it doesn't
support the intermediate/mixed state.

As such, this makes the following changes:

- We make switch support the required attribute.
- We make interaction regions support switches.

* LayoutTests/accessibility/aria-required-expected.txt:
* LayoutTests/accessibility/aria-required.html:
* LayoutTests/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:
* LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html:
* LayoutTests/interaction-region/aria-roles-expected.txt:
* LayoutTests/interaction-region/aria-roles.html:
* LayoutTests/platform/gtk/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::supportsRequiredAttribute const):
* Source/WebCore/page/InteractionRegion.cpp:
(WebCore::shouldAllowAccessibilityRoleAsPointerCursorReplacement):

Canonical link: https://commits.webkit.org/269311@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Align-AccessibilityRoleSwitch-with-CheckBox branch from 179d374 to 5ce55c6 Compare October 13, 2023 18:33
@webkit-commit-queue
Copy link
Collaborator

Committed 269311@main (5ce55c6): https://commits.webkit.org/269311@main

Reviewed commits have been landed. Closing PR #18938 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 13, 2023
@webkit-commit-queue webkit-commit-queue merged commit 5ce55c6 into WebKit:main Oct 13, 2023
@annevk annevk deleted the eng/Align-AccessibilityRoleSwitch-with-CheckBox branch October 13, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility For bugs related to accessibility.
Projects
None yet
6 participants