Bug 76486 – RenderLayer ClipRect not accounting for transforms

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

A reduced test case is attached.  It reproduces on both Chromium tip-of-tree and Webkit nightly.  On chromium, this bug is the cause of http://code.google.com/p/chromium/issues/detail?id=102943 but that maps bug does not reproduce on Safari.

Explanation: the ClipRect computed by the backgroundClipRect() does not include transforms.  If the layer is translated far enough, the intersection between layerbounds and cliprect is empty, and an empty rect is stored in the overlapMap.  As a result, the layer on top does not realize it should be composited, because it thinks it doesn't overlap anything.

Simon, can you please suggest the "correct" way to fix this is?  The fixes I've tried so far break other layout tests, and I'm probably just missing some simple detail about clipRect usage.   Thanks in advance!

[Shawn Singh]

Created attachment 122826 [details]
Reduced test case

[Simon Fraser (smfr)]

Can you make a testcase that fails more obviously? I just see a green box in the attachment.

I not sure I get the issue. I would expect that backgroundClipRect() is never used "across" transform boundaries.

[Shawn Singh]

(In reply to comment #2)
> Can you make a testcase that fails more obviously? I just see a green box in the attachment.
> 
> I not sure I get the issue. I would expect that backgroundClipRect() is never used "across" transform boundaries.

Thanks for the quick reply =)    If you're seeing green, then it passed; I just coded it up as a layout test.  I'm able to reproduce this on today's WebKit nightly on a mac 10.6.

The way the code is now,  backgroundClipRect() (and convertToLayerCoords()) can indeed be used with a transform in-between... specifically this can happen in RenderLayerCompositor::addToOverlapMap(), where we convert between the local and root-layer space.

If that shouldn't happen, then what would be the correct way to transform from local-to-absolute space here?  I tried replacing convertToLayerCoords() with localToAbsolute(...)  in RenderLayer::calculateClipRects(), but I don't think that's correct.

I can enumerate exact lines of code and what is happening, if you wanted to hear more details.

[Shawn Singh]

I was able to bisect this issue down to revision 91114 - https://bugs.webkit.org/show_bug.cgi?id=63493

This validates the debugging I've done, but doesn't add much information to the problem.  At least seeing this change should help clarify the problem.  In RenderLayerCompositor::addToOverlapMap(),  a new line was added that computes the clipRect.  That computation indirectly calls convertToLayerCoords(), which supposedly should not be called when transforms are involved, but in this situation transforms may be in the hierarchy.

Simon, did you have a chance to check again that the reduced test case reproduces in webkit nightly?  It still reproduces for me, I'm on osx 10.6.8.  I see a red square indicating the green div does not get properly composited.  If you open the Inspector and reduce the x-translation, then there is a non-empty entry in the overlapMap, which causes the green div to be composited correctly (even though the clipRect is still not transformed correctly).

Also, both my attempted hacks do indeed fix the problem, but they break other layout tests.  I have tried two hacks:

(1) in RenderLayer::convertToLayerCoords, at the very bottom of the function before returning, I tried to add the 2-D component of m_transform.

(2) in RenderLayer::calculateClipRects  I tried replacing convertToLayerCoords with a call to localToAbsolute().

I can continue by debugging why other layout tests fail in those cases, but it would save me several days of frustration if someone can suggest what the correct fix might look like. =)

[Simon Fraser (smfr)]

Testcase still fails in Safari TOT.

The overlap map is a huge hack and source of bugs.

[Shawn Singh]

Created attachment 125726 [details]
Patch

Tested (before and after applying the fix) on osx, for chromium and safari... no obvious regressions

[Shawn Singh]

Julien, when you have time could you please give this an unofficial review?   (unofficial, because I think this should also pass through Simon when he's back.)

Thanks in advance!

[Simon Fraser (smfr)]

Comment on attachment 125726 [details]
Patch

I'd rather we investigate how to fix this for all transforms, rather than have a hack that just works for translation.

[Shawn Singh]

(In reply to comment #8)
> (From update of attachment 125726 [details])
> I'd rather we investigate how to fix this for all transforms, rather than have a hack that just works for translation.

OK... Thanks for looking at this =)  Supporting all transforms will be a much deeper change.

Proposed Fix:

Create a second version of convertToLayerCoords.  Where appropriate, we'll use the new version instead of the old one.

Details:

Many calls to convertToLayerCoords assume that the top-left corner in local space will remain the top-left corner in the ancestor space.  However, this is incorrect - for example if a layer is rotated by 90 degrees.  So... to support transforms in general, we will need a new version of convertToLayerCoords that actually applies to all four corners (probably in a FloatQuad), and at the end, computes the bounding box.

As a side-note, there already is a convertToLayerCoords( LayoutRect ).  The new implementation I'm proposing might replace that function.

[Shawn Singh]

Created attachment 126895 [details]
ConvertToLayerCoords fixed for general transforms

[Shawn Singh]

About the most recent patch:

This patch:
  - Tested manually and with all layout tests on osx for both chromium and safari.
  - The patch is not a "hack" anymore, but there will need to be a few follow-up patches.


I didn't put it all in one patch because:
- Chromium needs this fix to be back-ported to 18, and its too risky to do that with additional changes unrelated to our bug.  This bug is also becoming a priority since it will become visible to more users on Google Maps for version 18.
- To make it easier to review.  The main fix is in this patch already.


The follow-up patches that are needed:
1. implement support in clipRects for converting a full rect rather than just the offset.  Ideally to be accompanied by a rotation test case similar to the translation one provided in this patch.
2. check the FIXME in RenderLayerCompositor::recursiveRepaintLayerRects()... with this fix, this function may work correctly with transforms after being updated slightly.

[James Robinson]

+cc Levi and Emil in case they have feedback on the LayoutUnit/FloatPoint stuff.

[WebKit Review Bot]

Comment on attachment 126895 [details]
ConvertToLayerCoords fixed for general transforms

Attachment 126895 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11515115

New failing tests:
fast/block/float/overhanging-tall-block.html

[Simon Fraser (smfr)]

Comment on attachment 126895 [details]
ConvertToLayerCoords fixed for general transforms

View in context: https://bugs.webkit.org/attachment.cgi?id=126895&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1335
> +    if (useTransforms && m_transform)
> +        location = m_transform->mapPoint(location);

This doesn't do the right thing with 3D transform, because it will flatten the point at every layer. The code would need to use TransformState to get this right (or map points via RenderObjects).

I still think the right approach is to avoid calling convertToLayerCoords across transform boundaries (maybe just 3D boundaries)? I'd have to wrap my head around what that implies for clip rects though.

> Source/WebCore/rendering/RenderLayer.cpp:1354
> +    FloatPoint p1 = FloatPoint(rect.minXMinYCorner());
> +    FloatPoint p2 = FloatPoint(rect.maxXMinYCorner());
> +    FloatPoint p3 = FloatPoint(rect.minXMaxYCorner());
> +    FloatPoint p4 = FloatPoint(rect.maxXMaxYCorner());
> +
> +    convertToLayerCoordsInternal(ancestorLayer, p1, true);
> +    convertToLayerCoordsInternal(ancestorLayer, p2, true);
> +    convertToLayerCoordsInternal(ancestorLayer, p3, true);
> +    convertToLayerCoordsInternal(ancestorLayer, p4, true);
> +
> +    // Create a LayoutRect from the four converted corners.
> +    float xmin = min(min(p1.x(), p2.x()), min(p3.x(), p4.x()));
> +    float ymin = min(min(p1.y(), p2.y()), min(p3.y(), p4.y()));
> +    float xmax = max(max(p1.x(), p2.x()), max(p3.x(), p4.x()));
> +    float ymax = max(max(p1.y(), p2.y()), max(p3.y(), p4.y()));

I think it would be better to write methods that take FloatQuads to avoid calling things 4 times.

> Source/WebCore/rendering/RenderLayer.h:429
> +    void convertToLayerCoords(const RenderLayer* ancestorLayer, LayoutPoint& location, bool useTransforms = false) const;

The boolean argument makes the calling code less readable. It would be better to use an enum or have two separate methods.

[Shawn Singh]

Thanks for the comments...  I also found a few more details that finally shed more light on it =)

Will come back with a better patch in a few days.

[Shawn Singh]

Created attachment 128513 [details]
Cleaner solution, please see follow-up comments

tested on osx, both webkit and chromium layout tests, no apparent regressions.

[Shawn Singh]

The Bottom Line:

The proposed fix is essentially the last good option I can propose.   Simon, Julien, and Enne, thank you very much for your patience and feedback.


Details:

There was a cleaner option, to use selfClipRect().  I gave my sincerest effort to use that function in RenderLayerCompositor::addToOverlapMap, which should have been the correct solution.  However, doing this led to a deep rabbit hole of several issues, and eventually I reached a fundamental design knot between backgroundClipRect and clipping for composited layers.  This may come back to bite us, now that CSS transforms and clipping are more commonly used.   Simon if you think this is important, then let's discuss it.

[Simon Fraser (smfr)]

Comment on attachment 128513 [details]
Cleaner solution, please see follow-up comments

Rather than all this new code, could you use RenderObject::localToContainerQuad() to convert the clip rect?

[Shawn Singh]

(In reply to comment #18)
> (From update of attachment 128513 [details])
> Rather than all this new code, could you use RenderObject::localToContainerQuad() to convert the clip rect?

That should work, yes.  I'll try it out, test it fully, and submit a new patch.   Earlier I thought we had a strong preference to using the RenderLayer hierarchy when possible, for performance.

[Shawn Singh]

Created attachment 128644 [details]
Patch

Tested on mac on both webkit and chromium, no obvious regressions.

[Shawn Singh]

(In reply to comment #20)
> Created an attachment (id=128644) [details]
> Patch
> 
> Tested on mac on both webkit and chromium, no obvious regressions.

Requesting this to be reviewed, hopefully early this week?  Thanks very much in advance.

[Simon Fraser (smfr)]

Comment on attachment 128644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128644&action=review

> Source/WebCore/rendering/RenderLayer.cpp:3657
> -        convertToLayerCoords(rootLayer, offset);
> +        offset = roundedIntPoint(renderer()->localToContainerPoint(FloatPoint(), rootLayer->renderer()));

I think a comment here would be useful, explaining why you can't use convertToLayerCoords().

> Source/WebCore/rendering/RenderObject.cpp:2098
> +    // Track the point at the center of the quad's bounding box. As mapLocalToContainer() calls offsetFromContainer(),

This comment is confusing, since there is no other reference to a quad in this method.

[Shawn Singh]

Created attachment 129105 [details]
Patch for landing

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 129105 [details]:

css3/filters/effect-invert-hw.html bug 79639 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 129105 [details]
Patch for landing

Clearing flags on attachment: 129105

Committed r109060: <http://trac.webkit.org/changeset/109060>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.