Deflake focus tests that relied on stale layout information [chromium/src : main]
Di Zhang (Gerrit)
Attention is currently required from: Philip Rogers.
Di Zhang uploaded patch set #2 to the change originally created by Philip Rogers.
Deflake focus tests that relied on stale layout information
KeyboardFocusableScrollers is enabled for tests and allows focus for
scrollable elements. The logic for whether an object is scrollable
depends on layout size, but `IsScrollableNode` was called without
updating layout. This patch adds a CHECK that layout is up to date, and
updates layout in `Element::Focus`.
Bug: 1444332
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
2 files changed, 7 insertions(+), 0 deletions(-)
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed, Philip Rogers.
Di Zhang would like Mason Freed to review this change authored by Philip Rogers.
Deflake focus tests that relied on stale layout information
KeyboardFocusableScrollers is enabled for tests and allows focus for
scrollable elements. The logic for whether an object is scrollable
depends on layout size, but `IsScrollableNode` was called without
updating layout. This patch adds a CHECK that layout is up to date, and
updates layout in `Element::Focus`.
Bug: 1444332
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/third_party/blink/renderer/core/dom/element.cc b/third_party/blink/renderer/core/dom/element.cc
index 2bad349f..1bfac14 100644
--- a/third_party/blink/renderer/core/dom/element.cc
+++ b/third_party/blink/renderer/core/dom/element.cc
@@ -5224,6 +5224,12 @@
GetDocument().UpdateStyleAndLayoutTreeForNode(this,
DocumentUpdateReason::kFocus);
+ // `IsScrollableNode` is used by keyboard focusable scrollers (see:
+ // `Element::IsKeyboardFocusable`) and requires up-to-date layout.
+ if (RuntimeEnabledFeatures::KeyboardFocusableScrollersEnabled()) {
+ GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kFocus);
+ }
+
// https://html.spec.whatwg.org/C/#focusing-steps
//
// 1. If new focus target is not a focusable area, ...
diff --git a/third_party/blink/renderer/core/page/spatial_navigation.cc b/third_party/blink/renderer/core/page/spatial_navigation.cc
index 79869f51..b5e40d83 100644
--- a/third_party/blink/renderer/core/page/spatial_navigation.cc
+++ b/third_party/blink/renderer/core/page/spatial_navigation.cc
@@ -336,6 +336,7 @@
return true;
if (auto* box = DynamicTo<LayoutBox>(node->GetLayoutObject())) {
+ CHECK(!box->NeedsLayout());
return box->IsUserScrollable();
}
return false;
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed, Philip Rogers.
1 comment:
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #2, Line 339:
CHECK(!box->NeedsLayout());This check is failing because the check for SupportsFocus() (which will call IsScrollableNode) is often used independent from whether Focus() gets called.
Do you think the call GetDocument().UpdateStyleAndLayout() should be in SupportsFocus() instead? Although that would really increase the number of update style calls since not all elements that can support focus will get focused.
What about calling it in Element::IsScrollableContainerThatShouldBeKeyboardFocusable()?
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Di Zhang, Philip Rogers.
1 comment:
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #2, Line 339:
CHECK(!box->NeedsLayout());
This check is failing because the check for SupportsFocus() (which will call IsScrollableNode) is of […]
So I think it'd be a bad idea to call `UpdateStyleAndLayout()` in SupportsFocus, for performance reasons. But we could call something like `UpdateStyleAndLayoutIfNeeded` if there was such a thing on `Document` (there isn't currently). I think it's perfectly reasonable to update style and layout to check on focusability, if style/layout are dirty. And yes, I think `IsScrollableContainerThatShouldBeKeyboardFocusable()` is probably the right place to put that call, since that comes after a bunch of faster checks.
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang, Philip Rogers.
Di Zhang uploaded patch set #3 to the change originally created by Philip Rogers.
Deflake focus tests that relied on stale layout information
KeyboardFocusableScrollers is enabled for tests and allows focus for
scrollable elements. The logic for whether an object is scrollable
depends on layout size, but `IsScrollableNode` was called without
updating layout. This patch adds a CHECK that layout is up to date, and
updates layout in `Element::Focus`.
Bug: 1444332
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
2 files changed, 4 insertions(+), 4 deletions(-)
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Philip Rogers.
2 comments:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #3, Line 5515:
// FIXME: supportsFocus() can be called when layout is not up to date.This FIXME is outdated, the function layoutObjectIsFocusable doesn't exist anymore.
Further, the checks below (outside the scroller check) don't need an updated layout.Patch Set #3, Line 5518:
DocumentLifecycle::DisallowTransitionScope scope(GetDocument().Lifecycle());I wonder if having this line will block the UpdateStyleAndLayout() call.
masonf@, why is this line necessary?
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #3, Line 5518:
DocumentLifecycle::DisallowTransitionScope scope(GetDocument().Lifecycle());I wonder if having this line will block the UpdateStyleAndLayout() call. […]
Yes, this will cause a CHECK failure if you call UpdateStyleAndLayout, so this would need to be removed. This was here before to "prove" that SupportsFocus did not update style or layout. This CL changes that, so you'll need to make sure to update the comment above SupportsFocus in element.h accordingly. But this line can/should be removed.
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang, Philip Rogers.
Di Zhang uploaded patch set #4 to the change originally created by Philip Rogers.
Deflake focus tests that relied on stale layout information
KeyboardFocusableScrollers is enabled for tests and allows focus for
scrollable elements. The logic for whether an object is scrollable
depends on layout size, but `IsScrollableNode` was called without
updating layout. This patch adds a CHECK that layout is up to date, and
updates layout in `Element::Focus`.
Bug: 1444332
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
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/spatial_navigation.cc
3 files changed, 7 insertions(+), 7 deletions(-)
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang, Philip Rogers.
Di Zhang uploaded patch set #5 to the change originally created by Philip Rogers.
Deflake focus tests that relied on stale layout information
KeyboardFocusableScrollers is enabled for tests and allows focus for
scrollable elements. The logic for whether an object is scrollable
depends on layout size, but `IsScrollableNode` was called without
updating layout. This patch adds a CHECK that layout is up to date, and
updates layout in `Element::Focus`.
Bug: 1444332
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
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/spatial_navigation.cc
3 files changed, 6 insertions(+), 6 deletions(-)
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Philip Rogers.
Di Zhang uploaded patch set #6 to the change originally created by Philip Rogers.
Deflake focus tests that relied on stale layout information
The logic for whether an object is scrollable depends on layout size,
but `IsScrollableNode` was called without updating layout. This patch
updates layout if needed before checking if element is a scrollable
node.
Fixed: 1445159
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
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/spatial_navigation.cc
3 files changed, 6 insertions(+), 6 deletions(-)
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Philip Rogers.
Di Zhang uploaded patch set #7 to the change originally created by Philip Rogers.
Deflake focus tests that relied on stale layout information
The logic for whether an object is scrollable depends on layout size,
but `IsScrollableNode` was called without updating layout. This patch
updates layout if needed before checking if element is a scrollable
node.
Fixed: 1445159
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
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/spatial_navigation.cc
A third_party/blink/web_tests/shadow-dom/focus-navigation/focus-scroller-layout-update.html
4 files changed, 34 insertions(+), 6 deletions(-)
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed, Philip Rogers.
Patch set 7:Commit-Queue +1
2 comments:
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #3, Line 5518:
DocumentLifecycle::DisallowTransitionScope scope(GetDocument().Lifecycle());
Yes, this will cause a CHECK failure if you call UpdateStyleAndLayout, so this would need to be remo […]
Done
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #2, Line 339:
CHECK(!box->NeedsLayout());
So I think it'd be a bad idea to call `UpdateStyleAndLayout()` in SupportsFocus, for performance rea […]
Done
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Di Zhang, Philip Rogers.
Patch set 7:Code-Review +1
5 comments:
Patchset:
LGTM with several comments/questions. Thanks!
File third_party/blink/renderer/core/dom/element.h:
and
// it might update layout itself to check a condition (for example to see if
// an element's up-to-date layout is a scroller).nit:
`but in some cases it might run a style/layout lifecycle update on the document.`
File third_party/blink/renderer/core/dom/element.cc:
Patch Set #3, Line 5515:
// FIXME: supportsFocus() can be called when layout is not up to date.This FIXME is outdated, the function layoutObjectIsFocusable doesn't exist anymore. […]
Done
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #7, Line 331:
IsScrollableNodeYou should update the comment above this function in the .h file to include that it might trigger a style/layout lifecycle update.
File third_party/blink/web_tests/shadow-dom/focus-navigation/focus-scroller-layout-update.html:
Nice test. Did you make sure it fails before this patch?
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Di Zhang, Mason Freed, Philip Rogers.
Di Zhang uploaded patch set #8 to the change originally created by Philip Rogers.
The following approvals got outdated and were removed: Code-Review+1 by Mason Freed
The change is no longer submittable: Code-Owners and Code-Review are unsatisfied now.
Deflake focus tests that relied on stale layout information
The logic for whether an object is scrollable depends on layout size,
but `IsScrollableNode` was called without updating layout. This patch
updates layout if needed before checking if element is a scrollable
node.
Fixed: 1445159
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
---
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/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
A third_party/blink/web_tests/shadow-dom/focus-navigation/focus-scroller-layout-update.html
5 files changed, 37 insertions(+), 9 deletions(-)
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Attention is currently required from: Mason Freed, Philip Rogers.
Patch set 8:Commit-Queue +2
4 comments:
Patchset:
Thanks!
File third_party/blink/renderer/core/dom/element.h:
and
// it might update layout itself to check a condition (for example to see if
// an element's up-to-date layout is a scroller).
nit: […]
Done
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #7, Line 331:
IsScrollableNode
You should update the comment above this function in the . […]
Done
File third_party/blink/web_tests/shadow-dom/focus-navigation/focus-scroller-layout-update.html:
Nice test. […]
Yes
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Di Zhang, Philip Rogers.
Patch set 8:Code-Review +1
1 comment:
Patchset:
Thanks! LGTM.
To view, visit change 4527136. To unsubscribe, or for help writing mail filters, visit settings.
Di Zhang (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Deflake focus tests that relied on stale layout information
The logic for whether an object is scrollable depends on layout size,
but `IsScrollableNode` was called without updating layout. This patch
updates layout if needed before checking if element is a scrollable
node.
Fixed: 1445159
Change-Id: I647df39bb5709dea8f0ba285b184247556d8361e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4527136
Commit-Queue: Di Zhang <dizh...@chromium.org>
Reviewed-by: Mason Freed <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1202725}
---
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/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
A third_party/blink/web_tests/shadow-dom/focus-navigation/focus-scroller-layout-update.html
5 files changed, 37 insertions(+), 9 deletions(-)