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

Issue 754927 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

[Blink] Non-composited clips are not applied correctly when an element escapes clips

Project Member Reported by trchen@chromium.org, Aug 12 2017

Issue description

Chrome Version: ToT
OS: All

What steps will reproduce the problem?
(1) blink/tools/run_layout_tests.sh compositing/overflow/fixed-pos-escape-clip-and-apply-noncomposited-clip.html

What is the expected result? Test should pass.
What happens instead? Test failed.

The aforementioned test is not submitted at time of reporting. See attachment for the test.

The bug is due to that an element has to escape some clip, but then its true clip parent was not composited thus its composited layer clip parent points to the true clip parent's enclosing composited layer. In this case an ancestor clipping layer should be created.

This bug was discovered during a code review (https://chromium-review.googlesource.com/c/609321/6/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp#2462), and also from investigating another clip-escaping bug ( crbug.com/752182 ).

Note: SPv2 is immune to this bug!
 
fixed-pos-escape-clip-and-apply-noncomposited-clip.html
728 bytes View Download
fixed-pos-escape-clip-and-apply-noncomposited-clip-expected.html
287 bytes View Download
Labels: BugSource-Team PaintTeamTriaged-20170814 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Available (was: Untriaged)
Not sure if chrishtr@ or myself will get to this first.
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 15 2017

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

commit a7482a88d73040a616ef53294eeef75ca8b6c5a2
Author: Tien-Ren Chen <trchen@chromium.org>
Date: Tue Aug 15 23:58:04 2017

[Blink] Escape clip even if true clip container is not stacking context

CompositingInputsUpdater.cpp:HasClippedStackingAncestor reported false
negative when the true clipping container is not stacking context,
because the layer->CompositingContainer() chain will never hit a
non-stacking context, thus fell through and returned false.

This CL made it return early as soon as an escaped clip was found. As a
side change, also early return when compositing container chain already
ran past the true clipping container, because no possible escaped clip
can be found at this point.

BUG= 752182 , 754927 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I3cb1aca53e38aa4e4cd9f6c7c0a1587a11eb7c75
Reviewed-on: https://chromium-review.googlesource.com/612584
Commit-Queue: Tien-Ren Chen <trchen@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494615}
[modify] https://crrev.com/a7482a88d73040a616ef53294eeef75ca8b6c5a2/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/a7482a88d73040a616ef53294eeef75ca8b6c5a2/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/a7482a88d73040a616ef53294eeef75ca8b6c5a2/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-and-apply-noncomposited-clip-expected.html
[add] https://crrev.com/a7482a88d73040a616ef53294eeef75ca8b6c5a2/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-and-apply-noncomposited-clip.html
[add] https://crrev.com/a7482a88d73040a616ef53294eeef75ca8b6c5a2/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-having-non-stacking-context-clipping-container-expected.html
[add] https://crrev.com/a7482a88d73040a616ef53294eeef75ca8b6c5a2/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-having-non-stacking-context-clipping-container.html
[modify] https://crrev.com/a7482a88d73040a616ef53294eeef75ca8b6c5a2/third_party/WebKit/Source/core/paint/compositing/CompositingInputsUpdater.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 16 2017

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

commit 7f3d3dc4e5fdb09d8e53af63d97cb91d35fd4f32
Author: Tien-Ren Chen <trchen@chromium.org>
Date: Wed Aug 16 23:13:20 2017

Revert "[Blink] Escape clip even if true clip container is not stacking context"

This reverts commit a7482a88d73040a616ef53294eeef75ca8b6c5a2.

Reason for revert:  https://crbug.com/755850 

Original change's description:
> [Blink] Escape clip even if true clip container is not stacking context
> 
> CompositingInputsUpdater.cpp:HasClippedStackingAncestor reported false
> negative when the true clipping container is not stacking context,
> because the layer->CompositingContainer() chain will never hit a
> non-stacking context, thus fell through and returned false.
> 
> This CL made it return early as soon as an escaped clip was found. As a
> side change, also early return when compositing container chain already
> ran past the true clipping container, because no possible escaped clip
> can be found at this point.
> 
> BUG= 752182 , 754927 
> 
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: I3cb1aca53e38aa4e4cd9f6c7c0a1587a11eb7c75
> Reviewed-on: https://chromium-review.googlesource.com/612584
> Commit-Queue: Tien-Ren Chen <trchen@chromium.org>
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#494615}

TBR=trchen@chromium.org,chrishtr@chromium.org

Change-Id: Ibf42da5bdbbeec47ab75692f846e8c5e2a11e5d0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  752182 ,  754927 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/618102
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Commit-Queue: Tien-Ren Chen <trchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494992}
[modify] https://crrev.com/7f3d3dc4e5fdb09d8e53af63d97cb91d35fd4f32/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/7f3d3dc4e5fdb09d8e53af63d97cb91d35fd4f32/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/ed801e3b65bd6fb79017603ecd720c7c1413fef4/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-and-apply-noncomposited-clip-expected.html
[delete] https://crrev.com/ed801e3b65bd6fb79017603ecd720c7c1413fef4/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-and-apply-noncomposited-clip.html
[delete] https://crrev.com/ed801e3b65bd6fb79017603ecd720c7c1413fef4/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-having-non-stacking-context-clipping-container-expected.html
[delete] https://crrev.com/ed801e3b65bd6fb79017603ecd720c7c1413fef4/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-having-non-stacking-context-clipping-container.html
[modify] https://crrev.com/7f3d3dc4e5fdb09d8e53af63d97cb91d35fd4f32/third_party/WebKit/Source/core/paint/compositing/CompositingInputsUpdater.cpp

Comment 4 by trchen@chromium.org, Aug 16 2017

Cc: krajshree@chromium.org kkaluri@chromium.org trchen@chromium.org brajkumar@chromium.org ajha@chromium.org
 Issue 755850  has been merged into this issue.
trchen: do you know what the problem in your CL was that caused the revert?

Comment 6 by trchen@chromium.org, Aug 21 2017

Yes, it is a series of bugs related to clip parent. Hunting them down was a traumatizing experience, but I think I fixed all of them. The CL is growing a bit big though. Let's go over it today.
Owner: trchen@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 26 2017

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

commit 09dd9e06d4c19f4702597e8563d9b956e4dac9ae
Author: Tien-Ren Chen <trchen@chromium.org>
Date: Sat Aug 26 00:43:03 2017

[Blink] Fixing a series of composited clip bug

This CL fixes a number of issues around composited clip escaping.

1. Layers wouldn't to set clip_parent correctly if its clipping container is not a
stacking context. For example:

<div style="position:relative; overflow:hidden;">
  <div style="will-change:opacity; overflow:hidden;">
    <div style="position:absolute;"></div>
  </div>
</div>

The innermost element should be only affected by the outer clip, but not the clip in
the middle. This was due to a bug in
CompositingInputsUpdater.cpp:HasClippedStackingAncestor that the loop iterates over
CompositingContainer would fall through if the clipping container is not in the chain.

2. Layers with clip parent do not have ancestor clipping layer computed correctly.

Using the same example above, if the innermost element has clip_parent correctly set
to the outer clip, it would still not have the correct clip applied. It is because
the outer clip does not get composited, thus its enclosing composited layer (root
layer in this case) will be used as clip_parent instead. The other clip therefore
is a non-composited clip, and must be applied by ancestor clipping layer. Previously
CompositedLayerMapping::OwningLayerClippedOrMaskedByLayerNotAboveCompositedAncestor()
and CompositedLayerMapping::UpdateAncestorClippingLayerGeometry() computes only the
clips between the compositing ancestor and the layer. The clips between the clip
parent and the layer should be computed instead.

3. With both above issues fixed, layers might sometimes need to compute ancestor clip
across transforms. This caused problem because PaintLayerClipper doesn't treat
transforms as fixed-pos container, nor handle coordinate conversion in any way.
For example:
<div style="overflow:hidden;">
  <div style="transform:rotate(30deg);">
    <div style="will-change:opacity; overflow:hidden;">
      <div style="position:absolute;"></div>
    </div>
  </div>
</div>

The fixed-pos element set its clip_parent to the outer clip. Again, the clip was
not composited, thus the root layer is used as the clip parent and the clip will
be applied by ancestor clipping layer. However PaintLayerClipper doesn't compute
clip correctly in this case. It is impossible to, because the clip is not even in
a space axis-aligned to the compositing container. This can be fixed by setting
clip_parent to the nearest containing block parent (the transformed element in
this case) instead. In this example, the transformed element will be composited
for descendant reason, and the innermost element can inherit its clip using clip
parent.

4. Out-of-flow layers escape clip unnecessarily. For example:
<div style="will-change:opacity; overflow:hidden;">
  <div style="position:absolute; z-index:0;">
    <div style="position:absolute;"></div>
  </div>
</div>
The innermost element will have clip_parent set, but is not necessary because it
inherits clip state from its parent stacking context. This is due to
CompositingInputsUpdater.cpp:HasClippedStackingAncestor looping over all ancestor
stacking context to see if any stacking context clip needs to be escaped. Instead,
we only need to check if we need to escape clip applied to the direct parent
stacking context, because by induction if our parent is in its target clip state
and we don't have to escape anything in it, we would have inherited the correct
state from the parent.

BUG= 752182 , 754927 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ib36dfb7e45a494fd609df4dd3a239233ff763d8e
Reviewed-on: https://chromium-review.googlesource.com/624615
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Tien-Ren Chen <trchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497613}
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/cc/layers/layer.h
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/clip-parent-across-transform-boundary-expected.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/clip-parent-across-transform-boundary.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/escape-clip-and-paint-before-clip-parent-expected.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/escape-clip-and-paint-before-clip-parent.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-and-apply-noncomposited-clip-expected.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-and-apply-noncomposited-clip.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-having-non-stacking-context-clipping-container-expected.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/fixed-pos-escape-clip-having-non-stacking-context-clipping-container.html
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/no-excessive-clip-parent-if-parent-escaped-expected.png
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/no-excessive-clip-parent-if-parent-escaped-expected.txt
[add] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/no-excessive-clip-parent-if-parent-escaped.html
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/compositing/overflow/scroll-parent-absolute-with-backdrop-filter-expected.txt
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/LayoutTests/virtual/prefer_compositing_to_lcd_text/compositing/overflow/scroll-parent-absolute-with-backdrop-filter-expected.txt
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.h
[modify] https://crrev.com/09dd9e06d4c19f4702597e8563d9b956e4dac9ae/third_party/WebKit/Source/core/paint/compositing/CompositingInputsUpdater.cpp

Comment 9 by trchen@chromium.org, Aug 26 2017

Status: Fixed (was: Assigned)

Sign in to add a comment