Bug 6770 - REGRESSION: Incomplete repaint when block with clipping grows
Summary: REGRESSION: Incomplete repaint when block with clipping grows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2006-01-24 13:20 PST by mitz
Modified: 2006-04-19 19:48 PDT (History)
2 users (show)

See Also:


Attachments
Testcase (401 bytes, text/html)
2006-01-24 13:22 PST, mitz
no flags Details
Possible fix (2.73 KB, patch)
2006-04-16 14:07 PDT, mitz
no flags Details | Formatted Diff | Diff
Possible fix (2.73 KB, patch)
2006-04-16 15:36 PDT, mitz
no flags Details | Formatted Diff | Diff
Possible fix (2.74 KB, patch)
2006-04-16 15:37 PDT, mitz
hyatt: review-
Details | Formatted Diff | Diff
Patch, including change log and manual test (5.52 KB, patch)
2006-04-18 14:15 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-01-24 13:20:40 PST
See the attached testcase. In this case, since the block with the border grows and repaints before its container (which is clipping) grows, so in RenderObject::repaintAfterLayoutIfNeeded it sees that its old bounds are equal to its new bounds and it doesn't repaint. When the container repaints, it paints only the delta, so the old border of the inner block isn't erased.
Comment 1 mitz 2006-01-24 13:22:00 PST
Created attachment 5926 [details]
Testcase
Comment 2 Alice Liu 2006-03-20 06:47:53 PST
<rdar://problem/4483838>
Comment 3 mitz 2006-04-16 14:07:41 PDT
Created attachment 7750 [details]
Possible fix

I think this patch does not add a lot of painting, and what it does add can be divided between
1) what's needed to fix the bug: the layer did not need layout, but ended up resizing because of a child.
2) addressing the fact that the selfNeedsLayout() condition in repaintAfterLayoutIfNeeded() is always false when called from the layer.
Comment 4 mitz 2006-04-16 15:36:15 PDT
Created attachment 7751 [details]
Possible fix

Please see my previous comment.
Comment 5 mitz 2006-04-16 15:37:54 PDT
Created attachment 7752 [details]
Possible fix

Sorry, uploaded the wrong file.
Comment 6 Darin Adler 2006-04-16 19:39:54 PDT
Comment on attachment 7752 [details]
Possible fix

This sure looks good to me, but I think it's something Hyatt will want to review himself.
Comment 7 Dave Hyatt 2006-04-17 14:12:04 PDT
Won't this cause way too much repainting?  Turn on paint flashing and try loading a huge file like:

http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp

Do you see the whole window repainting now with this fix as chunks of data come in?

It seems like this fix will cause the root block's layer to repaint whenever new chunks of data come in that make the root block's layer grow.
Comment 8 mitz 2006-04-17 15:00:26 PDT
(In reply to comment #7)
> It seems like this fix will cause the root block's layer to repaint whenever
> new chunks of data come in that make the root block's layer grow.

Oh boy, I overlooked that! I think instead of 
+        m_fullRepaintOnResize = m_object->selfNeedsLayout() || m_object->normalChildNeedsLayout();
it should be
+        m_fullRepaintOnResize = m_object->selfNeedsLayout() || m_object->hasOverflowClip() && m_object->normalChildNeedsLayout();
Comment 9 Dave Hyatt 2006-04-17 17:39:55 PDT
Comment on attachment 7752 [details]
Possible fix

Even that change will still cause overflow blocks to repaint over and over as they load content.
Comment 10 mitz 2006-04-18 01:13:06 PDT
(In reply to comment #9)
> (From update of attachment 7752 [details] [edit])
> Even that change will still cause overflow blocks to repaint over and over as
> they load content.

In what case, other than when adding content causes their layer to resize (which is what this bug is about)?
Comment 11 mitz 2006-04-18 14:15:56 PDT
Created attachment 7810 [details]
Patch, including change log and manual test

Added the hasOverflowClip() check and renamed the flag per Hyatt's suggestion.
Comment 12 Dave Hyatt 2006-04-18 14:17:30 PDT
Comment on attachment 7810 [details]
Patch, including change log and manual test

r=me
Comment 13 Timothy Hatcher 2006-04-19 19:48:01 PDT
Landed in r13982.