Skip to content

GitLab

T
tangled
Commit a61bc12b authored by Peter Kasting's avatar Peter Kasting Committed by Commit Bot
Browse files
Options

Revert "[RootScroller] Allow main document horizontal overflow" 

This reverts commit ace3f1f7.

Reason for revert: Checking to see if this caused flaky scrolling failures on Mac 10.13 ( https://bugs.chromium.org/p/chromium/issues/detail?id=873435 )

Original change's description:
> [RootScroller] Allow main document horizontal overflow
> 
> When implicitly promoting an element to be the root scroller, we first
> check if the main document has any overflow. If it does, we err on the
> side of not promoting since it is likely the main document's content is
> important and meant to be scrollable.
> 
> This heuristic turns out to be too restrictive. For example, this makes
> it hard to build a root scrolling carousel. This CL loosens the
> restriction to allow horizontal overflow.
> 
> Bug: 838683
> Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I6007f6faa8e2006799a9dca6240b68ff0de7f09a
> Reviewed-on: https://chromium-review.googlesource.com/1165859
> Commit-Queue: David Bokan <bokan@chromium.org>
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581408}

TBR=bokan@chromium.org,dtapuska@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 838683
Change-Id: I14e6a0158bb8b06f2344c3b30b38b86c6eabd583
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1172004Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582439}
parent af6cc155
Hide whitespace changes
Inline Side-by-side
Showing with 6 additions and 81 deletions
third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
View file @ a61bc12b
......@@ -92,21 +92,6 @@ PaintLayerScrollableArea* GetScrollableArea(const Element& element) {
return ToLayoutBox(element.GetLayoutObject())->GetScrollableArea();
}
bool ScrollsVerticalOverflow(LayoutView& layout_view) {
DCHECK(layout_view.GetScrollableArea());
if (layout_view.Size().IsZero() ||
!layout_view.GetScrollableArea()->HasVerticalOverflow() ||
!layout_view.ScrollsOverflowY())
return false;
ScrollbarMode h_mode;
ScrollbarMode v_mode;
layout_view.CalculateScrollbarModes(h_mode, v_mode);
return v_mode != kScrollbarAlwaysOff;
}
} // namespace
// static
......@@ -389,9 +374,9 @@ void RootScrollerController::ProcessImplicitCandidates() {
if (!document_->GetLayoutView())
return;
// If the main document has vertical scrolling, that's a good sign we
// shouldn't implicitly promote anything.
if (ScrollsVerticalOverflow(*document_->GetLayoutView()))
// If the document has scrollable content, that's a good sign we shouldn't
// implicitly promote anything.
if (document_->GetLayoutView()->GetScrollableArea()->ScrollsOverflow())
return;
Element* highest_z_element = nullptr;
......
third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
View file @ a61bc12b
......@@ -2335,66 +2335,6 @@ TEST_F(ImplicitRootScrollerSimTest, PromotionChangesLayoutSize) {
<< "Once loaded, the iframe should be promoted.";
}
// Test that we don't promote any elements implicitly if the main document have
// vertical overflow.
TEST_F(ImplicitRootScrollerSimTest, OverflowInMainDocumentRestrictsImplicit) {
WebView().ResizeWithBrowserControls(IntSize(800, 600), 50, 0, true);
SimRequest main_request("https://example.com/test.html", "text/html");
SimRequest child_request("https://example.com/child.html", "text/html");
LoadURL("https://example.com/test.html");
main_request.Complete(R"HTML(
<!DOCTYPE html>
<style>
::-webkit-scrollbar {
width: 0px;
height: 0px;
}
body, html {
width: 100%;
height: 100%;
margin: 0px;
}
iframe {
width: 100%;
height: 100%;
border: 0;
}
div {
position: absolute;
left: 0;
top: 0;
height: 150%;
width: 150%;
}
</style>
<iframe id="container" src="child.html">
</iframe>
<div id="spacer"></div>
)HTML");
child_request.Complete(R"HTML(
<!DOCTYPE html>
<style>
body {
height: 1000px;
}
</style>
)HTML");
Compositor().BeginFrame();
EXPECT_EQ(GetDocument(),
GetDocument().GetRootScrollerController().EffectiveRootScroller())
<< "iframe shouldn't be promoted due to overflow in the main document.";
Element* spacer = GetDocument().getElementById("spacer");
spacer->style()->setProperty(&GetDocument(), "height", "100%", String(),
ASSERT_NO_EXCEPTION);
Compositor().BeginFrame();
EXPECT_EQ(GetDocument().getElementById("container"),
GetDocument().GetRootScrollerController().EffectiveRootScroller())
<< "Once vertical overflow is removed, the iframe should be promoted.";
}
class RootScrollerHitTest : public RootScrollerTest {
public:
void CheckHitTestAtBottomOfScreen() {
......
third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h
View file @ a61bc12b
......@@ -543,14 +543,14 @@ class CORE_EXPORT PaintLayerScrollableArea final
bool VisualViewportSuppliesScrollbars() const override;
bool HasHorizontalOverflow() const;
bool HasVerticalOverflow() const;
void Trace(blink::Visitor*) override;
private:
explicit PaintLayerScrollableArea(PaintLayer&);
bool HasHorizontalOverflow() const;
bool HasVerticalOverflow() const;
bool NeedsScrollbarReconstruction() const;
void ResetScrollOriginChanged() { scroll_origin_changed_ = false; }
......
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