Bug 76486

Created attachment 122826 [details] Reduced test case

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.

(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.

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. =)

Testcase still fails in Safari TOT. The overlap map is a huge hack and source of bugs.

Created attachment 125726 [details] Patch Tested (before and after applying the fix) on osx, for chromium and safari... no obvious regressions

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!

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.

(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.

Created attachment 126895 [details] ConvertToLayerCoords fixed for general transforms

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.

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

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

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.

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.

Created attachment 128513 [details] Cleaner solution, please see follow-up comments tested on osx, both webkit and chromium layout tests, no apparent regressions.

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.

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?

(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.

Created attachment 128644 [details] Patch Tested on mac on both webkit and chromium, no obvious regressions.

(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.

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.

Created attachment 129105 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 129105 [details]: css3/filters/effect-invert-hw.html bug 79639 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 129105 [details] Patch for landing Clearing flags on attachment: 129105 Committed r109060: <http://trac.webkit.org/changeset/109060>

All reviewed patches have been landed. Closing bug.