Bug 12123 – REGRESSION: Incomplete repaint of floats' overflows

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

See the attached test case. This is a regression from fixing bug 5633.

Created attachment 12238 [details]
Test case

Created attachment 12239 [details]
Another case (floats belong to a descendant of the moving object)

[Mark Rowe (bdash)]

<rdar://problem/4960268>

Created attachment 12902 [details]
Assign painting of overflowing floats to the parent

Comment on attachment 12902 [details]
Assign painting of overflowing floats to the parent

I'm seeing some problems with this patch (the same float being painted by two blocks). Investigating.

Created attachment 12904 [details]
Assign painting of overflowing floats to the parent

Comment on attachment 12904 [details]
Assign painting of overflowing floats to the parent

I have one more thing to fix, so don't review yet!

Created attachment 12907 [details]
Assign painting of overflowing floats to the parent

[Dave Hyatt]

Comment on attachment 12907 [details]
Assign painting of overflowing floats to the parent

This is a pretty significant change.  I'm a little nervous about it.

[Maciej Stachowiak]

We've gotta fix the bug somehow...

The idea of the patch is to use the floating objects list not only for a block's own floats and children/sibling overhanging floats but also for children's overflowing floats. The latter never affect the layout of the parent block, but having them in the list lets the parent include them in its repaint rects (without having to query the children) and repaint them (again, without having to drill down the subtree).

[Dave Hyatt]

Comment on attachment 12907 [details]
Assign painting of overflowing floats to the parent

Ok, I think conceptually this is good.  I have a slight concern about performance in that you removed some hasOverhangingFloats() checks in favor of simply always walking the floating objects list.  This means all the floats that don't overhang or overflow are being examined all the time now.

Seems like you should rename  repaintOverhangingFloats to repaintOverflowingFloats, since it now repaints both potentially.

Please add flexible box tests for both the horizontal and vertical directions.

Please add tests for inline blocks and inline tables.

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

Created attachment 13050 [details]
Assign painting of overflowing floats to the parent

(In reply to comment #12)
> Seems like you should rename  repaintOverhangingFloats to
> repaintOverflowingFloats, since it now repaints both potentially.

Done.

> Please add flexible box tests for both the horizontal and vertical directions.

Done.

> Please add tests for inline blocks and inline tables.

Done. The inline block case is failing and I don't have a fix for it in this patch. The problem is that if I hand over the inline block's floats to the outer block then the painting order is wrong. I think the right way to fix the inline block case is to include the floats' overflow in the root line box's overflow, but I didn't want to do it in this patch (the simplest test case for this is broken in Safari 2, so it's not a regression).

[Dave Hyatt]

Comment on attachment 13050 [details]
Assign painting of overflowing floats to the parent

This code in RenderTable:

+    if (paintPhase == PaintPhaseFloat)
+        paintFloats(paintInfo, tx, ty, false);

will (I believe) cause a regression when doing selection dragging, since the text inside the floats will no longer be painted as part of the selection.

I believe that assigning overflowing floats inside table cells to be painted by the table will result in potential stacking order regressions if the floats overlap.

Cells do not paint precisely in document order and yet that's what this float painting is going to do.

I think now that a different approach is perhaps warranted here.  The issue at hand seems to be that there are repaint issues with floats because of horizontal float "overflow".

These sorts of repaint issues have already been solved with normal flow overflow.  I think a much simpler solution that will avoid forcing tables and flexible boxes to worry about maintaining floating objects lists would be to incorporate horizontal spillage from float overflow into m_overflowLeft and m_overflowWidth.

Vertical float spillage was traditionally not considered overflow simply because the next block could also contain the float.  However, this is never the case with horizontal float spillage.  I think you can treat horizontal float spillage as overflow out of any blocks that the float ends up intersecting, and you'll end up with a much simpler patch.

Created attachment 13090 [details]
Unify floats with overflow

This patch takes a different approach, as suggested by Hyatt. It includes a change log, but the gist of it is to treat floats as any other overflow, while establishing that overflow is strictly visual (by removing the only case where overflow affects layout in WebKit, which is table cell's expand to encompass overflow behavior). Overhanging floats whose painting is propagated to an ancestor only factor into the overflow of the ancestor painting them. This is done by deferring the addition of floats to the overflow until a parent gets a chance to adopt overhanging floats. This can be changed, in the spirit of the previous patch, to have parents adopt also overflowing floats, but for now I've left things the way they are. Blocks that are not in a block formatting context (including table cells) always paint their own floats and therefore their floats are always added to their overflow.

[Dave Hyatt]

Can't you go further and just eliminate the floatRect() method?  Won't it just return the same answer as overflowRect?  It no longer seems necessary to me.

(In reply to comment #17)
> Can't you go further and just eliminate the floatRect() method?  Won't it just
> return the same answer as overflowRect?  It no longer seems necessary to me.
> 

I'm using floatRect() to add the floats to the overflowRect (in the non-deferring cases). I guess I can make it protected (if it isn't already).

Comment on attachment 13090 [details]
Unify floats with overflow

Going to make one small change and add a test with float:right.

Created attachment 13143 [details]
Unify floats with overflow

Added test for float:right and made floatRect() protected.

[Dave Hyatt]

Comment on attachment 13143 [details]
Unify floats with overflow

r=me

Woo hoo.

[Dave Hyatt]

Comment on attachment 13143 [details]
Unify floats with overflow

r=me

Woo hoo.

[Mark Rowe (bdash)]

Landed in r19588.

[Mark Rowe (bdash)]

I rolled this patch out in r19593 as it had caused build failures, and when the obvious fix was applied I was seeing a hang in the layout tests.

Created attachment 13147 [details]
Unify floats with overflow

Fixed the build error by leaving floatRect() public. The other change is that I made sure that table cells' vertical overflow is repainted, now that it can exist, and added a test for it.

I think the layout test failures mentioned in comment #24 are from a different patch (<http://trac.webkit.org/projects/webkit/changeset/19583>).

[Dave Hyatt]

Comment on attachment 13147 [details]
Unify floats with overflow

r=me

[Alexey Proskuryakov]

I tried to land this patch, but got a number of failures in tables/mozilla_expected_failures/marvin/table_overflow*, and also mozilla/bugs/bug196870.

Looks like the change to fast/text/text-shadow-extreme-value.html comes from another fix.

Created attachment 13226 [details]
Unify floats with overflow

Updated the test results section and moved a star in RenderLayer.cpp :-)

[Alexey Proskuryakov]

Committed revision 19696.