Bug 7204

Summary:

float inserted in fixed height block via DOM not repainted

Product:

WebKit

Reporter:

Randy Saldinger <randy>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt, ian, mitz, morten.larsen

Priority:

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

Bug Blocks:

9647

Attachments:

Description	Flags
example showing the rendering problem	none
screenshot showing rendering problem	none
Reduced testcase	none
Repaint all floating descendants when moving during layout	hyatt: review-
Revised patch	hyatt: review+

Randy Saldinger

Reported 2006-02-11 19:17:35 PST

The document in this example has the following structure: div1 - height: 75px div2 - margin-right: 25px div3 - float: right text node div1 is static, and div2+div3+text are alternately inserted and deleted via JavaScript. After insertion, the div3 float gets only partially painted, as shown in the upper screenshot. The problem doesn't happen if the div1 height is auto. Nor does it happen if div3 has position: relative. Nor if the whole structure is static on the page (as shown in the lower screenshot).

Attachments
example showing the rendering problem (2.55 KB, text/html) 2006-02-11 19:18 PST, Randy Saldinger	no flags	Details
screenshot showing rendering problem (13.70 KB, image/png) 2006-02-11 19:19 PST, Randy Saldinger	no flags	Details
Reduced testcase (597 bytes, text/html) 2006-02-13 11:59 PST, mitz	no flags	Details
Repaint all floating descendants when moving during layout (34.97 KB, patch) 2006-06-16 04:14 PDT, mitz	hyatt: review-	Details Formatted Diff Diff
Revised patch (36.07 KB, patch) 2006-06-30 02:22 PDT, mitz	hyatt: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Randy Saldinger

Comment 1 2006-02-11 19:18:52 PST

Created attachment 6422 [details] example showing the rendering problem

Randy Saldinger

Comment 2 2006-02-11 19:19:40 PST

Created attachment 6423 [details] screenshot showing rendering problem

mitz

Comment 3 2006-02-13 11:59:46 PST

Created attachment 6462 [details] Reduced testcase This demonstrates at least one aspect of the problem. The titleBlock div repaints itself including the float (which also repaints itself) during its layout, but only then it is moved to its final position by its containing block, which appropriately calls the titleBlock's repaintDuringLayoutIfMoved. However, repaintDuringLayoutIfMoved does not invalidate the float, since it is painted by <body> (which is unaware of any of this). I don't understand why repaintDuringLayoutIfMoved doesn't use full bounds like repaintAfterLayoutIfNeeded does.

mitz

Comment 4 2006-06-09 00:42:26 PDT

See also bug 8267.

mitz

Comment 5 2006-06-16 04:14:31 PDT

Created attachment 8865 [details] Repaint all floating descendants when moving during layout

mitz

Comment 6 2006-06-16 04:15:07 PDT

*** Bug 8267 has been marked as a duplicate of this bug. ***

Dave Hyatt

Comment 7 2006-06-23 22:26:06 PDT

Comment on attachment 8865 [details] Repaint all floating descendants when moving during layout I'm not convinced on this one quite yet. When a float moves during layout, it should still get its coordinates changed in the floating objects list of the block that will ultimately paint it. What bothers me about this patch is that it will end up repainting the same float over and over when you have a bunch of nested blocks that all have the same float in their floatingObjects list. I also don't quite understand your point about repaintAfterLayout vs. repaintDuringLayout since the former uses getAbsoluteRepaintRectIncludingFloats (which also has the !noPaint check). Also for a check like this: + if (paintAll || !r->noPaint && !r->node->layer()) { You'll do a repaint even of something with a layer, and that's not necessary (since layers handle repainting themselves properly already... make the same test case with position:relative on the float and you'll see it work I bet).

mitz

Comment 8 2006-06-24 03:06:57 PDT

(In reply to comment #7) > (From update of attachment 8865 [details] [edit]) > I'm not convinced on this one quite yet. When a float moves during layout, it > should still get its coordinates changed in the floating objects list of the > block that will ultimately paint it. The way it works is that the floating objects list is first cleared, then the float is re-inserted with the changed coordinates, so there's no easy way to identify the "moved" case. > What bothers me about this patch is that it will end up repainting the same > float over and over when you have a bunch of nested blocks that all have the > same float in their floatingObjects list. Only if each one of those blocks has only moved during layout, which I had a hard time creating an example of (I had to change the height of two "spacer" blocks at the same time, each casuing a different ancestor of the float to move - but not resize). Actually, it was easier (though still requiring two changes) when I didn't require both blocks to be ancestors of the float, but I think that case can be handled by painting only descendants in repaintFloatingDescendants() if the paintAll flag is true (you may want to rename that method to repaintOverhangingFloats(), which is what it really does, and then the bool could be called paintAllDescendants). > I also don't quite understand your point about repaintAfterLayout vs. > repaintDuringLayout since the former uses getAbsoluteRepaintRectIncludingFloats > (which also has the !noPaint check). Yeah, I was wrong, and I realized it once I started working on a fix. > Also for a check like this: > > + if (paintAll || !r->noPaint && !r->node->layer()) { > > You'll do a repaint even of something with a layer, and that's not necessary > (since layers handle repainting themselves properly already... make the same > test case with position:relative on the float and you'll see it work I bet). > Agreed. To sum up, I think that with the descendants-only condition added, the layer case fixed, and a possible rename, it comes down to how serious the repeated painting (invalidation really) issue is...

mitz

Comment 9 2006-06-30 02:22:45 PDT

Created attachment 9102 [details] Revised patch

Dave Hyatt

Comment 10 2006-06-30 14:27:00 PDT

Comment on attachment 9102 [details] Revised patch r=me.

David Kilzer (:ddkilzer)

Comment 11 2006-06-30 19:46:04 PDT

Committed revision 15121.

Note You need to log in before you can comment on or make changes to this bug.