Bug 12123 - REGRESSION: Incomplete repaint of floats' overflows
Summary: REGRESSION: Incomplete repaint of floats' overflows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
: 9315 (view as bug list)
Depends on:
Blocks: 10116 10898
  Show dependency treegraph
 
Reported: 2007-01-05 04:04 PST by mitz
Modified: 2007-02-18 08:26 PST (History)
6 users (show)

See Also:


Attachments
Test case (430 bytes, text/html)
2007-01-05 04:05 PST, mitz
no flags Details
Another case (floats belong to a descendant of the moving object) (981 bytes, text/html)
2007-01-05 05:06 PST, mitz
no flags Details
Assign painting of overflowing floats to the parent (36.64 KB, patch)
2007-02-03 12:11 PST, mitz
no flags Details | Formatted Diff | Diff
Assign painting of overflowing floats to the parent (37.42 KB, patch)
2007-02-03 15:59 PST, mitz
no flags Details | Formatted Diff | Diff
Assign painting of overflowing floats to the parent (38.44 KB, patch)
2007-02-03 23:12 PST, mitz
hyatt: review-
Details | Formatted Diff | Diff
Assign painting of overflowing floats to the parent (45.40 KB, patch)
2007-02-08 03:46 PST, mitz
hyatt: review-
Details | Formatted Diff | Diff
Unify floats with overflow (601.19 KB, patch)
2007-02-09 12:32 PST, mitz
no flags Details | Formatted Diff | Diff
Unify floats with overflow (635.37 KB, patch)
2007-02-12 15:19 PST, mitz
no flags Details | Formatted Diff | Diff
Unify floats with overflow (654.03 KB, patch)
2007-02-13 01:50 PST, mitz
hyatt: review+
Details | Formatted Diff | Diff
Unify floats with overflow (656.33 KB, patch)
2007-02-18 07:52 PST, mitz
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-01-05 04:04:47 PST
See the attached test case. This is a regression from fixing bug 5633.
Comment 1 mitz 2007-01-05 04:05:46 PST
Created attachment 12238 [details]
Test case
Comment 2 mitz 2007-01-05 05:06:20 PST
Created attachment 12239 [details]
Another case (floats belong to a descendant of the moving object)
Comment 3 Mark Rowe (bdash) 2007-01-28 19:11:29 PST
<rdar://problem/4960268>
Comment 4 mitz 2007-02-03 12:11:35 PST
Created attachment 12902 [details]
Assign painting of overflowing floats to the parent
Comment 5 mitz 2007-02-03 15:08:10 PST
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.
Comment 6 mitz 2007-02-03 15:59:51 PST
Created attachment 12904 [details]
Assign painting of overflowing floats to the parent
Comment 7 mitz 2007-02-03 16:23:28 PST
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!
Comment 8 mitz 2007-02-03 23:12:25 PST
Created attachment 12907 [details]
Assign painting of overflowing floats to the parent
Comment 9 Dave Hyatt 2007-02-07 02:25:22 PST
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.
Comment 10 Maciej Stachowiak 2007-02-07 08:05:12 PST
We've gotta fix the bug somehow...
Comment 11 mitz 2007-02-07 08:42:04 PST
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).
Comment 12 Dave Hyatt 2007-02-07 15:47:40 PST
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.
Comment 13 mitz 2007-02-08 03:07:55 PST
*** Bug 9315 has been marked as a duplicate of this bug. ***
Comment 14 mitz 2007-02-08 03:46:53 PST
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).
Comment 15 Dave Hyatt 2007-02-08 21:34:34 PST
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.
Comment 16 mitz 2007-02-09 12:32:31 PST
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.
Comment 17 Dave Hyatt 2007-02-12 02:45:32 PST
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.

Comment 18 mitz 2007-02-12 02:56:05 PST
(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 19 mitz 2007-02-12 14:53:13 PST
Comment on attachment 13090 [details]
Unify floats with overflow

Going to make one small change and add a test with float:right.
Comment 20 mitz 2007-02-12 15:19:36 PST
Created attachment 13143 [details]
Unify floats with overflow

Added test for float:right and made floatRect() protected.
Comment 21 Dave Hyatt 2007-02-12 15:21:25 PST
Comment on attachment 13143 [details]
Unify floats with overflow

r=me

Woo hoo.
Comment 22 Dave Hyatt 2007-02-12 15:21:27 PST
Comment on attachment 13143 [details]
Unify floats with overflow

r=me

Woo hoo.
Comment 23 Mark Rowe (bdash) 2007-02-12 16:02:49 PST
Landed in r19588.
Comment 24 Mark Rowe (bdash) 2007-02-12 17:52:57 PST
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.
Comment 25 mitz 2007-02-13 01:50:01 PST
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>).
Comment 26 Dave Hyatt 2007-02-17 23:06:09 PST
Comment on attachment 13147 [details]
Unify floats with overflow

r=me
Comment 27 Alexey Proskuryakov 2007-02-18 07:11:34 PST
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.
Comment 28 mitz 2007-02-18 07:52:39 PST
Created attachment 13226 [details]
Unify floats with overflow

Updated the test results section and moved a star in RenderLayer.cpp :-)
Comment 29 Alexey Proskuryakov 2007-02-18 08:26:48 PST
Committed revision 19696.