New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 711137 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 417782



Sign in to add a comment

[root layer scrolls] create vertical scrollbar by default

Project Member Reported by skobes@chromium.org, Apr 13 2017

Issue description

FrameView starts with a vertical scrollbar, but PLSA doesn't.

That means root layer scrolling ~doubles the cost of the initial layout in the common case of vertical overflow, since we have to layout again when the scrollbar is added.

The root PLSA should start with a vertical scrollbar.

Maybe all PLSA's should start with a vertical scrollbar?
 

Comment 1 by szager@chromium.org, Apr 13 2017

I don't think all PLSA's should start with a vertical scrollbar.  Because the auto-scrollbar logic is not memoryless (crbug.com/625300), starting with a vertical scrollbar will inevitably cause some behavioral differences in corner cases.

Comment 2 by skobes@chromium.org, Apr 13 2017

Cc: ikilpatrick@chromium.org esprehn@chromium.org
But those corner cases are buggy already.  The proper fix for that is aborting on overflow detection which I believe is planned for LayoutNG.  Until then there is likely a perf win from starting with a scrollbar.  Scrollers with more content are more expensive to layout, so it seems better to incur double-layout in the non-overflowing case.

@esprehn do you have any thoughts about this?
It makes sense to me that the viewport would start with a scrollbar to avoid a double layout since most pages do actually scroll. Common pages that don't scroll are things that are "apps", and they usually do overflow: hidden; on the html element so we should be able to optimize that case.

I agree with skobes@ that the double layout for smaller content makes more sense than the double layout for huge content, though I don't know how many pages are putting overflow: auto randomly on lots of the page that doesn't scroll where we'd potentially make the layouts much more expensive by starting with a scrollbar. Doing some sampling of sites looking at Gmail, Facebook, G+, etc. with:

Array.from(document.querySelectorAll("*")).filter((e) => getComputedStyle(e).overflowY == 'auto')

it looks like it's mostly menus and dialogs where you want a scrollbar only if needed. The menu case doesn't seem too bad, but doing a double layout on every popup dialog would be unfortunate (but not sure how bad that is?)

Could we make PLSA only start with a scrollbar when the Layer is isRootLayer() == true for now? That would preserve the existing behavior and let us ship, and then we could decide one way or the other later.

I'll have to think more about the trade offs and do more research into where sites would end up with the double layout if we decided to always start with or without one and how bad it'd be.

Comment 4 by skobes@chromium.org, Apr 13 2017

Owner: cbiesin...@chromium.org
Status: Assigned (was: Available)
SGTM.

@cbiesinger do you have bandwidth for this one?

Comment 5 by szager@chromium.org, Apr 13 2017

Just my additional $.02: my chief concern is in the bugs that would get filed if we changed the default behavior, rather than the performance implication.  For that reason, I agree with esprehn (and myself in comment #1).
Cc: -esprehn@chromium.org
Owner: skobes@chromium.org

Comment 7 by skobes@chromium.org, Aug 24 2017

Cc: skobes@chromium.org sriram...@samsung.com satay...@samsung.com
Owner: ----
Status: Available (was: Assigned)
@skobes,
I am trying to understand what to do here.
From my initial study, the frameview bydefault starts with vertical scrollbar in UpdateLayout function if it is the first_layout_ pass.
And the scrollbars are created in UpdateScrollbars.
And incase of RLS the scrollbars are computed in PLSA::UpdateAfterLayout.

Any pointers or initial thoughts on what exactly we have to do here would help me, i will continue my study meanwhile.

Comment 9 by skobes@chromium.org, Aug 29 2017

The task here is to make the root PLSA start with a vertical scrollbar, when RLS is enabled.  This is so that the LayoutView will only go through layout once instead of twice, on a page with vertical overflow.
This might be as simple as making this change to FrameView::UpdateLayout :

 if (first_layout_) {
   ...
   if (v_mode == kScrollbarAuto) {
     SetVerticalScrollbarMode(kScrollbarAlwaysOn);
+    if (RuntimeEnabledFeatures::RootLayerScrollingEnabled())
+      GetLayoutView()->GetScrollableArea()->SetHasVerticalScrollbar(true);
   }
   if (h_mode == kScrollbarAuto) {
     SetHorizontalScrollbarMode(kScrollbarAlwaysOff);
+    if (RuntimeEnabledFeatures::RootLayerScrollingEnabled())
+      GetLayoutView()->GetScrollableArea()->SetHasHorizontalScrollbar(false);
   }
 }
Thanks for the patch. Submitted changes at https://chromium-review.googlesource.com/c/chromium/src/+/647515
There are few failures. I will check them tomorrow.
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 7 2017

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

commit e3f1cce3d4d5e89a2cc8ff864217fc66e4a4a96a
Author: srirama <srirama.m@samsung.com>
Date: Thu Sep 07 20:20:51 2017

[RLS]Create vertical scrollbar bydefault

FrameView starts with a vertical scrollbar, but PLSA doesn't.
That means root layer scrolling ~doubles the cost of the
initial layout in the common case of vertical overflow,
since we have to layout again when the scrollbar is added.
Making root PLSA start with a vertical scrollbar bydefault.

Bug:  711137 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I162c8b0ee647a4452346f326830746ba6e6958db
Reviewed-on: https://chromium-review.googlesource.com/647515
Commit-Queue: srirama chandra sekhar <srirama.m@samsung.com>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500367}
[modify] https://crrev.com/e3f1cce3d4d5e89a2cc8ff864217fc66e4a4a96a/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/e3f1cce3d4d5e89a2cc8ff864217fc66e4a4a96a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h

Status: Fixed (was: Available)

Sign in to add a comment