Scroll clamping during layout can fail to invalidate compositing |
|||||||
Issue descriptionChrome Version: 67.0.3367.0/Canary OS: Android What steps will reproduce the problem? (1) Visit https://www.bogleheads.org/forum/viewtopic.php?f=10&t=240012&p=3753852#p3753852 (2) Scroll all the way to the bottom (3) Scroll up a little and notice content disappears. Steve and David, I think this is likely related to one of your recent patches, but I don't know which one.
,
Mar 12 2018
I'm able to repro in 67.0.3365.0 on a Nexus 6. Turning off root-layer-scrolls with --disable-features=RootLayerScrolling fixes the issue so it's related to RLS. I'll investigate...
,
Mar 12 2018
Missed that this doesn't repro in 66 branch (66.0.3359.14) so this is a recent, unmerged patch.
,
Mar 12 2018
Eh, I should really read the existing messages more closely. It is indeed the patch in #1 and Chris is better suited to look into it than me.
,
Mar 12 2018
Confirmed that reverting my patch fixes this issue.
,
Mar 12 2018
I think the bug is that the scroll offset due to the initial scroll due to a deep link with <a> tag gets things messed up.
,
Mar 12 2018
I think there may be a more fundamental bug with scroll offsets after navigation. Here's a repro for a scroll bug on Android: 1) Go to https://www.bogleheads.org 2) Click on a post that has a few comments (so you can scroll). 3) Scroll halfway down the page. 4) Press back, you should now be on https://www.bogleheads.org again. 5) Use the dots menu and navigate forward, you should now be scrolled halfway down a post. 6) Scroll up and down slowly, notice the scroll offset jumps to 0,0.
,
Mar 12 2018
I've split off comment #7 into https://crbug.com/821191 and asked for a bisect.
,
Mar 12 2018
Forcing off URL bar hiding causes the original bug here not to reproduce.
,
Mar 12 2018
What happens is that after scroll all the way to the bottom of the webpage, another attempt to scroll up will for some reason result in PLSA::UpdateScrollDimensions getting called, which then adds 25px of height for some reason. Then the next attempt to scroll will scroll into these 25px.
,
Mar 13 2018
I was able to fix the bug by restricting the fast path to a ScrolType of kCompositorScroll.
,
Mar 14 2018
Removing 'Needs-Bisect' label.
,
Mar 14 2018
The root cause is that CompositedLayerMapping::UpdateScrollingLayerGeometry
decides whether to update scroll offset based on whether it appears that
the parameters for the scroller have changed (overflow size, scroll offset in
PaintLayerScrollableArea, scroller size).
But those could have changed to another value and then changed back to the
current ones between compositing updates, via JS like this:
changeLayout();
element.offsetWidth; // Forces style and layout (but not compositing), including
// scroll anchoring adjustment. Scroll anchoring adjustment can
// change scroll offset in PLSA.
changeLayoutBackToPrevious();
element.offsetWidth; // Again forces style and layout, setting things back, but not necessarily
// adjusting scroll offset back (*)
(*) Another root cause could be that should be setting scroll offset correctly and it's not sometimes.
,
Mar 14 2018
If the scroll offset changes during the first element.offsetWidth, and doesn't change back during the second element.offsetWidth, why wouldn't CompositedLayerMapping see a different offset from the previous frame?
,
Mar 14 2018
Because PLSA is clamping the scroll back to the offset it was the last time CLM was run, upon PLSA resize. Digging now into the exact reason and what changed.
,
Mar 14 2018
Indeed, moving UpdateCompositingLayersAfterScroll() out of the if (!frame_view->IsInPerformLayout()) in PaintLayerScrollableArea::UpdateScrollOffset appears to fix the bug, because UpdateScrollOffset is indeed called during clamping, but in this situation the clamping is during layout.
,
Mar 14 2018
,
Mar 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d commit bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d Author: Chris Harrelson <chrishtr@chromium.org> Date: Thu Mar 15 02:24:30 2018 [RLS] Always update compositing layer offset and related dirty bits when scroll offset changes. Previously, it did not update update them during layout, which could result in failure to update composited scroll offset after situations like scroll clamping during synchronous forced layouts. Bug: 820839 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I129e7db4c5f4bc7b862b38de84ff174e7950fd5c Reviewed-on: https://chromium-review.googlesource.com/963517 Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#543287} [add] https://crrev.com/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d/third_party/WebKit/LayoutTests/compositing/scroll-update-with-clamp-expected.html [add] https://crrev.com/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d/third_party/WebKit/LayoutTests/compositing/scroll-update-with-clamp.html [modify] https://crrev.com/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
,
Mar 15 2018
,
Mar 19 2018
Issue 821699 has been merged into this issue. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by pnangunoori@chromium.org
, Mar 12 2018Labels: hasbisect-per-revision Target-67 RegressedIn-67 Triaged-Mobile M-67 Needs-triage-Mobile FoundIn-67
Owner: chrishtr@chromium.org
Status: Assigned (was: Untriaged)