Make KeyboardFocusableScrollers keyboard-focus but not mouse-focus [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Aaron Leventhal
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
I'm only an owner of the a11y code. The changes to ax_object.cc look good. However, I wanted to make sure that our list of things to check (from chat) are good to go:
1. We disconnect keyboard focusability from click focusability, right? (That's the main point of this CL I believe)
2. Make scrollers just keyboard focusable. In both cases, IsFocusable() will be true.
3. Make sure the area is still scrollable with the keyboard after a click, as it used to be, even if it is not focused
4. Make sure AXObject::OnNativeClickAction() still sets the focus for scrollable areas. That's the function that's called when a screen reader user asks to activate something.
5. Determine what to do for scrollableArea.focus() -- I assume this should take focus even if it is not click-focusable
Do we need to any any tests for these?
Related details
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Auto-Submit | +0 |
I'm only an owner of the a11y code. The changes to ax_object.cc look good. However, I wanted to make sure that our list of things to check (from chat) are good to go:
1. We disconnect keyboard focusability from click focusability, right? (That's the main point of this CL I believe)
2. Make scrollers just keyboard focusable. In both cases, IsFocusable() will be true.
3. Make sure the area is still scrollable with the keyboard after a click, as it used to be, even if it is not focused
4. Make sure AXObject::OnNativeClickAction() still sets the focus for scrollable areas. That's the function that's called when a screen reader user asks to activate something.
5. Determine what to do for scrollableArea.focus() -- I assume this should take focus even if it is not click-focusableDo we need to any any tests for these?
Yep, thanks!
1. That's right - this CL does that.
2. That's right, scrollers are always IsFocusable() (even when not keyboard focusable, e.g. when they have focusable content), and they are sometimes keyboard focusable. But they're not made to be mouse focusable unless they have a `tabindex`.
3. Yes. Should be verified by tests.
4. Is there a test that verifies this? It should be true but I'd like to confirm.
5. Yes, since IsFocusable() is now true for all scrollers, scroller.focus() works. The new test in this CL verifies this.
TL;DR I need help on #4 but they rest are good.
Related details
- Aaron Leventhal
- David Baron
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
Mason FreedI'm only an owner of the a11y code. The changes to ax_object.cc look good. However, I wanted to make sure that our list of things to check (from chat) are good to go:
1. We disconnect keyboard focusability from click focusability, right? (That's the main point of this CL I believe)
2. Make scrollers just keyboard focusable. In both cases, IsFocusable() will be true.
3. Make sure the area is still scrollable with the keyboard after a click, as it used to be, even if it is not focused
4. Make sure AXObject::OnNativeClickAction() still sets the focus for scrollable areas. That's the function that's called when a screen reader user asks to activate something.
5. Determine what to do for scrollableArea.focus() -- I assume this should take focus even if it is not click-focusableDo we need to any any tests for these?
Yep, thanks!
1. That's right - this CL does that.
2. That's right, scrollers are always IsFocusable() (even when not keyboard focusable, e.g. when they have focusable content), and they are sometimes keyboard focusable. But they're not made to be mouse focusable unless they have a `tabindex`.
3. Yes. Should be verified by tests.
4. Is there a test that verifies this? It should be true but I'd like to confirm.
5. Yes, since IsFocusable() is now true for all scrollers, scroller.focus() works. The new test in this CL verifies this.
TL;DR I need help on #4 but they rest are good.
For #4, you can create a unit test in ax_object_test.cc.
You can copy some of the test setup, and create a scrollable div, then:
```
ui::AXActionData action_data;
action_data.action = ax::mojom::blink::Action::kDoDefault;
const ui::AXTreeID div_child_tree_id = ui::AXTreeID::CreateNewAXTreeID();
action_data.target_node_id = div->AXObjectID();
action_data.child_tree_id = div_child_tree_id;
div->PerformAction(action_data);
```
Finally, check that focus is on the div.
Related details
- David Baron
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
-dbaron for now, I need to fix a couple issues with the test that I just noticed.
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
dbaron@ ok should be ok to review now. It passes locally for me, hopefully the same on the bots.
Related details
- David Baron
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 4 comments![]( "Open in Gerrit")
Bug: 361072782[Semi-standard](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors) way to share credit is to add to the git footer:
```
Co-authored-by: Di Zhang <dizh...@chromium.org>
```
DCHECK(CanBeKeyboardFocusableScroller(update_behavior));I think passing an `update_behavior` to a call inside of a `DCHECK()` (which I think gets removed when `!DCHECK_IS_ON()` is a bad idea, since it means that updating layout depends on `DCHECK_IS_ON()`.
if (HasElementFlag(ElementFlags::kTabIndexWasSetExplicitly)) {
// If the element has a tabindex, then that determines keyboard
// focusability.
return GetIntegralAttribute(html_names::kTabindexAttr, 0) >= 0;
}
if (!CanBeKeyboardFocusableScroller(update_behavior)) {
// IsFocusable is true for some other reason than keyboard focusable
// scrollers. This element should be keyboard focusable.
return true;
}This (and similar code in `IsMouseFocusable`) seems fragile. It seems (if I'm understanding correctly) like it's trying to answer the question "is `CanBeKeyboardFocusableScroller` returning true the only reason that `IsFocusable` returned true". If that's the case, then would it be cleaner to give `IsFocusable` a tri-state result rather than a boolean?
}
// Otherwise, if the element `IsFocusable()`, then it should also be mouse-
// focusable, *unless* it is a keyboard focusable scroller.
if (!CanBeKeyboardFocusableScroller(update_behavior)) {
return true;
}
return !IsKeyboardFocusableScroller(update_behavior);This logic doesn't seem right. This is saying that if a scroller doesn't have a focusable descendant, then it's not mouse focusable (because it is keyboard focusable)... but then if a focusable descendant is added (which makes it no longer keyboard focusable) then it becomes mouse focusable. That seems a little too weird to be what we want. (Or is it?)
Related details
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 6 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Commit-Queue | +1 |
Thanks! Done.
Alright, thanks for the comments. In particular, dbaron@ thanks for the enum suggestion. While that makes this CL a lot bigger, it also makes the logic a lot easier to follow, so I think it's a worthwhile change. I believe the bots should pass with this patchset, but I haven't waited around to make sure. But either way, it'd be great if you could take a look and see if anything seems off.
[Semi-standard](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors) way to share credit is to add to the git footer:
```
Co-authored-by: Di Zhang <dizh...@chromium.org>
```
Never done that before - thanks.
DCHECK(CanBeKeyboardFocusableScroller(update_behavior));I think passing an `update_behavior` to a call inside of a `DCHECK()` (which I think gets removed when `!DCHECK_IS_ON()` is a bad idea, since it means that updating layout depends on `DCHECK_IS_ON()`.
Ohh interesting catch. Ok, I ended up adding a value to the UpdateBehavior enum called `kAssertNoLayoutUpdates`, and there I just assert that the lifecycle doesn't move at all. I also cleaned up the enum just a bit and added some comments. PTAL!
if (HasElementFlag(ElementFlags::kTabIndexWasSetExplicitly)) {
// If the element has a tabindex, then that determines keyboard
// focusability.
return GetIntegralAttribute(html_names::kTabindexAttr, 0) >= 0;
}
if (!CanBeKeyboardFocusableScroller(update_behavior)) {
// IsFocusable is true for some other reason than keyboard focusable
// scrollers. This element should be keyboard focusable.
return true;
}This (and similar code in `IsMouseFocusable`) seems fragile. It seems (if I'm understanding correctly) like it's trying to answer the question "is `CanBeKeyboardFocusableScroller` returning true the only reason that `IsFocusable` returned true". If that's the case, then would it be cleaner to give `IsFocusable` a tri-state result rather than a boolean?
Yes, that's exactly what it's doing. And yes, it's a bit brittle. The issue is that doing things like making `IsFocusable` an enum spreads this knowledge out quite far, since there are many callers (over 60). I could make yet another method like `GetFocusableState` that returns the enum, then check that from `IsFocusable` and avoid changing the callers. Hmm. Maybe I'll try that...
Ok, I ended up doing something like that. I switched to `SupportsFocus()` and `IsFocusableState()` (which is new) returning a `FocusableState` enum that includes no, yes, or KFS. That is a pretty widespread change, but I think it seems cleaner now, at least in the core `Element::Is{Mouse|Keyboard}Focusable()` routines. I also removed some of the default values for the `UpdateBehavior` enum, because it was mostly being passed around anyway.
// Otherwise, if the element `IsFocusable()`, then it should also be mouse-
// focusable, *unless* it is a keyboard focusable scroller.
if (!CanBeKeyboardFocusableScroller(update_behavior)) {
return true;
}
return !IsKeyboardFocusableScroller(update_behavior);This logic doesn't seem right. This is saying that if a scroller doesn't have a focusable descendant, then it's not mouse focusable (because it is keyboard focusable)... but then if a focusable descendant is added (which makes it no longer keyboard focusable) then it becomes mouse focusable. That seems a little too weird to be what we want. (Or is it?)
Yes, I believe this was previously screwed up. Thankfully, with the new enum, the code is quite a bit easier to read, and I think I've eliminated this bug. I also fixed the test accordingly. PTAL.
Related details
- Aaron Leventhal
- David Baron
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 8 comments![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
8 comments
LGTM with a few comments.
I admit that I didn't carefully audit all the callers of `IsFocusable` to check which should be changed back to `IsMouseFocusable`. I'm a little curious how you determined that set. (It doesn't seem like it's a revert of an earlier change, based on digging through history and finding commit cc0fdaf229e85447ee197abe6e2d8d7a2b799afc.)
drop the blank line here -- `Co-authored-by:` should be a part of the footer along with `Bug:` and `Change-Id:`.
[[fallthrough]];Do you need `[[fallthrough]];` after a `CHECK()` or does the compiler know that it never returns?
auto focusable_state = Element::IsFocusableState(update_behavior);`FocusableState` rather than `auto`
auto focusable_state = Element::IsFocusableState(update_behavior);`FocusableState` rather than `auto`
IsFocusableState(update_behavior) != FocusableState::kNotFocusable) {This *could* use `IsFocusable` but it doesn't have to... ok either way, I guess.
// The SupportsFocus call above will ensure style and layout is clean here.That's not true for all overrides of the method; I think you should undo this change.
if (SupportsFocus(update_behavior) == FocusableState::kNotFocusable) {
return FocusableState::kNotFocusable;
}
if (!IsFullscreen()) {
return FocusableState::kFocusable;
}
return HTMLElement::IsFocusableState(update_behavior);I think this could probably do a better job of propagating the `FocusableState` result. I *think* (but you should check this) that the logic boils down to:
```
if (!IsFullscreen()) {
return SupportsFocus(update_behavior);
}
return HTMLElement::IsFocusableState(update_behavior);
```
Related details
- Aaron Leventhal
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 8 comments![]( "Open in Gerrit")
LGTM with a few comments.
I admit that I didn't carefully audit all the callers of `IsFocusable` to check which should be changed back to `IsMouseFocusable`. I'm a little curious how you determined that set. (It doesn't seem like it's a revert of an earlier change, based on digging through history and finding commit cc0fdaf229e85447ee197abe6e2d8d7a2b799afc.)
I will admit that I trusted a) Di who made the patch originally, and b) the passing tests. That's a bit lazy, but based on this comment I manually walked through all callers of `IsFocusable()` (modulo tests) and didn't see anything that looked missed. There was one change I made which is debatable (and I'll need to make sure it passes tests) about dialog initial focus.
drop the blank line here -- `Co-authored-by:` should be a part of the footer along with `Bug:` and `Change-Id:`.
Done
[[fallthrough]];Do you need `[[fallthrough]];` after a `CHECK()` or does the compiler know that it never returns?
I think I still need a fallthrough, since it *does* return (fall through) in the case that the `CHECK` passes, right? (The compiler tells me I'm right... 😊)
auto focusable_state = Element::IsFocusableState(update_behavior);Mason Freed`FocusableState` rather than `auto`
Thought it was too wordy. But ok, done.
auto focusable_state = Element::IsFocusableState(update_behavior);Mason Freed`FocusableState` rather than `auto`
Done
IsFocusableState(update_behavior) != FocusableState::kNotFocusable) {This *could* use `IsFocusable` but it doesn't have to... ok either way, I guess.
Yeah, I like that better, done.
// The SupportsFocus call above will ensure style and layout is clean here.That's not true for all overrides of the method; I think you should undo this change.
Yep, good point. I added a comment to keep future-me from trying to delete it again.
if (SupportsFocus(update_behavior) == FocusableState::kNotFocusable) {
return FocusableState::kNotFocusable;
}
if (!IsFullscreen()) {
return FocusableState::kFocusable;
}
return HTMLElement::IsFocusableState(update_behavior);I think this could probably do a better job of propagating the `FocusableState` result. I *think* (but you should check this) that the logic boils down to:
```
if (!IsFullscreen()) {
return SupportsFocus(update_behavior);
}
return HTMLElement::IsFocusableState(update_behavior);
```
- Aaron Leventhal
- David Baron
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Baron
| Code-Review | +1 |
1 comment
[[fallthrough]];Mason FreedDo you need `[[fallthrough]];` after a `CHECK()` or does the compiler know that it never returns?
I think I still need a fallthrough, since it *does* return (fall through) in the case that the `CHECK` passes, right? (The compiler tells me I'm right... 😊)
Oops, somehow I was half thinking it was a `NOTREACHED()` rather than a `CHECK()`; you definitely do still need a `[[fallthrough]]`.
Related details
- Aaron Leventhal
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Leventhal (Gerrit)
Aaron Leventhal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aaron Leventhal
| Code-Review | +1 |
1 comment
Related details
- Mason Freed
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Commit-Queue | +2 |
Mason FreedLGTM with a few comments.
I admit that I didn't carefully audit all the callers of `IsFocusable` to check which should be changed back to `IsMouseFocusable`. I'm a little curious how you determined that set. (It doesn't seem like it's a revert of an earlier change, based on digging through history and finding commit cc0fdaf229e85447ee197abe6e2d8d7a2b799afc.)
I will admit that I trusted a) Di who made the patch originally, and b) the passing tests. That's a bit lazy, but based on this comment I manually walked through all callers of `IsFocusable()` (modulo tests) and didn't see anything that looked missed. There was one change I made which is debatable (and I'll need to make sure it passes tests) about dialog initial focus.
...and coming back to this, that one "extra" call site I found turned out to break things, so I put it back. Looks like Di's original pass was correct.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/dom/element.cc
Insertions: 2, Deletions: 2.
@@ -6221,9 +6221,9 @@
for (Element& descendant : ElementTraversal::DescendantsOf(*where_to_look)) {
// Dialog elements should only initially focus keyboard focusable elements,
// not mouse focusable elements.
- if (descendant.IsFocusable(UpdateBehavior::kStyleAndLayout) &&
+ if (descendant.IsFocusable() &&
(!IsA<HTMLDialogElement>(this) ||
- descendant.IsKeyboardFocusable(UpdateBehavior::kStyleAndLayout))) {
+ FocusController::AdjustedTabIndex(descendant) >= 0)) {
return &descendant;
}
if (Element* focusable_area =
```
Change information
Make KeyboardFocusableScrollers keyboard-focus but not mouse-focus
This (re-)introduces the concept of IsMouseFocusable, which
represents whether an element should be focused by the mouse. This
is now distinct from IsKeyboardFocusable: elements can be one of
these without being the other. Both require IsFocusable() to be
true.
This behavior is guarded behind the flag KeyboardFocusableScrollers.
When the flag disabled, IsMouseFocusable and IsKeyboardFocusable will
always return the same value. With the flag enabled, scrollers
(without an explicit tabindex) will be keyboard focusable, but will
not be mouse focusable.
Along the way, I ended up (thanks to a comment from dbaron@) doing
a refactor of `SupportsFocus()` and `IsFocusable()` (now called
`IsFocusableState()`) to return a tri-state enum, which explicitly
keeps track of whether the element supports or is focusable due to
keyboard focusable scrollers. That ends up making the logic for
`IsMouseFocusable()` and `IsKeyboardFocusable()` a lot easier to
grok since there aren't hidden dependencies on the logic in
`SupportsFocus()`. I also removed the default parameter values for
both methods, since they were passed explicitly almost everywhere.
- M third_party/blink/renderer/core/dom/container_node.cc
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/dom/element.h
- M third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.cc
- M third_party/blink/renderer/core/html/fenced_frame/html_fenced_frame_element.h
- M third_party/blink/renderer/core/html/forms/clear_button_element.h
- M third_party/blink/renderer/core/html/forms/date_time_field_element.cc
- M third_party/blink/renderer/core/html/forms/date_time_field_element.h
- M third_party/blink/renderer/core/html/forms/html_field_set_element.cc
- M third_party/blink/renderer/core/html/forms/html_field_set_element.h
- M third_party/blink/renderer/core/html/forms/html_form_control_element.cc
- M third_party/blink/renderer/core/html/forms/html_form_control_element.h
- M third_party/blink/renderer/core/html/forms/html_label_element.cc
- M third_party/blink/renderer/core/html/forms/html_opt_group_element.cc
- M third_party/blink/renderer/core/html/forms/html_opt_group_element.h
- M third_party/blink/renderer/core/html/forms/html_option_element.cc
- M third_party/blink/renderer/core/html/forms/html_option_element.h
- M third_party/blink/renderer/core/html/forms/html_output_element.cc
- M third_party/blink/renderer/core/html/forms/html_output_element.h
- M third_party/blink/renderer/core/html/forms/html_select_element.cc
- M third_party/blink/renderer/core/html/forms/html_select_element.h
- M third_party/blink/renderer/core/html/forms/html_select_list_element.h
- M third_party/blink/renderer/core/html/forms/spin_button_element.h
- M third_party/blink/renderer/core/html/forms/text_control_inner_elements.h
- M third_party/blink/renderer/core/html/html_anchor_element.cc
- M third_party/blink/renderer/core/html/html_anchor_element.h
- M third_party/blink/renderer/core/html/html_area_element.cc
- M third_party/blink/renderer/core/html/html_area_element.h
- M third_party/blink/renderer/core/html/html_dialog_element.cc
- M third_party/blink/renderer/core/html/html_dialog_element.h
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/renderer/core/html/html_element.h
- M third_party/blink/renderer/core/html/html_frame_element_base.cc
- M third_party/blink/renderer/core/html/html_frame_element_base.h
- M third_party/blink/renderer/core/html/html_permission_element.cc
- M third_party/blink/renderer/core/html/html_permission_element.h
- M third_party/blink/renderer/core/html/html_plugin_element.cc
- M third_party/blink/renderer/core/html/html_plugin_element.h
- M third_party/blink/renderer/core/html/html_summary_element.cc
- M third_party/blink/renderer/core/html/html_summary_element.h
- M third_party/blink/renderer/core/html/media/html_media_element.cc
- M third_party/blink/renderer/core/html/media/html_media_element.h
- M third_party/blink/renderer/core/input/mouse_event_manager.cc
- M third_party/blink/renderer/core/page/touch_adjustment.cc
- M third_party/blink/renderer/core/svg/svg_a_element.cc
- M third_party/blink/renderer/core/svg/svg_a_element.h
- M third_party/blink/renderer/core/svg/svg_clip_path_element.h
- M third_party/blink/renderer/core/svg/svg_defs_element.h
- M third_party/blink/renderer/core/svg/svg_element.h
- M third_party/blink/renderer/core/svg/svg_graphics_element.h
- M third_party/blink/renderer/modules/accessibility/ax_object.cc
- M third_party/blink/renderer/modules/accessibility/ax_object_test.cc
- A third_party/blink/web_tests/shadow-dom/focus-navigation/focus-scroller-activeElement-on-event.html
- M third_party/blink/web_tests/shadow-dom/resources/focus-utils.js
Code-Review: +1 by Aaron Leventhal, +1 by David Baron
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |