Skip to content

GitLab

T
tangled
Commit ac816f8f authored by David Bokan's avatar David Bokan Committed by Commit Bot
Browse files
Options

Fix root scroller crash 

Root scroller assumes that if a LayoutBox is scrollable it must have an
associated Node. However, there are some edge cases where an anonymous
LayoutBox is created, such as for overflowing <input> and <fieldset>
controls.

To be strictly correct, we could make root scroller work in these cases
but it's not likely to be useful in any use cases. It'd also be
non-trivial work to fix root scroller machinery - for example, "is root
scroller" bits are currently cached on the Node's LayoutBox rather than
on the LayoutBoxForScrolling. Instead, we'll simply ignore these cases
to avoid crashing.

Bug: 1125621
Change-Id: I92880f4430aaa4a4b67d9bdf1ebed2e8dada7d9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411396
Auto-Submit: David Bokan <bokan@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: default avatarKent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807262}
parent ecea9496
Hide whitespace changes
Inline Side-by-side
Showing with 62 additions and 6 deletions
third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
View file @ ac816f8f
...@@ -90,8 +90,10 @@ PaintLayerScrollableArea* GetScrollableArea(const Element& element) { ...@@ -90,8 +90,10 @@ PaintLayerScrollableArea* GetScrollableArea(const Element& element) {
return frame_view->LayoutViewport(); return frame_view->LayoutViewport();
} }
DCHECK(element.GetLayoutObject()->IsBox()); if (!element.GetLayoutBoxForScrolling())
return ToLayoutBox(element.GetLayoutObject())->GetScrollableArea(); return nullptr;
return element.GetLayoutBoxForScrolling()->GetScrollableArea();
} }
} // namespace } // namespace
......
third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
View file @ ac816f8f
...@@ -2749,6 +2749,54 @@ TEST_F(ImplicitRootScrollerSimTest, AppliedAtFractionalZoom) { ...@@ -2749,6 +2749,54 @@ TEST_F(ImplicitRootScrollerSimTest, AppliedAtFractionalZoom) {
<< "<iframe> should remain promoted when URL bar is hidden"; << "<iframe> should remain promoted when URL bar is hidden";
} }
// Ensure that a scrollable fieldset doesn't get promoted to root scroller.
// With FieldsetNG, a scrollable fieldset creates an anonymous LayoutBox that
// doesn't have an associated Node. RootScroller is premised on the fact that a
// scroller is associated with a Node. It'd be non-trivial work to make this
// work without a clear benefit so for now ensure it doesn't get promoted and
// doesn't cause any crashes. https://crbug.com/1125621.
TEST_F(ImplicitRootScrollerSimTest, FieldsetNGCantBeRootScroller) {
// This test is specifically ensuring we avoid crashing with the LayoutNG
// version of fieldset since it uses an anonymous LayoutBox for scrolling.
if (!RuntimeEnabledFeatures::LayoutNGEnabled())
return;
WebView().MainFrameWidget()->Resize(WebSize(800, 600));
SimRequest request("https://example.com/test.html", "text/html");
LoadURL("https://example.com/test.html");
request.Complete(R"HTML(
<!DOCTYPE html>
<style>
::-webkit-scrollbar {
width: 0px;
height: 0px;
}
body, html {
width: 100%;
height: 100%;
margin: 0px;
}
fieldset {
width: 100%;
height: 100%;
overflow: scroll;
border: 0;
margin: 0;
padding: 0;
}
div {
height: 200%;
}
</style>
<fieldset>
<div></div>
</fieldset>
)HTML");
Compositor().BeginFrame();
EXPECT_TRUE(GetDocument().GetLayoutView()->IsEffectiveRootScroller());
}
class RootScrollerHitTest : public ImplicitRootScrollerSimTest { class RootScrollerHitTest : public ImplicitRootScrollerSimTest {
public: public:
void CheckHitTestAtBottomOfScreen(Element* target) { void CheckHitTestAtBottomOfScreen(Element* target) {
......
third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
View file @ ac816f8f
...@@ -2372,10 +2372,16 @@ void PaintLayerScrollableArea::UpdateScrollableAreaSet() { ...@@ -2372,10 +2372,16 @@ void PaintLayerScrollableArea::UpdateScrollableAreaSet() {
*owner); *owner);
} }
} else { } else {
GetLayoutBox() // In some cases, the LayoutBox may not be associated with a Node (e.g.
->GetDocument() // <input> and <fieldset> can generate anonymous LayoutBoxes for their
.GetRootScrollerController() // scrollers). We don't care about those cases for root scroller so
.ConsiderForImplicit(*GetLayoutBox()->GetNode()); // simply avoid these. https://crbug.com/1125621.
if (GetLayoutBox()->GetNode()) {
GetLayoutBox()
->GetDocument()
.GetRootScrollerController()
.ConsiderForImplicit(*GetLayoutBox()->GetNode());
}
} }
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment