Bug 22570 – Issues rendering reflected, overflow:hidden elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Testcase shows a problem where rendering of an element with overflow:hidden breaks when it has a reflection, and the reflection is not rendered correctly.

[Simon Fraser (smfr)]

Created attachment 25632 [details]
Testcase

[Simon Fraser (smfr)]

<rdar://problem/6385962>

[Simon Fraser (smfr)]

Created attachment 25633 [details]
Patch, testcase, changelog

Here's a sledgehammer approach: nuke the cached clip rects before and after rendering the reflection.

I also renamed the confusing clearClipRect()/clearClipRects().

A better approach might be to allow the computation of a non-cached set of clip rects, and only use those transiently during reflection painting.

[Simon Fraser (smfr)]

I fix the misspelling of clearClipRectsIncludingDescedants before committing.

[Dave Hyatt]

Comment on attachment 25633 [details]
Patch, testcase, changelog

Descedants should be Descendants.

Reflections don't change the painting root. Maybe you meant that the root layer gets shifted because the reflection has a transform. The comment is wrong, since the |paintingRoot| argument doesn't change.

Anyway rather than clearing the cached results (twice), I think a simpler fix might be to just make the reflection painting not use the cached values.

[Dave Hyatt]

However I'll still r+ it if you fix the comments and typos.

[Simon Fraser (smfr)]

I'll change it so that reflection painting doesn't use the cached values. That will require the ability to compute clipRects independently of caching them, which may be useful for other reasons (but might get messy to do up the parent chain).

[Simon Fraser (smfr)]

Created attachment 25830 [details]
Just the clearClipRect(s) method renames as step 1

[Simon Fraser (smfr)]

Cleanup patch committed:
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderBox.cpp
	M	WebCore/rendering/RenderLayer.cpp
	M	WebCore/rendering/RenderLayer.h
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderWidget.cpp
Committed r39175

Keeping open for the actual fix.

[Simon Fraser (smfr)]

Created attachment 26115 [details]
Patch, testcase, changelog

This patch does the right thing; it adds the ability to compute clipRects on the fly, and passes a flag down when painting reflections to use such transient clip rects to avoid the layer caching invalid ones.

[Simon Fraser (smfr)]

Created attachment 26116 [details]
Patch, testcase, changelog (minor patch change)

reflectionLayer()->paintLayer() doesn't need to set the temporaryClipRects flag, because the RenderReplicate paintLayer() does that.

[Dave Hyatt]

Comment on attachment 26116 [details]
Patch, testcase, changelog (minor patch change)

r=me

[Dave Hyatt]

I'm giving this r+, but it does seem to have O(n^2) behavior when painting reflections with a deep layer hierarchy, since each individual layer will compute clip rects without caching... and end up crawling all the way up to the root of the reflection.

[Simon Fraser (smfr)]

Filed bug 22916 to fix the O(n^2) behavior while painting reflected layers.

[Simon Fraser (smfr)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/reflections/reflection-overflow-hidden.html
	A	LayoutTests/platform/mac/fast/reflections/reflection-overflow-hidden-expected.checksum
	A	LayoutTests/platform/mac/fast/reflections/reflection-overflow-hidden-expected.png
	A	LayoutTests/platform/mac/fast/reflections/reflection-overflow-hidden-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderLayer.cpp
	M	WebCore/rendering/RenderLayer.h
	M	WebCore/rendering/RenderReplica.cpp
Committed r39373