SupportsFocus() should return true for all scrollers [chromium/src : main]
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang.
Di Zhang uploaded patch set #5 to this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
4 files changed, 26 insertions(+), 21 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang.
Di Zhang uploaded patch set #6 to this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
4 files changed, 26 insertions(+), 21 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang.
Di Zhang uploaded patch set #7 to this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
4 files changed, 28 insertions(+), 23 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed.
3 comments:
Commit Message:
Patch Set #3, Line 7:
KeyboardFocusableScrollersnit: this needs a shorter (<72 char) title.
Done
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #3, Line 6027:
SupportsScrollerFocusI like that this is shorter name - the long one below is awful. (And I can say that - I chose it.) […]
Done
File third_party/blink/renderer/core/page/focus_controller.cc:
Patch Set #3, Line 501:
inline bool IsNonKeyboardFocusableShadowHost(const Element& element) {I'm having a hard time following the logic here. […]
Honestly, that is my understanding as well (shadow host means there should be a shadow root). Let me change to this logic and see if that causes any test to fail.
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Di Zhang.
5 comments:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #7, Line 1681:
CanBeKeyboardFocusableScrollerIt would be good to add a comment about what this does also. And that it might update layout, right?
bool CanBeKeyboardFocusableScroller() const;
// This checks whether the element is a scrollable container that should be
// made keyboard focusable. Note that this is slow, because it must do a tree
// walk to look for descendant focusable nodes.
bool IsKeyboardFocusableScroller()It's always a good idea to put function declarations *above* the variables. So `element_data` above should be last, and these should be before that.
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #7, Line 6074:
// Note that IsKeyboardFocusableScroller is slow.I don't think you need this comment any more.
Patch Set #7, Line 6076:
CanBeKeyboardFocusableScrollerThis will get called more often with this code. Previously, the call to IsScrollableContainerThatShould... method was last. Now, this will get hit for any element that doesn't have a `tabindex` attribute, which means we'll now be calling IsScrollableNode() more often, which (I think!) means we might be updating layout more often.
Is that right? Are you worried?
File third_party/blink/renderer/core/page/focus_controller.cc:
!(element.IsKeyboardFocusable() ||
element.GetIntegralAttribute(html_names::kTabindexAttr, 0) < 0);Can you add a comment explaining this? On its face it doesn't make sense. It says we're a non-keyboard focusable shadow host if the element isn't keyboard focusable AND it has a >=0 tabindex. But tabindex>=0 makes it keyboard focusable.
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang.
Di Zhang uploaded patch set #8 to this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
4 files changed, 33 insertions(+), 23 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed.
5 comments:
File third_party/blink/renderer/core/dom/element.h:
Patch Set #7, Line 1681:
CanBeKeyboardFocusableScroller
It would be good to add a comment about what this does also. […]
Done
bool CanBeKeyboardFocusableScroller() const;
// This checks whether the element is a scrollable container that should be
// made keyboard focusable. Note that this is slow, because it must do a tree
// walk to look for descendant focusable nodes.
bool IsKeyboardFocusableScroller()
It's always a good idea to put function declarations *above* the variables. […]
Done
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #7, Line 6074:
// Note that IsKeyboardFocusableScroller is slow.I don't think you need this comment any more.
Until I fix the caching, IsKeyboardFocusableScroller() is still traversing the children to determine if the scroller can be focusable..
Patch Set #7, Line 6076:
CanBeKeyboardFocusableScroller
This will get called more often with this code. […]
Logically, this order makes sense so I am not _that_ worried.
During SupportsFocus/IsFocusable, we check if element is a scroller.
During IsKeyboardFocusable, we check if element is a scroller again if tabindex is not set. But when we reached this point, I would expect the layout to be up to date.
File third_party/blink/renderer/core/page/focus_controller.cc:
!(element.IsKeyboardFocusable() ||
element.GetIntegralAttribute(html_names::kTabindexAttr, 0) < 0);
Can you add a comment explaining this? On its face it doesn't make sense. […]
Done
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Di Zhang.
3 comments:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #1, Line 6078:
SupportsFocusIf we call IsScrollableContainerThatShouldBeKeyboardFocusable() in IsFocusable, then there is no nee […]
I think this comment has been addressed, correct?
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #7, Line 6074:
// Note that IsKeyboardFocusableScroller is slow.
Until I fix the caching, IsKeyboardFocusableScroller() is still traversing the children to determine […]
Yeah, I just meant that the original comment was pointing out that it gets called **twice**. Now that it's not being called by SupportsFocus any longer, it's probably not necessary to point out that it is slow.
Patch Set #7, Line 6076:
CanBeKeyboardFocusableScroller
Logically, this order makes sense so I am not _that_ worried. […]
Ok, sounds good! We should just watch for performance regressions after this lands.
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang.
Di Zhang uploaded patch set #9 to this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
5 files changed, 35 insertions(+), 19 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang.
Di Zhang uploaded patch set #10 to this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
5 files changed, 34 insertions(+), 18 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed.
Patch set 10:Commit-Queue +1
3 comments:
File chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js:
Patch Set #9, Line 426:
.call(doCmd('nextObject'))With the mismatch between "IsFocussable" and "IsKeyboardFocusable" now, this needs an extra tab to get to the next speech item (to pass the focus on scroller with child).
This is because chromevox uses ax_object.cc code, which uses ComputeCanSetFocusAttribute() (return elem->SupportsFocus())
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #1, Line 6078:
SupportsFocusIf we call IsScrollableContainerThatShouldBeKeyboardFocusable() in IsFocusable, then there is no nee […]
Acknowledged
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #7, Line 6074:
// Note that IsKeyboardFocusableScroller is slow.
Yeah, I just meant that the original comment was pointing out that it gets called **twice**. […]
Acknowledged
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Di Zhang.
4 comments:
Patchset:
Only one real question, about the tabindex thing. I don't understand that.
File chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js:
Patch Set #9, Line 426:
.call(doCmd('nextObject'))
With the mismatch between "IsFocussable" and "IsKeyboardFocusable" now, this needs an extra tab to g […]
Interesting. Is it that this test actually contains a scroller that is now keyboard focusable (because it has no descendent focusable elements)? Or is it that the test (perhaps incorrectly) uses SupportsFocus() when it should use IsKeyboardFocusable()?
(I'm ok with this change, it's more for whomever will review this file.)
File third_party/blink/renderer/core/dom/element.h:
Patch Set #10, Line 1685:
Call to IsScrollableNodenit: s/Call to IsScrollableNode/This
File third_party/blink/renderer/core/page/focus_controller.cc:
Patch Set #10, Line 510:
tabindex is set to negativeThis part I really still do not understand. If its tabindex is negative, then it **isn't** keyboard focusable.
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang.
Di Zhang uploaded patch set #11 to this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
5 files changed, 38 insertions(+), 18 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed.
Patch set 11:Commit-Queue +1
3 comments:
File chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js:
Patch Set #9, Line 426:
.call(doCmd('nextObject'))
Interesting. […]
It seems that this test contains a scroller is not keyboard focusable, but do support focus (so _has_ focusable children). Since ax_object.cc code use SupportsFocus(), this scroller is now returning true.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.cc;l=4220;drc=63d3f56debfce4e0d3eba2aeefb91be0a0367c4a
This test might be using SupportsFocus incorrectly, but there is already a TODO (crbug.com/1489580 which you own actually) that mentions it might be wise to use IsFocusable() or IsKeyboardFocusable() instead. But I didn't want to make/mix the change on this patch. What do you think?(I checked with the person who wrote this test and they seem ok to modify it)
File third_party/blink/renderer/core/dom/element.h:
Patch Set #10, Line 1685:
Call to IsScrollableNodenit: s/Call to IsScrollableNode/This
Rephrased (?)
File third_party/blink/renderer/core/page/focus_controller.cc:
Patch Set #10, Line 510:
tabindex is set to negative
This part I really still do not understand. […]
Rewrote this to explain better. I agree with your statement, this condition is meant to check for when KF is false. In that case, tabindex could be negative, but isn't always the case (i.e. scroller with focusable children).
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Di Zhang.
Patch set 11:Code-Review +1
4 comments:
Patchset:
LGTM! Good luck!
File chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js:
Patch Set #9, Line 426:
.call(doCmd('nextObject'))
It seems that this test contains a scroller is not keyboard focusable, but do support focus (so _has […]
Ok, if the person who wrote the test is ok, then I guess ok.
File third_party/blink/renderer/core/dom/element.h:
Patch Set #10, Line 1685:
Call to IsScrollableNode
Rephrased (?)
👍
File third_party/blink/renderer/core/page/focus_controller.cc:
Patch Set #10, Line 510:
tabindex is set to negative
Rewrote this to explain better. […]
Yes, thanks. That helps.
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Akihiro Ota.
Di Zhang would like Akihiro Ota to review this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
---
M chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
5 files changed, 38 insertions(+), 18 deletions(-)
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Akihiro Ota.
LGTM! Good luck!
Thanks!
Adding reviewer for chromevox test change.
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Akihiro Ota (Gerrit)
1 comment:
Patchset:
ChromeVox LGTM
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Patch set 11:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
SupportsFocus() should return true for all scrollers
For feature KeyboardFocusableScrollers, SupportsFocus() call IsScrollableContainerThatShouldBeKeyboardFocusable which can potentially
traverse through the scroller's children to find focusable child.
This isn't necessary to check if an element should support focus as
all scrollers should be focusable. Instead, we call IsScrollableNode().
We also rename function
IsScrollableContainerThatShouldBeKeyboardFocusable to
IsKeyboardFocusableScroller.
Change-Id: Iaa6b4fc754a5b15978c3ebfa4d006b5a491b3e7d
Bug: 1499038
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5001904
Reviewed-by: Mason Freed <mas...@chromium.org>
Commit-Queue: Di Zhang <dizh...@chromium.org>
Reviewed-by: Akihiro Ota <akihi...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1223868}
---
M chrome/browser/resources/chromeos/accessibility/chromevox/panel/tutorial_test.js
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
5 files changed, 38 insertions(+), 18 deletions(-)
Darren Shen (Gerrit)
Darren Shen has created a revert of this change.
To view, visit change 5001904. To unsubscribe, or for help writing mail filters, visit settings.