Chromium Code Reviews Chromium Code Reviews
Issue 2780883002:
Correct overlay scrollbar check FrameView::scrollbarExistenceDidChange (Closed)
| Index: third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp |
| diff --git a/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp b/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp |
| index eee9d25621738eb0c628ed40bede8ff32fd0bc23..21240406fd5d8ffa1e36e2ca04c4801e93ef68ec 100644 |
| --- a/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp |
| +++ b/third_party/WebKit/Source/web/tests/ScrollbarsTest.cpp |
| @@ -2,6 +2,7 @@ |
| // Use of this source code is governed by a BSD-style license that can be |
| // found in the LICENSE file. |
| +#include "core/frame/FrameView.h" |
| #include "core/layout/LayoutView.h" |
| #include "core/paint/PaintLayerScrollableArea.h" |
| #include "platform/testing/RuntimeEnabledFeaturesTestHelpers.h" |
| @@ -42,6 +43,65 @@ TEST_F(ScrollbarsTest, DocumentStyleRecalcPreservesScrollbars) { |
| ASSERT_TRUE(plsa->verticalScrollbar() && plsa->horizontalScrollbar()); |
| } |
| +// Ensure that causing a change in scrollbar existence causes a nested layout |
| +// to recalculate the existence of the opposite scrollbar. The bug here was |
| +// caused by trying to avoid the layout when overlays are enabled but not |
| +// checking whether the scrollbars should be custom - which do take up layout |
| +// space. https://crbug.com/668387. |
| +TEST_F(ScrollbarsTest, CustomScrollbarsCauseLayoutOnExistenceChange) { |
| + // The bug reproduces only with RLS off. When RLS ships we can keep the test |
| + // but remove this setting. |
| + ScopedRootLayerScrollingForTest turnOffRootLayerScrolling(false); |
| + |
| + // This test is specifically checking the behavior when overlay scrollbars |
| + // are enabled. |
| + DCHECK(ScrollbarTheme::theme().usesOverlayScrollbars()); |
|
skobes
2017/03/28 21:22:34
Doesn't this DCHECK make the test crash on platfor
bokan
2017/03/28 21:26:32
No, unit tests run with mock overlay scrollbars en
|
| + |
| + webView().resize(WebSize(800, 600)); |
| + SimRequest request("https://example.com/test.html", "text/html"); |
| + loadURL("https://example.com/test.html"); |
| + request.complete( |
| + "<!DOCTYPE html>" |
| + "<style>" |
| + " ::-webkit-scrollbar {" |
| + " height: 16px;" |
| + " width: 16px" |
| + " }" |
| + " ::-webkit-scrollbar-thumb {" |
| + " background-color: rgba(0,0,0,.2);" |
| + " }" |
| + " html, body{" |
| + " margin: 0;" |
| + " height: 100%;" |
| + " }" |
| + " .box {" |
| + " width: 100%;" |
| + " height: 100%;" |
| + " }" |
| + " .transformed {" |
| + " transform: translateY(100px);" |
| + " }" |
| + "</style>" |
| + "<div id='box' class='box'></div>"); |
| + |
| + ScrollableArea* layoutViewport = |
| + document().view()->layoutViewportScrollableArea(); |
| + |
| + compositor().beginFrame(); |
| + ASSERT_FALSE(layoutViewport->verticalScrollbar()); |
| + ASSERT_FALSE(layoutViewport->horizontalScrollbar()); |
| + |
| + // Adding translation will cause a vertical scrollbar to appear but not dirty |
| + // layout otherwise. Ensure the change of scrollbar causes a layout to |
| + // recalculate the page width with the vertical scrollbar added. |
| + mainFrame().executeScript(WebScriptSource( |
| + "document.getElementById('box').className = 'box transformed';")); |
| + compositor().beginFrame(); |
| + |
| + ASSERT_TRUE(layoutViewport->verticalScrollbar()); |
| + ASSERT_FALSE(layoutViewport->horizontalScrollbar()); |
| +} |
| + |
| } // namespace |
| } // namespace blink |
