Bug 22570 - Issues rendering reflected, overflow:hidden elements
Summary: Issues rendering reflected, overflow:hidden elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-12-01 11:53 PST by Simon Fraser (smfr)
Modified: 2008-12-18 09:43 PST (History)
1 user (show)

See Also:


Attachments
Testcase (1.78 KB, text/html)
2008-12-01 11:54 PST, Simon Fraser (smfr)
no flags Details
Patch, testcase, changelog (12.03 KB, patch)
2008-12-01 12:17 PST, Simon Fraser (smfr)
hyatt: review-
Details | Formatted Diff | Diff
Just the clearClipRect(s) method renames as step 1 (5.03 KB, patch)
2008-12-07 09:50 PST, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff
Patch, testcase, changelog (25.36 KB, patch)
2008-12-17 23:29 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch, testcase, changelog (minor patch change) (25.35 KB, patch)
2008-12-17 23:35 PST, Simon Fraser (smfr)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-12-01 11:53:31 PST
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.
Comment 1 Simon Fraser (smfr) 2008-12-01 11:54:04 PST
Created attachment 25632 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2008-12-01 11:54:21 PST
<rdar://problem/6385962>
Comment 3 Simon Fraser (smfr) 2008-12-01 12:17:43 PST
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.
Comment 4 Simon Fraser (smfr) 2008-12-01 15:44:11 PST
I fix the misspelling of clearClipRectsIncludingDescedants before committing.
Comment 5 Dave Hyatt 2008-12-03 09:18:47 PST
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.
Comment 6 Dave Hyatt 2008-12-03 09:19:13 PST
However I'll still r+ it if you fix the comments and typos.

Comment 7 Simon Fraser (smfr) 2008-12-03 09:25:27 PST
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).
Comment 8 Simon Fraser (smfr) 2008-12-07 09:50:13 PST
Created attachment 25830 [details]
Just the clearClipRect(s) method renames as step 1
Comment 9 Simon Fraser (smfr) 2008-12-10 09:18:37 PST
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.
Comment 10 Simon Fraser (smfr) 2008-12-17 23:29:03 PST
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.
Comment 11 Simon Fraser (smfr) 2008-12-17 23:35:24 PST
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.
Comment 12 Dave Hyatt 2008-12-18 09:29:12 PST
Comment on attachment 26116 [details]
Patch, testcase, changelog (minor patch change)

r=me
Comment 13 Dave Hyatt 2008-12-18 09:31:46 PST
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.

Comment 14 Simon Fraser (smfr) 2008-12-18 09:40:01 PST
Filed bug 22916 to fix the O(n^2) behavior while painting reflected layers.
Comment 15 Simon Fraser (smfr) 2008-12-18 09:43:28 PST
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