Drop HasFocusableNonOptionChild from IsLeaf [chromium/src : master]
Fabian Henneke (Gerrit)
Attention is currently required from: Fabian Henneke, Mark Schillaci.
Fabian Henneke uploaded patch set #4 to this change.
Drop HasFocusableNonOptionChild from IsLeaf
In BrowserAccessibilityAndroid, the function HasFocusableNonOptionChild
is used to determine whether an accessibility node can be treated as a
leaf in IsLeaf. This function can be very costly (on the order of a few
milliseconds) since it traverses the tree.
IsLeaf is eventually called by both RetargetForEvent and CanFireEvents,
which in turn are called in every run of the loop over all events in
BrowserAccessibilityManager::OnAccessibilityEvents. For pages with a
complicated tree (e.g. cs.chromium.org), this can cause complete
unresponsiveness for periods of 10s and more.
This change removes the function.
Bug: 1014945
Change-Id: Idfde20271847425996d1f370e0fa1d8b964aa3f3
AX-Relnotes: n/a.
---
M content/browser/accessibility/browser_accessibility_android.cc
M content/browser/accessibility/browser_accessibility_android.h
2 files changed, 0 insertions(+), 20 deletions(-)
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Fabian Henneke (Gerrit)
Attention is currently required from: Dominic Mazzoni, Mark Schillaci.
1 comment:
Patchset:
That's a great find. I think HasFocusableNonOptionChild is definitely […]
Removing the call (that is, the entire if statement) has a strong positive effect on the responsiveness of the browser during page load: The ~20s of unresponsiveness are gone and everything is very smooth, apart from a few short "lags" very early on page load. This finally makes cs.chromium.org usable with Accessibility/Autofill enabled. While the effect is very similar to the one Patchset 1 had, I would certainly prefer getting rid of the call entirely as this could help improve performance of events other than SUBTREE_CREATED.
Looking at IsLeaf(), the call to HasFocusableNonOptionChild() looks almost redundant given that IsLeaf() returns false anyway apart from a few edge cases. Fingers crossed that removing the call doesn't break too many tests.
I added more logging and can confirm that the remaining periods of unresponsiveness are mostly caused by the remaining calls to `Unserialize()`, which takes about 500ms each of the four times it is called when loading the page mentioned above. I suspect this is expected for a tree this large?
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Fabian Henneke (Gerrit)
Attention is currently required from: Dominic Mazzoni, Mark Schillaci.
Patch set 4:Commit-Queue +1
Fabian Henneke (Gerrit)
Attention is currently required from: Dominic Mazzoni, Mark Schillaci.
Fabian Henneke uploaded patch set #5 to this change.
Drop HasFocusableNonOptionChild from IsLeaf
In BrowserAccessibilityAndroid, the function HasFocusableNonOptionChild
is used to determine whether an accessibility node can be treated as a
leaf in IsLeaf. This function can be very costly (on the order of a few
milliseconds) since it traverses the tree.
IsLeaf is eventually called by both RetargetForEvent and CanFireEvents,
which in turn are called in every run of the loop over all events in
BrowserAccessibilityManager::OnAccessibilityEvents. For pages with a
complicated tree (e.g. cs.chromium.org), this can cause complete
unresponsiveness for periods of 10s and more.
This change moves the call to HasFocusableNonOptionChild behind the
individual checks for special situations in which the lack of focusable
children would allow to prune the accessibility tree. This results in
far less calls to the function while preserving the structure of the
tree.
Bug: 1014945
Change-Id: Idfde20271847425996d1f370e0fa1d8b964aa3f3
AX-Relnotes: n/a.
---
M content/browser/accessibility/browser_accessibility_android.cc
1 file changed, 14 insertions(+), 8 deletions(-)
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Fabian Henneke (Gerrit)
Attention is currently required from: Dominic Mazzoni, Mark Schillaci.
Patch set 4:Commit-Queue +1
1 comment:
Patchset:
Patchset 4 (please ignore 2 and 3) breaks quite a few tests in an essential way: Headings and focusable nodes might very well contain important focusable children.
I uploaded a new patchset that should preserve the current tree structure, but moves the call to HasFocusableNonOptionChild into the individual checks for special cases. I will report back with performance measurements.
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Fabian Henneke (Gerrit)
Attention is currently required from: Dominic Mazzoni, Mark Schillaci.
Patch set 5:Commit-Queue +1
Fabian Henneke (Gerrit)
Attention is currently required from: Dominic Mazzoni, Mark Schillaci.
1 comment:
Patchset:
The current patchset achieves a performance gain and all tests pass.
I am not sure whether accessibility nodes with only static text children can have focusable children, but at least the tests don't seem to contain any examples.
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Fabian Henneke (Gerrit)
Patchset:
The current patchset achieves a performance gain and all tests pass. […]
*a very similar performance gain
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Mazzoni (Gerrit)
Attention is currently required from: Fabian Henneke, Mark Schillaci.
Patch set 5:Code-Review +1Commit-Queue +2
1 comment:
Patchset:
Thank you!
Since all tests pass, this definitely lgtm. I think there's a
possibility of a corner case left so I'll try to follow up
by coming up with a more efficient way to keep track of whether
a node has a focusable descendant.
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Fabian Henneke (Gerrit)
Attention is currently required from: Mark Schillaci.
1 comment:
Patchset:
Thank you!
Since all tests pass, this definitely lgtm. I think there's a
possibility of a corner case left so I'll try to follow up
by coming up with a more efficient way to keep track of whether
a node has a focusable descendant.
Thanks! I just noticed that I forgot to update the commit message title, which is now slightly confusing. I hope that is not a big deal.
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Mazzoni (Gerrit)
Attention is currently required from: Fabian Henneke, Mark Schillaci.
1 comment:
Patchset:
> Thank you! […]
Feel free to update to something more clear and then land, no
need for re-review. Thanks.
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Drop HasFocusableNonOptionChild from IsLeaf
In BrowserAccessibilityAndroid, the function HasFocusableNonOptionChild
is used to determine whether an accessibility node can be treated as a
leaf in IsLeaf. This function can be very costly (on the order of a few
milliseconds) since it traverses the tree.
IsLeaf is eventually called by both RetargetForEvent and CanFireEvents,
which in turn are called in every run of the loop over all events in
BrowserAccessibilityManager::OnAccessibilityEvents. For pages with a
complicated tree (e.g. cs.chromium.org), this can cause complete
unresponsiveness for periods of 10s and more.
This change moves the call to HasFocusableNonOptionChild behind the
individual checks for special situations in which the lack of focusable
children would allow to prune the accessibility tree. This results in
far less calls to the function while preserving the structure of the
tree.
Bug: 1014945
Change-Id: Idfde20271847425996d1f370e0fa1d8b964aa3f3
AX-Relnotes: n/a.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580802
Reviewed-by: Dominic Mazzoni <dmaz...@chromium.org>
Commit-Queue: Dominic Mazzoni <dmaz...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836650}
---
M content/browser/accessibility/browser_accessibility_android.cc
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/content/browser/accessibility/browser_accessibility_android.cc b/content/browser/accessibility/browser_accessibility_android.cc
index ac8c8ea..1e02e70 100644
--- a/content/browser/accessibility/browser_accessibility_android.cc
+++ b/content/browser/accessibility/browser_accessibility_android.cc
@@ -473,21 +473,27 @@
if (IsLink())
return false;
- // If it has a focusable child, we definitely can't leave out children.
- if (HasFocusableNonOptionChild())
- return false;
-
BrowserAccessibilityManagerAndroid* manager_android =
static_cast<BrowserAccessibilityManagerAndroid*>(manager());
if (manager_android->prune_tree_for_screen_reader()) {
// Headings with text can drop their children.
base::string16 name = GetInnerText();
- if (GetRole() == ax::mojom::Role::kHeading && !name.empty())
- return true;
+ if (GetRole() == ax::mojom::Role::kHeading && !name.empty()) {
+ if (HasFocusableNonOptionChild()) {
+ return false;
+ } else {
+ return true;
+ }
+ }
// Focusable nodes with text can drop their children.
- if (HasState(ax::mojom::State::kFocusable) && !name.empty())
- return true;
+ if (HasState(ax::mojom::State::kFocusable) && !name.empty()) {
+ if (HasFocusableNonOptionChild()) {
+ return false;
+ } else {
+ return true;
+ }
+ }
// Nodes with only static text as children can drop their children.
if (HasOnlyTextChildren())
To view, visit change 2580802. To unsubscribe, or for help writing mail filters, visit settings.