RESOLVED FIXED 7204
float inserted in fixed height block via DOM not repainted 
https://bugs.webkit.org/show_bug.cgi?id=7204
Summary float inserted in fixed height block via DOM not repainted
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-
DetailsFormatted DiffDiff
Revised patch (36.07 KB, patch)
2006-06-30 02:22 PDT, mitz 		hyatt: review+
DetailsFormatted DiffDiff
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.