New issue
Advanced search Search tips

Issue 777672 link

Starred by 2 users

Issue metadata

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

Blocking:
issue 544140



Sign in to add a comment

[root layer scrolls] failure to promote child of uncomposited scroller which overlaps promoted content outside it

Project Member Reported by skobes@chromium.org, Oct 24 2017

Issue description

With RLS (all platforms), this test fails with an image diff caused by incorrect stacking of the blue and green boxes.

$ blink/tools/run_layout_tests.sh -t d --additional-driver-flag=--root-layer-scrolls compositing/overflow/nested-render-surfaces-with-intervening-clip.html
 
nested-render-surfaces-with-intervening-clip-diff.png
3.4 KB View Download

Comment 1 by bokan@chromium.org, Oct 24 2017

Status: Started (was: Assigned)
Related to compositing. The result is correct when I add --enable-prefer-compositing-to-lcd-text 

Comment 2 by bokan@chromium.org, Oct 24 2017

Ok, I've found the reason for the difference but not the solution yet.

The issue lies in the clipped/unclipped split of overlap map. Since the blue boxes are inside an uncomposited scroller, they always add their *clipped* bounding box. Without RLS, the frame isn't considered a composited scrolling ancestor layer so the position fixed element does too. But if we turn on RLS then suddenly the frame is a composited scrolling ancestor and so the position: fixed box adds its _unclipped_ bounding box. Since it's added first, the clipped bounding boxes following it won't check for overlap against the unclipped box and so they don't realize they need to composite for correct paint order.

I'm not yet sure what the correct solution is - I'll continue studying the code to understand better (I don't completely understand why unclipped can be compared with either type of rect but not vice versa) but if any of you know off the top of your head you could help expedite the process. Perhaps an exception for the root layer is appropriate? IIRC from diving through PaintLayerClipper we don't clip the root layer at all...

Comment 3 by skobes@chromium.org, Oct 25 2017

We do special-case the root layer for some position:fixed stuff, that might be the best thing here.

Comment 4 by skobes@chromium.org, Oct 26 2017

This might be the same root cause as  issue 776969 .
Here is a reduction of the overlap map bug that does not depend on root layer scrolling or position: fixed.

  https://output.jsbin.com/putulim/quiet

(On high DPI, pass --disable-prefer-compositing-to-lcd-text.)

I think a good fix would address this case as well, which probably means not special-casing position: fixed.
Summary: inside a composited scroller, overlap testing makes incorrect assumptions about clipping (was: [root layer scrolls] nested-render-surfaces-with-intervening-clip.html)

Comment 7 by bokan@chromium.org, Nov 7 2017

Thanks Steve, you're right (as usual), my analysis in the review was wrong.

So a stacking context inside a non-composited scroller A, nested in a composited scroller B, won't correctly overlap test a sibling that's in B. One of the assumptions we make in the overlap map is wrong but I'll need some time to study it more carefully and TPAC is quite the distraction. I'll take a deeper look by Thursday.

Comment 8 by skobes@chromium.org, Nov 13 2017

Summary: [root layer scrolls] failure to promote child of uncomposited scroller which overlaps promoted content outside it (was: inside a composited scroller, overlap testing makes incorrect assumptions about clipping)
Reducing scope of title.  General (non-RLS) bug now tracked by issue 783532.

Per offline discussion we should try to unblock RLS by preserving existing promotion behavior if possible - for example, by making CompositingRequirementsUpdater::RecursionData::has_composited_scrolling_ancestor_ ignore the root layer.  But this alone may not be sufficient due to RLS changing the behavior of PaintLayerClipper.

Comment 9 by skobes@chromium.org, Nov 17 2017

Adding notes from chat logs and such, for posterity.

Last week bokan reported that ignoring root layer for the purpose of setting has_composited_scrolling_ancestor_ doesn't fix offscreen overlap scenarios.  Our theory was that PaintLayerClipper applies viewport clip with RLS, so even if CompositingRequirementsUpdater matches pre-RLS behavior for picking between clipped vs. unclipped, it's getting different "clipped" rects from PaintLayerClipper.

During weekly sync meeting chrishtr mentioned PaintLayerClipper has options to control this stuff, which I had forgotten about.  Specifically the caller specifies a ClipRectsCacheSlot corresponding to a set of behaviors.  I then found that overlap testing uses PaintLayer::AncestorDependentCompositingInputs::clipped_absolute_bounding_box which is based on kAbsoluteClipRects, which is not used by anything else, and suggested that we change kAbsoluteClipRects to ignore the viewport clip, similar to how kRootRelativeClipRectsIgnoringViewportClip ignores the viewport clip.  That should let us use the pre-RLS rects for the overlap test.

Comment 10 by bokan@chromium.org, Nov 20 2017

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

commit 51eeec6a3fcdb833e81fa8cc73165613a997009d
Author: David Bokan <bokan@chromium.org>
Date: Mon Nov 20 16:09:55 2017

[root-layer-scrolling] Fix compositing overlap test

When RLS is turned on, children of the root layer now have a composited
scrolling ancestor. This causes them to use their unclipped bounding
rects to test overlap. It turns out overlap testing inside composited
scrollers is somewhat buggy (https://crbug.com/783532).

In order to prevent regressing existing behavior, this CL makes an
exception for children whose composited scrolling ancestor is the root
layer. In that that case, we use the clipped rects. This is necessary so
that overlap testing is performed against other clipped rects (see
comments in OverlapMap::Add for how clipped vs unclipped comparisons
work).

However, since the root layer is now a scrolling layer, this would
break overlap testing for boxes that are offscreen. This CL
additionally makes overlap test-related clipping ignore the root layer
so that the clipped_absolute_bounding_box for the root layer is actually
unclipped, as pre-RLS. The newly added test covers this case.

Bug: 783532

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_root_layer_scrolls;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I5e302f3f29775d829b94593e8053025c5da0f980
Reviewed-on: https://chromium-review.googlesource.com/742506
Reviewed-by: Steve Kobes <skobes@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517835}
[modify] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls
[add] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/LayoutTests/compositing/overflow/surface-over-composited-offscreen-expected.html
[add] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/LayoutTests/compositing/overflow/surface-over-composited-offscreen.html
[modify] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/Source/core/paint/ClipRectsCache.h
[modify] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[modify] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/Source/core/paint/PaintLayerClipper.h
[modify] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/Source/core/paint/compositing/CompositingInputsUpdater.cpp
[modify] https://crrev.com/51eeec6a3fcdb833e81fa8cc73165613a997009d/third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp

Comment 11 by bokan@chromium.org, Nov 20 2017

Status: Fixed (was: Started)

Sign in to add a comment