[root layer scrolls] create vertical scrollbar by default |
||||||
Issue descriptionFrameView 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?
,
Apr 13 2017
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?
,
Apr 13 2017
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.
,
Apr 13 2017
SGTM. @cbiesinger do you have bandwidth for this one?
,
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).
,
Jun 1 2017
,
Aug 24 2017
,
Aug 29 2017
@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.
,
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.
,
Sep 1 2017
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);
}
}
,
Sep 4 2017
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.
,
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
,
Sep 8 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by szager@chromium.org
, Apr 13 2017