New issue
Advanced search Search tips

Issue 771832 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 711468



Sign in to add a comment

[root layer scrolls] erroneous horizontal scrollbar when content is close to the window size

Project Member Reported by pdr@chromium.org, Oct 5 2017

Issue description

If the content is nearly as large to the window, but not quite, a horizontal scrollbar will erroneously be shown with --root-layer-scrolls. For example, open the following with a window size of 800x600 (the default for content shell):
---------------------------8<---------------------------
<style>
  body {
    margin: 0;
    background: blue;
    height: 199px;
    width: 799px;
  }
</style>
---------------------------8<---------------------------

This is the root cause of the failure on svg/as-background-image/svg-as-background-1.html and likely many other tests.
 
Ick.  PLSA needs to get a little smarter about removing scrollbars.  This has been reported before in the context of overflow scrollers, e.g. issue 625300.  Note that we start with a vertical scrollbar for performance reasons ( issue 711137 ).
Blocking: -490942 417782
Blocking: -417782 711468

Comment 4 by pdr@chromium.org, Oct 5 2017

Components: -Blink>Scroll Blink>Layout
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f07c3a528cd94b0d171e335b7fcc11287f49550

commit 9f07c3a528cd94b0d171e335b7fcc11287f49550
Author: Philip Rogers <pdr@chromium.org>
Date: Tue Oct 10 17:05:24 2017

Add "attempt_to_remove_scrollbars" for root layer scrolling.

LocalFrameView has a codepath for removing auto scrollbars when there
is no overflow on the first pass without scrollbars. This patch implements
the same logic for root layer scrolling in PLSA.

We may want to extend this logic to work with all scrollbars, and
doing so may fix crbug.com/625300, but this is a larger change.

BUG:  771832 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I6e7126267683ecb037f6fa47d196d5d9aa7f2514
Reviewed-on: https://chromium-review.googlesource.com/706159
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507697}
[modify] https://crrev.com/9f07c3a528cd94b0d171e335b7fcc11287f49550/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls
[modify] https://crrev.com/9f07c3a528cd94b0d171e335b7fcc11287f49550/third_party/WebKit/Source/core/layout/ScrollbarsTest.cpp
[modify] https://crrev.com/9f07c3a528cd94b0d171e335b7fcc11287f49550/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp

Comment 6 by pdr@chromium.org, Oct 10 2017

Status: Fixed (was: Started)

Comment 7 by szager@chromium.org, Oct 13 2017

Status: Assigned (was: Fixed)
More probable failures:

compositing/gestures/gesture-tapHighlight-1-overflow-div-layout-change-2.html
compositing/gestures/gesture-tapHighlight-1-overflow-div-layout-change.html
compositing/gestures/gesture-tapHighlight-1-overflow-div-scrolled-late-composite.html
compositing/gestures/gesture-tapHighlight-1-overflow-div-scrolled.html
compositing/gestures/gesture-tapHighlight-1-overflow-div.html
compositing/gestures/gesture-tapHighlight-2-iframe-composited-inner.html
compositing/gestures/gesture-tapHighlight-2-iframe-composited-outer.html
compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-inner-late-composite.html
compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-inner.html
compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-outer-late-composite.html
compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-outer.html
compositing/gestures/gesture-tapHighlight-2-iframe.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-composited-inner-scroll-inner.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-composited-inner-scroll-outer.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-composited-inner.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-composited-outer-scroll-inner.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-composited-outer-scroll-outer.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-composited-outer.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-scrolled-inner.html
compositing/gestures/gesture-tapHighlight-2-overflow-div-scrolled-outer.html
compositing/gestures/gesture-tapHighlight-2-overflow-div.html
compositing/gestures/gesture-tapHighlight-on-promoted-overflow-div-scrolled.html
compositing/gestures/gesture-tapHighlight-simple-scaledX.html
compositing/gestures/gesture-tapHighlight-simple-scaledY.html

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/87903183a34ddc29250575aa10e0685f1cc54416

commit 87903183a34ddc29250575aa10e0685f1cc54416
Author: Philip Rogers <pdr@chromium.org>
Date: Wed Oct 18 19:37:57 2017

[root layer scrolls] Do additional layout for initial vertical scrollbar

Overflow scrollbars require two layouts (one regular layout + one
"overflow" relayout) to calculate scrollbars when starting from no
scrollbars. The LayoutView has an optimization to use an initial
vertical scrollbar because vertical document scrolling is the common
case. When this is incorrect, and all scrollbars are 'auto', an
additional third relayout is necessary.

For example, a div with width 101% and height 10px will go through:
1) Initial layout with vertical scrollbar. 101% width with a vertical
scrollbar will be (800-15)*101% = ~793px wide. The height will be 10px.
2) Because the div (793px by 10px) fully fits in the window, scrollbars
are removed. This logic was added in [1]. A second layout is performed
which results in width 800*101%=808px, and height 10px.
3) Due to horizontal overflow, a horizontal scrollbar is added and a
third layout is performed which results in width 808px and height 10px
with just a horizontal scrollbar.

The third layout is necessary for correct scrollbars. This logic matches
the third layout logic in LocalFrameView.

This patch fixes 25 layout tests with root layer scrolling enabled, though
24 of those were failing at head, possibly regressing in [1].

[1] https://chromium.googlesource.com/chromium/src/+/9f07c3a528cd94b0d171e335b7fcc11287f49550

Bug:  771832 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I88c9e9c7dcb6af1d51d11b77c1f2bc14f0bc98f6
Reviewed-on: https://chromium-review.googlesource.com/720065
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509840}
[modify] https://crrev.com/87903183a34ddc29250575aa10e0685f1cc54416/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls
[modify] https://crrev.com/87903183a34ddc29250575aa10e0685f1cc54416/third_party/WebKit/Source/core/layout/ScrollbarsTest.cpp
[modify] https://crrev.com/87903183a34ddc29250575aa10e0685f1cc54416/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/87903183a34ddc29250575aa10e0685f1cc54416/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h

Comment 9 by pdr@chromium.org, Oct 18 2017

Status: Fixed (was: Assigned)

Sign in to add a comment