New issue
Advanced search Search tips

Issue 638386 link

Starred by 1 user
Status: Fixed
Owner:
wangxianzhu@chromium.org
Closed: Aug 2016
Cc:
pdr@chromium.org
chrishtr@chromium.org
vmp...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

CSS clipping of fixed-position descendants is broken in geometry mapping code

Project Member Reported by szager@chromium.org, Aug 16 2016 
Interestingly, this case already has a unit test:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp?rcl=0&l=866

In this case, the 'fixed' element is completely clipped out by the 'clip' element:

http://jsbin.com/buqizofocu/edit?html,output

... so it is expected that the geometry mapping code will return an empty rect when the overflow rect for the 'fixed' element is mapped up to the LayoutView.  But that's not what happens; the geometry mapping code does not apply the CSS clip.

This should be caught by the call to CHECK_EXACT_VISUAL_RECT in the test, but it's not because -- lo and behold -- both the new (GeometryMapper::mapToVisualRectInDestinationSpace) and old (LayoutObject::mapToVisualRectInAncestorSpace) geometry mapping code is broken in the same way.

chrishtr@ recently landed this attempt to fix the problem for LayoutObject::mapToVisualRectInAncestorSpace:

https://codereview.chromium.org/2241663002/

However, this misses the case when an an intermediate ancestor between a fixed-pos child and its container has a css clip; it only looks for css clip properties on the container:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBox.cpp?rcl=0&l=2174

GeometryMapper is broken because slowMapRectToDestinationSpace does not apply CSS clipping (i.e., it never calls localToAncestorClipRect).  That shouldn't be an issue when mapping from the 'fixed' element to the LayoutView, because the 'fixed' element is a descendant of the LayoutView; but due to weirdness in the construction of the PaintPropertyTree, the PropertyTreeState of the 'fixed' element is not a descendant of the PropertyTreeState of the LayoutView.  So, localToVisualRectInAncestorSpace fails, and slowMapRectToDestinationSpace generates the result.

/mic drop

Comment 1 by wangxianzhu@chromium.org, Aug 16 2016

Labels: -Pri-1 Pri-2
Thanks for the good catch.

However, this won't cause wrong rendering. We try best to apply all clippings but sometimes we omit some clippings if applying them is too hard. This may result non-optimal paint invalidation but should not cause wrong rendering.

For spv1 (mapToVisualRectInAncestorSpace) we support css clip for fixed- or absolute- positioned objects only if the clip is on the container. We won't fix this because this path will be discarded soon.

For spv2, for now we support css clip during mapping in the fast path only. We have recorded cssClipForFixedPosition() for the fixed-position object, and we should use for correct mapping.

Comment 2 by wangxianzhu@chromium.org, Aug 16 2016

Status: Assigned (was: Untriaged)

Comment 3 by szager@chromium.org, Aug 16 2016 

Two comments:

1) These "fuzzy" semantics may be appropriate for paint invalidation, but they are not appropriate for IntersectionObserver, which currently uses LayoutObject::mapToVisualRectInAncestorSpace.  I'm not sure if there are any other callers.

2) It's extremely misleading to have a test that checks for an *exact* match when you're using fuzzy semantics.  I stumbled across this because the test fails with root layer scrolling enabled -- with root layer scrolling, it *does* apply the css clip -- and I've spent two entire days tracking it down.  There needs to be a better way to test this; or maybe just don't use CHECK_EXACT_VISUAL_RECT for these kinds of cases.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 23 2016 

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

commit 7b7daf557bcc65228cb1431dc3f9f9b8547fd9c9
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Tue Aug 23 03:11:56 2016

Support css clip on fixed-position GeometryMapper

Previously in GeometryMapper::mapToVisualRectInDestinationSpace(), if
the fast path localToVisualRectToAncestorSpace() doesn't work because
the transform node of destinationSpace is not an ancestor of the
transform node of sourceState, we fallback to the slow path which
previously didn't support clip.

Now support clip if possible in the slow path.

BUG= 638386 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2268773002
Cr-Commit-Position: refs/heads/master@{#413652}

[modify] https://crrev.com/7b7daf557bcc65228cb1431dc3f9f9b8547fd9c9/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/7b7daf557bcc65228cb1431dc3f9f9b8547fd9c9/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
[modify] https://crrev.com/7b7daf557bcc65228cb1431dc3f9f9b8547fd9c9/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h
[modify] https://crrev.com/7b7daf557bcc65228cb1431dc3f9f9b8547fd9c9/third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp

Comment 5 by wangxianzhu@chromium.org, Aug 23 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment