[a11y] Fix accessibility click action on combo boxes [chromium/src : main]
Aaron Moss (Gerrit)
Aaron Moss voted and added 6 comments![]( "Open in Gerrit")
Votes added by Aaron Moss
| Commit-Queue | +1 |
6 comments
The test change I included here should fix the only failures from the dry-run on PS 4, but if you'd like to wait for the dry run on this PS to finish you're welcome to.
I did a bit of an odd thing here where the fix was to change the behaviour of `TextFieldInputType::AccessKeyAction()`, but I combined that with a refactor that unified (now) 4 of 6 identical subclass overrides into the base class behaviour in `InputTypeView`; if you'd like me to split the cleanup refactor into a separate CL I can.
void AccessKeyAction(SimulatedClickCreationScope creation_scope) final;The key issue for this bug was that the accessibility click path called `AccessKeyAction()` on its underlying element (reusing the `accesskey` keyboard shortcut attribute behaviour for the element, which momstly clicks the element, but has some useful special handling for, e.g. list box elements and hidden elements). [HTMLInputElement::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;l=806) forwards to the underlying [InputTypeView::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type_view.cc;drc=c83b80f441edc1c50de8d3977fa3feeb0ae96bc3;l=79).
The base `InputTypeView::AccessKeyAction()` previously only focused the element, but three of the six existing overloads did a "focus-then-click" with a TODO to merge their implementations into a common base-class, which is also the correct behaviour for the (previously un-overloaded) `TextFieldInputType::AccessKeyAction()`. Rather than add a fourth identical overload, I changed the `InputTypeView::AccessKeyAction()` base-class behaviour to "focus-then-click", removed the three now-redundant overloads, left the existing `HiddenInputType` do-nothing overload as-is, and added an overload for `MultipleFieldsTemporalInputTypeView::AccessKeyAction()` with the previous base-class behaviour.
void HTMLInputElement::AccessKeyAction(This might not be the right place to make the change, I'd follow it in a level and possibly try to change one of the subclass overrides of [InputTypeView::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type_view.cc;l=79;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;bpv=1;bpt=1).
if (IsTextField())crrev.com/c/1388388 (2019) added this check to give TalkBack a way to focus `contenteditable` nodes, but it seems to block explicit "click" actions from ATs from actually clicking nodes (instead of just focusing them), so I've removed it.
// For most elements, AccessKeyAction triggers sending a simulated`AccessKeyAction()` is a handler for the (seldom-used) [`accesskey` attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/accesskey), and is specifically overridden for [HTMLInputElement](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;l=806) to only focus the node rather than the default simulated click in [HTMLElement](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=3354;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65). Given that this is specifically for the "click" accessibility action, I tried to replace this call with `DispatchSimulatedClick()` in PS 1, but that broke some web tests for the `<select>` element, which depended on special handling for its child `<option>` elements.
4 input type text clickedThe changes to this test file make the failed CQ tests from PS 4 pass. They seem to be within the test semantics as described in [fast/forms/access-key.html](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/fast/forms/access-key.html?q=fast%2Fforms%2Faccess-key.html) and match the intentional change made in this CL.
Related details
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 5 comments![]( "Open in Gerrit")
Could you add some context here to explain why clicking is better than just focusing?
void AccessKeyAction(SimulatedClickCreationScope creation_scope) final;The key issue for this bug was that the accessibility click path called `AccessKeyAction()` on its underlying element (reusing the `accesskey` keyboard shortcut attribute behaviour for the element, which momstly clicks the element, but has some useful special handling for, e.g. list box elements and hidden elements). [HTMLInputElement::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;l=806) forwards to the underlying [InputTypeView::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type_view.cc;drc=c83b80f441edc1c50de8d3977fa3feeb0ae96bc3;l=79).
The base `InputTypeView::AccessKeyAction()` previously only focused the element, but three of the six existing overloads did a "focus-then-click" with a TODO to merge their implementations into a common base-class, which is also the correct behaviour for the (previously un-overloaded) `TextFieldInputType::AccessKeyAction()`. Rather than add a fourth identical overload, I changed the `InputTypeView::AccessKeyAction()` base-class behaviour to "focus-then-click", removed the three now-redundant overloads, left the existing `HiddenInputType` do-nothing overload as-is, and added an overload for `MultipleFieldsTemporalInputTypeView::AccessKeyAction()` with the previous base-class behaviour.
I see, so did you add the MultipleFieldsTemporalInputTypeView because you know that those should be focused without being clicked, or just to make sure you didn't change the behavior on those elements?
I think some of this context would be helpful to put into the commit message.
void HTMLInputElement::AccessKeyAction(This might not be the right place to make the change, I'd follow it in a level and possibly try to change one of the subclass overrides of [InputTypeView::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type_view.cc;l=79;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;bpv=1;bpt=1).
Looks like html_input_element.cc isn't being changed in the latest patchset anymore?
if (IsTextField())crrev.com/c/1388388 (2019) added this check to give TalkBack a way to focus `contenteditable` nodes, but it seems to block explicit "click" actions from ATs from actually clicking nodes (instead of just focusing them), so I've removed it.
So does that break the fix that other patch made?
// For most elements, AccessKeyAction triggers sending a simulated`AccessKeyAction()` is a handler for the (seldom-used) [`accesskey` attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/accesskey), and is specifically overridden for [HTMLInputElement](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;l=806) to only focus the node rather than the default simulated click in [HTMLElement](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=3354;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65). Given that this is specifically for the "click" accessibility action, I tried to replace this call with `DispatchSimulatedClick()` in PS 1, but that broke some web tests for the `<select>` element, which depended on special handling for its child `<option>` elements.
I see. Would it still be a lot more appealing to use dispatchsimulatedclick if we could fix the select/option stuff in another way?
Related details
- Aaron Moss
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Moss (Gerrit)
Aaron Moss voted and added 5 comments![]( "Open in Gerrit")
Votes added by Aaron Moss
| Commit-Queue | +1 |
Could you add some context here to explain why clicking is better than just focusing?
Done; the linked bug provides the specific details, but this is about more-closely aligning the behaviour of the "click" accessibility action with an actual click.
void AccessKeyAction(SimulatedClickCreationScope creation_scope) final;Joey ArharThe key issue for this bug was that the accessibility click path called `AccessKeyAction()` on its underlying element (reusing the `accesskey` keyboard shortcut attribute behaviour for the element, which momstly clicks the element, but has some useful special handling for, e.g. list box elements and hidden elements). [HTMLInputElement::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;l=806) forwards to the underlying [InputTypeView::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type_view.cc;drc=c83b80f441edc1c50de8d3977fa3feeb0ae96bc3;l=79).
The base `InputTypeView::AccessKeyAction()` previously only focused the element, but three of the six existing overloads did a "focus-then-click" with a TODO to merge their implementations into a common base-class, which is also the correct behaviour for the (previously un-overloaded) `TextFieldInputType::AccessKeyAction()`. Rather than add a fourth identical overload, I changed the `InputTypeView::AccessKeyAction()` base-class behaviour to "focus-then-click", removed the three now-redundant overloads, left the existing `HiddenInputType` do-nothing overload as-is, and added an overload for `MultipleFieldsTemporalInputTypeView::AccessKeyAction()` with the previous base-class behaviour.
I see, so did you add the MultipleFieldsTemporalInputTypeView because you know that those should be focused without being clicked, or just to make sure you didn't change the behavior on those elements?
I think some of this context would be helpful to put into the commit message.
I added the MFTITV overload to keep the behaviour consistent with its previous behaviour (I could try dropping the overload and running another dry-run if you like, but I'd rather split the separate behaviour change out into a follow-up CL).
MFTITV isn't particularly-well documented, but the depth of discussion on crbug.com/40617194 suggests that a synthetic click might not be a correct solution.
I've also edited the commit message to include more of the what and why from upthread.
void HTMLInputElement::AccessKeyAction(Joey ArharThis might not be the right place to make the change, I'd follow it in a level and possibly try to change one of the subclass overrides of [InputTypeView::AccessKeyAction()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/input_type_view.cc;l=79;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;bpv=1;bpt=1).
Looks like html_input_element.cc isn't being changed in the latest patchset anymore?
Oops, left this comment draft for myself on an earlier PS and forgot to delete it before I sent the CL for review, sorry.
if (IsTextField())Joey Arharcrrev.com/c/1388388 (2019) added this check to give TalkBack a way to focus `contenteditable` nodes, but it seems to block explicit "click" actions from ATs from actually clicking nodes (instead of just focusing them), so I've removed it.
So does that break the fix that other patch made?
Good catch, there's a regression in this patch vs. Canary, I'll set it back to WIP.
// For most elements, AccessKeyAction triggers sending a simulatedJoey Arhar`AccessKeyAction()` is a handler for the (seldom-used) [`accesskey` attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/accesskey), and is specifically overridden for [HTMLInputElement](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65;l=806) to only focus the node rather than the default simulated click in [HTMLElement](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_element.cc;l=3354;drc=54af12075326fb3b52088fcf8c1a34d6fb264f65). Given that this is specifically for the "click" accessibility action, I tried to replace this call with `DispatchSimulatedClick()` in PS 1, but that broke some web tests for the `<select>` element, which depended on special handling for its child `<option>` elements.
I see. Would it still be a lot more appealing to use dispatchsimulatedclick if we could fix the select/option stuff in another way?
The kind of things that the `AccessKeyAction` lets us do (e.g. click listbox elements that haven't been visually expanded) wouldn't work with `DispatchSimulatedClick()`, but "should they work?" is a valid question -- I'll consult the team, I think the key question is "is there a way to accomplish the same thing using any given AT?". I do think not overloading `AccessKeyAction()` is conceptually cleaner here, but a larger change to existing Chrome behaviour.
On the other hand, the "change `AccessKeyAction()` for text fields" approach broke substantially fewer tests, and I didn't actually see a way to make [accessibility/select-option-click.html](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/accessibility/select-option-click.html?q=select-option-click.html) (my representative failed test with `DispatchSimulatedClick()`) work with the simulated click approach.
Related details
- Joey Arhar
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |