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).
Created attachment 6422 [details] example showing the rendering problem
Created attachment 6423 [details] screenshot showing rendering problem
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.
See also bug 8267.
Created attachment 8865 [details] Repaint all floating descendants when moving during layout
*** Bug 8267 has been marked as a duplicate of this bug. ***
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).
(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...
Created attachment 9102 [details] Revised patch
Comment on attachment 9102 [details] Revised patch r=me.
Committed revision 15121.