[selectmenu] Remove re-entrant layout for selectmenu popup positioning [chromium/src : main]
Dan Clark (Gerrit)
Attention is currently required from: Ionel Popescu, Mason Freed.
Dan Clark would like Ionel Popescu and Mason Freed to review this change.
[selectmenu] Remove re-entrant layout for selectmenu popup positioning
HTMLPopupElement::AdjustPopupPositionForSelectMenu uses the
LocalDOMWindow innerWidth/innerHeight getters, which can trigger a
relayout of the current document if viewport is enabled. Since we're
already doing layout at this point, this triggers a CHECK.
Update HTMLPopupElement::AdjustPopupPositionForSelectMenu to get the
size from the LocalFrameView directly such that the recursive
layout recalc is avoided.
Bug: 1121840
Change-Id: I953822deb9349ff1fca822ced4e032c7a6a47c41
---
M third_party/blink/renderer/core/html/html_popup_element.cc
A third_party/blink/web_tests/html/selectmenu/selectmenu-viewport.html
2 files changed, 44 insertions(+), 2 deletions(-)
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Dan Clark (Gerrit)
Attention is currently required from: Ionel Popescu, Mason Freed.
1 comment:
Patchset:
This fixes a crash I was hitting in Android when working on https://chromium-review.googlesource.com/c/chromium/src/+/3138235.
PTAL, thanks!
For anyone wondering, the crash stack was:
FATAL:document.cc(2122)] Check failed: false. We should not re-enter style recalc for the same document
0018583b logging::CheckError::~CheckError() ../../base/check.cc:106:3
01599276 blink::Document::UpdateStyleAndLayoutTreeForThisDocument() ../../third_party/blink/renderer/core/dom/document.cc:2122:5
0184f5aa blink::LocalFrameView::UpdateStyleAndLayoutInternal() ../../third_party/blink/renderer/core/frame/local_frame_view.cc:3336:28
018488d6 blink::LocalFrameView::UpdateStyleAndLayout() ../../third_party/blink/renderer/core/frame/local_frame_view.cc:3290:21
01598f52 blink::Document::UpdateStyleAndLayout(blink::DocumentUpdateReason) ../../third_party/blink/renderer/core/dom/document.cc:2483:17
0180ca21 blink::LocalDOMWindow::GetViewportSize() const ../../third_party/blink/renderer/core/frame/local_dom_window.cc:1355:17
0180cb20 blink::LocalDOMWindow::innerWidth() const ../../third_party/blink/renderer/core/frame/local_dom_window.cc:1382:43
019e2498 blink::HTMLPopupElement::AdjustPopupPositionForSelectMenu(blink::ComputedStyle&) ../../third_party/blink/renderer/core/html/html_popup_element.cc:373:29
019e2418 blink::HTMLPopupElement::CustomStyleForLayoutObject(blink::StyleRecalcContext const&) ../../third_party/blink/renderer/core/html/html_popup_element.cc:356:5
015e5d3b blink::Element::StyleForLayoutObject(blink::StyleRecalcContext const&) ../../third_party/blink/renderer/core/dom/element.cc:2855:13
015e6ab1 blink::Element::RecalcOwnStyle(blink::StyleRecalcChange, blink::StyleRecalcContext const&) ../../third_party/blink/renderer/core/dom/element.cc:3151:19
015e63b6 blink::Element::RecalcStyle(blink::StyleRecalcChange, blink::StyleRecalcContext const&) ../../third_party/blink/renderer/core/dom/element.cc:2948:20
0153419e blink::StyleEngine::RecalcStyle(blink::StyleRecalcChange, blink::StyleRecalcContext const&) ../../third_party/blink/renderer/core/css/style_engine.cc:2224:16
01534553 blink::StyleEngine::RecalcStyle() ../../third_party/blink/renderer/core/css/style_engine.cc:2245:3
015347f6 blink::StyleEngine::UpdateStyleAndLayoutTree() ../../third_party/blink/renderer/core/css/style_engine.cc:2311:7
01599a6e blink::Document::UpdateStyle() ../../third_party/blink/renderer/core/dom/document.cc:2212:20
0159942f blink::Document::UpdateStyleAndLayoutTreeForThisDocument() ../../third_party/blink/renderer/core/dom/document.cc:2159:3
015974ee blink::Document::UpdateStyleAndLayoutTree() ../../third_party/blink/renderer/core/dom/document.cc:2063:3
0159a48a blink::Document::UpdateStyleAndLayoutTreeForNode(blink::Node const*) ../../third_party/blink/renderer/core/dom/document.cc:2313:3
019e1b75 blink::HTMLPopupElement::SetFocus() ../../third_party/blink/renderer/core/html/html_popup_element.cc:158:17
019e18ec blink::HTMLPopupElement::show() ../../third_party/blink/renderer/core/html/html_popup_element.cc:103:3
01980799 blink::HTMLSelectMenuElement::OpenListbox() ../../third_party/blink/renderer/core/html/forms/html_select_menu_element.cc:331:20
0198159e blink::HTMLSelectMenuElement::ButtonPartEventListener::Invoke(blink::ExecutionContext*, blink::Event*) ../../third_party/blink/renderer/core/html/forms/html_select_menu_element.cc:745:27
0161684d blink::EventTarget::FireEventListeners(blink::Event&, blink::EventTargetData*, blink::HeapVector<blink::RegisteredEventListener, 1u>&) ../../third_party/blink/renderer/core/dom/events/event_target.cc:896:15
0161628c blink::EventTarget::FireEventListeners(blink::Event&) ../../third_party/blink/renderer/core/dom/events/event_target.cc:810:29
0163ccf0 blink::Node::HandleLocalEvents(blink::Event&) ../../third_party/blink/renderer/core/dom/node.cc:2917:3
01618b3b blink::NodeEventContext::HandleLocalEvents(blink::Event&) const ../../third_party/blink/renderer/core/dom/events/node_event_context.cc:55:10
016046dc blink::EventDispatcher::DispatchEventAtBubbling() ../../third_party/blink/renderer/core/dom/events/event_dispatcher.cc:293:21
0160424f blink::EventDispatcher::Dispatch() ../../third_party/blink/renderer/core/dom/events/event_dispatcher.cc:229:7
01603d07 blink::EventDispatcher::DispatchSimulatedClick(blink::Node&, blink::Event const*, blink::SimulatedClickCreationScope) ../../third_party/blink/renderer/core/dom/events/event_dispatcher.cc:143:8
0163d034 blink::Node::DispatchSimulatedClick(blink::Event const*, blink::SimulatedClickCreationScope) ../../third_party/blink/renderer/core/dom/node.cc:2969:3
019be4d1 blink::HTMLElement::click() ../../third_party/blink/renderer/core/html/html_element.cc:1110:3
00e13f63 blink::(anonymous namespace)::v8_html_element::ClickOperationCallback(v8::FunctionCallbackInfo<v8::Value> const&) gen/third_party/blink/renderer/bindings/modules/v8/v8_html_element.cc:3701:19
00d82e9c v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) ../../v8/src/api/api-arguments-inl.h:152:3
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Ionel Popescu (Gerrit)
Attention is currently required from: Dan Clark, Mason Freed.
Patch set 1:Code-Review +1
1 comment:
Patchset:
LGTM, thanks!
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Dan Clark.
Patch set 1:Code-Review +1
2 comments:
Patchset:
LGTM, but see the comment about the test/bug. Thanks!
File third_party/blink/web_tests/html/selectmenu/selectmenu-viewport.html:
Patch Set #1, Line 26:
setViewportEnabledI realize that this is likely why this test is a web_test and not a WPT. Is there any way to trigger the bug without this? If not, can you please add a comment in either the CL description or in the test itself explaining that?
And either way, can you please add a crbug link as a comment inside the test? The bug link in this CL just points back to the <selectmenu> implementation bug, so it'd be great to have a very quick bug filed with the context. (It sounds like this came from a clusterfuzz or something, so hopefully you just have a bug link handy!)
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Dan Clark (Gerrit)
Attention is currently required from: Dan Clark.
Dan Clark uploaded patch set #2 to this change.
[selectmenu] Remove re-entrant layout for selectmenu popup positioning
HTMLPopupElement::AdjustPopupPositionForSelectMenu uses the
LocalDOMWindow innerWidth/innerHeight getters, which can trigger a
relayout of the current document if viewport is enabled. Since we're
already doing layout at this point, this triggers a CHECK.
Update HTMLPopupElement::AdjustPopupPositionForSelectMenu to get the
size from the LocalFrameView directly such that the recursive
layout recalc is avoided.
The test included here is a web_test and not a WPT because the extra
same-document re-layout is only done when viewport is enabled, and
the desktop browser ignores <meta name="viewport">. So the test needs
to use internals.settings to enable viewport instead.
Bug: 1252600
Change-Id: I953822deb9349ff1fca822ced4e032c7a6a47c41
---
M third_party/blink/renderer/core/html/html_popup_element.cc
A third_party/blink/web_tests/html/selectmenu/selectmenu-viewport.html
2 files changed, 44 insertions(+), 2 deletions(-)
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Dan Clark (Gerrit)
Attention is currently required from: Dan Clark.
Dan Clark uploaded patch set #3 to this change.
[selectmenu] Remove re-entrant layout for selectmenu popup positioning
HTMLPopupElement::AdjustPopupPositionForSelectMenu uses the
LocalDOMWindow innerWidth/innerHeight getters, which can trigger a
relayout of the current document if viewport is enabled. Since we're
already doing layout at this point, this triggers a CHECK.
Update HTMLPopupElement::AdjustPopupPositionForSelectMenu to get the
size from the LocalFrameView directly such that the recursive
layout recalc is avoided.
The test included here is a web_test and not a WPT because the extra
same-document re-layout is only done when viewport is enabled, and
the desktop browser ignores <meta name="viewport">. So the test needs
to use internals.settings to enable viewport instead.
Bug: 1252600
Change-Id: I953822deb9349ff1fca822ced4e032c7a6a47c41
---
M third_party/blink/renderer/core/html/html_popup_element.cc
A third_party/blink/web_tests/html/selectmenu/selectmenu-viewport.html
2 files changed, 44 insertions(+), 2 deletions(-)
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Dan Clark (Gerrit)
2 comments:
Patchset:
Thanks for the reviews!
File third_party/blink/web_tests/html/selectmenu/selectmenu-viewport.html:
Patch Set #1, Line 26:
setViewportEnabled
I realize that this is likely why this test is a web_test and not a WPT. […]
The extra re-layout only happens when "viewport" (https://developer.mozilla.org/en-US/docs/Web/HTML/Viewport_meta_tag) is enabled (see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_dom_window.cc;l=1404), and <meta name="viewport"> is ignored on desktop browsers. So unfortunately I don't think there's a way to trigger this on desktop without internals.settings. I've added this context to a test comment and the CL description.
The issue was exposed in a CQ job (in an Android accessibility test) for a different change I was working on so I didn't initially have a bug, but I just filed crbug.com/1252600 and included it in the test comment.
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Dan Clark (Gerrit)
Patch set 4:Commit-Queue +2
Dan Clark (Gerrit)
Patch set 4:Commit-Queue +2
To view, visit change 3177072. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
1 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/web_tests/html/selectmenu/selectmenu-viewport.html
Insertions: 6, Deletions: 0.
@@ -23,6 +23,12 @@
promise_test(async () => {
if (window.internals) {
+ // Regression test for crbug.com/1252600
+ // On mobile, <meta name="viewport"> can trigger a layout pass when
+ // querying FrameView size. This test ensures that that this doesn't
+ // cause a re-entrant layout when opening the <selectmenu> popup.
+ // Since desktop browsers ignore <meta name="viewport">, set viewport
+ // using internals.settings.
internals.settings.setViewportEnabled(true);
internals.settings.setViewportMetaEnabled(true);
}
```
[selectmenu] Remove re-entrant layout for selectmenu popup positioning
HTMLPopupElement::AdjustPopupPositionForSelectMenu uses the
LocalDOMWindow innerWidth/innerHeight getters, which can trigger a
relayout of the current document if viewport is enabled. Since we're
already doing layout at this point, this triggers a CHECK.
Update HTMLPopupElement::AdjustPopupPositionForSelectMenu to get the
size from the LocalFrameView directly such that the recursive
layout recalc is avoided.
The test included here is a web_test and not a WPT because the extra
same-document re-layout is only done when viewport is enabled, and
the desktop browser ignores <meta name="viewport">. So the test needs
to use internals.settings to enable viewport instead.
Bug: 1252600
Change-Id: I953822deb9349ff1fca822ced4e032c7a6a47c41
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3177072
Commit-Queue: Dan Clark <dan...@microsoft.com>
Reviewed-by: Ionel Popescu <iopo...@microsoft.com>
Reviewed-by: Mason Freed <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#924798}
---
M third_party/blink/renderer/core/html/html_popup_element.cc
A third_party/blink/web_tests/html/selectmenu/selectmenu-viewport.html
2 files changed, 50 insertions(+), 2 deletions(-)