[KeyboardFocusableScrollers] Make scrollers not mouse focusable by default [chromium/src : main]
Di Zhang (Gerrit)
Di Zhang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
2 comments
IsKeyboardFocusableScroller(UpdateBehavior::kNoneForIsFocused));This is to make sure we don't make cases like 1) tabindex=-1 and 2) textarea (with default hidden tabindex=0) not mouse focusable when they should be.
Element::UpdateBehavior::kNoneForAccessibility) &&We can keep this as IsFocusable if we want AX behavior to still make click focus on scroller.
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. |
Aaron Leventhal (Gerrit)
Aaron Leventhal added 2 comments![]( "Open in Gerrit")
Did we implement everything in the list from chat? Does clicking in the area still allow scrolling, even though it's not focused?
Element::UpdateBehavior::kNoneForAccessibility) &&We can keep this as IsFocusable if we want AX behavior to still make click focus on scroller.
Yes, please keep as IsFocusable().
It should also return true for CanSetFocusAttribute().
Related details
- Di Zhang
- Mason Freed
Aaron Leventhal (Gerrit)
Aaron Leventhal added 1 comment![]( "Open in Gerrit")
Did we implement everything in the list from chat? Does clicking in the area still allow scrolling, even though it's not focused?
Unresolved.
Mason Freed (Gerrit)
Mason Freed added 4 comments![]( "Open in Gerrit")
// with no default/adjusted tabindex. Avoid overwriting this function asIn that case, don't make the function `virtual` at all.
IsKeyboardFocusableScroller(UpdateBehavior::kNoneForIsFocused));This is to make sure we don't make cases like 1) tabindex=-1 and 2) textarea (with default hidden tabindex=0) not mouse focusable when they should be.
Maybe
```
if (!Element::IsFocusable(update_behavior)) {
return false;
}
// Any element with tabindex is mouse focusable.
if (HasElementFlag(ElementFlags::kTabIndexWasSetExplicitly)) {
return true;
}
DCHECK_EQ(tabIndex(),DefaultTabIndex());
// If the element's default tabindex is >=0, it should be click focusable.
return DefaultTabIndex() >= 0;
```
Just a little easier to follow.
NOTE: this is a question. Note that I removed the `IsKeyboardFocusableScroller` check, because I don't think it should be there. Your existing logic here says that an element should be mouse focusable if:
1. it has an explicit tabindex, OR
2. it has a default tabindex >= 0, OR
3. it is NOT a keyboard focusable scroller.
But I don't know why #3 is there. I don't think we should make all `<div>`'s mouse focusable. Maybe I'm missing something though, because I would have thought there'd be a test that fails if I'm right about the logic.
await navigateFocusForward();
assert_equals(document.activeElement, childButton, 'navigate forward by tab does not focus scroller');Maybe add one more `navigateFocusForward()` and make sure `activeElement` keeps going past the scroller?
scroller.removeChild(childButton);It's a little cleaner if you add this at the top of the test instead:
```
promise_test(async (t) => {
const childButton = document.createElement('button');
t.add_cleanup(() => childButton.remove());
```
Related details
- Di Zhang
Di Zhang (Gerrit)
Di Zhang voted and added 7 comments![]( "Open in Gerrit")
Votes added by Di Zhang
| Commit-Queue | +1 |
Aaron LeventhalDid we implement everything in the list from chat? Does clicking in the area still allow scrolling, even though it's not focused?
Unresolved.
Points raised in chat:
1. Disconnect keyboard focusability from click focusability => Implemented in Element.cc by adding IsMouseFocusable.
2. Make scrollers just keyboard focusable. In both cases, IsFocusable() will be true. => Covered because we don't touch IsFocusable and IsKeyboardFocusable
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 => Testing in shadow-dom/focus-navigation/focus-scroller-activeElement-on-event.html.
4. Make sure AXObject::OnNativeClickAction() still sets the focus for scrollable areas. => Covered because we are not modifying AXObject and not changing IsFocusable/IsFocusableStyle.
5. Determine what to do for scrollableArea.focus() => Tested in shadow-dom/focus-navigation/focus-scroller-activeElement-on-event.html. This matches behavior in Gecko.
// with no default/adjusted tabindex. Avoid overwriting this function asIn that case, don't make the function `virtual` at all.
Done
return GetIntegralAttribute(html_names::kTabindexAttr, 0) >= 0;Should this be `return DefaultTabIndex() >= 0;` too?
Not sure if this change should be in this CL.
IsKeyboardFocusableScroller(UpdateBehavior::kNoneForIsFocused));Mason FreedThis is to make sure we don't make cases like 1) tabindex=-1 and 2) textarea (with default hidden tabindex=0) not mouse focusable when they should be.
Maybe
```
if (!Element::IsFocusable(update_behavior)) {
return false;
}
// Any element with tabindex is mouse focusable.
if (HasElementFlag(ElementFlags::kTabIndexWasSetExplicitly)) {
return true;
}
DCHECK_EQ(tabIndex(),DefaultTabIndex());
// If the element's default tabindex is >=0, it should be click focusable.
return DefaultTabIndex() >= 0;
```Just a little easier to follow.
NOTE: this is a question. Note that I removed the `IsKeyboardFocusableScroller` check, because I don't think it should be there. Your existing logic here says that an element should be mouse focusable if:
1. it has an explicit tabindex, OR
2. it has a default tabindex >= 0, OR
3. it is NOT a keyboard focusable scroller.
But I don't know why #3 is there. I don't think we should make all `<div>`'s mouse focusable. Maybe I'm missing something though, because I would have thought there'd be a test that fails if I'm right about the logic.
Oh, I think you might be right and IsKeyboardFocusableScroller() is not necessary.
I thought it was necessary because for a default scroller case:
- IsFocusable <- returns true
- !Has tabindex <- returns true
- tabindex < 0 <- might return true if an Element such as div has default tabindex -1
But with your changes:
- !IsFocusable, continue
- Has tabindex <- false, continue
- If default tabindex is negative, will return false. As desired
Will test it out.
Aaron LeventhalWe can keep this as IsFocusable if we want AX behavior to still make click focus on scroller.
Yes, please keep as IsFocusable().
It should also return true for CanSetFocusAttribute().
Ok, sounds good. Call stack for CanSetFocusAttribute includes:
CanSetFocusAttribute()
> UpdateCachedAttributeValuesIfNeeded()
-> ComputeCanSetFocusAttribute()
--> IsFocusableStyle() <- will return true for scroller
We end up not touching any calls in ax_object so everything is focusable the same way as before.
await navigateFocusForward();
assert_equals(document.activeElement, childButton, 'navigate forward by tab does not focus scroller');Maybe add one more `navigateFocusForward()` and make sure `activeElement` keeps going past the scroller?
Done
It's a little cleaner if you add this at the top of the test instead:
```
promise_test(async (t) => {
const childButton = document.createElement('button');
t.add_cleanup(() => childButton.remove());
```
- 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 abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |