Bug 119979 - REGRESSION(r127163): Respect clearance set on ancestors when placing floats
Summary: REGRESSION(r127163): Respect clearance set on ancestors when placing floats
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on: 120129
Blocks: 103116
  Show dependency treegraph
 
Reported: 2013-08-18 06:40 PDT by Robert Hogan
Modified: 2013-11-21 12:28 PST (History)
14 users (show)

See Also:


Attachments
Reduction (479 bytes, text/html)
2013-08-18 06:40 PDT, Robert Hogan
no flags Details
Patch (6.70 KB, patch)
2013-08-18 06:46 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (9.18 KB, patch)
2013-08-19 14:58 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (8.03 KB, patch)
2013-08-20 11:59 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (14.65 KB, patch)
2013-09-14 04:11 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (17.04 KB, patch)
2013-09-17 14:18 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (17.10 KB, patch)
2013-10-19 05:48 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (17.14 KB, patch)
2013-10-21 10:43 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (22.77 KB, patch)
2013-11-11 12:46 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (22.57 KB, patch)
2013-11-11 12:59 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (22.94 KB, patch)
2013-11-12 10:13 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (633.72 KB, application/zip)
2013-11-12 11:00 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (738.16 KB, application/zip)
2013-11-12 14:00 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (725.21 KB, application/zip)
2013-11-12 14:39 PST, Build Bot
no flags Details
Patch (30.45 KB, patch)
2013-11-15 11:14 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2013-08-18 06:40:47 PDT
Created attachment 209028 [details]
Reduction

..
Comment 1 Robert Hogan 2013-08-18 06:46:28 PDT
Created attachment 209029 [details]
Patch
Comment 2 Dave Hyatt 2013-08-19 13:08:10 PDT
Comment on attachment 209029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209029&action=review

r-

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2584
> +LayoutUnit RenderBlock::LineBreaker::marginOffsetForSelfCollapsingBlock(RenderBlock* child)

This function should be in RenderBlock.cpp and not in the line layout file.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2586
> +    while (child) {

child is a bad name for this variable, since you're looping up through parents. I'd assign to a current variable.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2589
> +        if (child->previousSibling() || !child->parent() || !child->parent()->isRenderBlock())

The previousSibling() could be floating or positioned. I think you are wanting to ask if there is a previous normal flow sibling.
Comment 3 Robert Hogan 2013-08-19 14:58:05 PDT
Created attachment 209127 [details]
Patch
Comment 4 Dave Hyatt 2013-08-20 11:18:34 PDT
Comment on attachment 209127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209127&action=review

r-

> Source/WebCore/rendering/RenderObject.h:171
> +    RenderObject* previousInFlowSibling() const
> +    {
> +        RenderObject* current = previousSibling();
> +        while (current && current->isOutOfFlowPositioned())
> +            current = current->previousSibling();
> +        return current;
> +    }

I would not put this function on RenderObject. I'd put it on RenderBox and call it previousInFlowSiblingBox. Also, you need to check isFloating() as well, since you need to skip floats as well as positioned objects.
Comment 5 Robert Hogan 2013-08-20 11:59:34 PDT
Created attachment 209216 [details]
Patch
Comment 6 Dave Hyatt 2013-08-21 10:15:44 PDT
Comment on attachment 209216 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2013-08-21 11:02:02 PDT
Comment on attachment 209216 [details]
Patch

Clearing flags on attachment: 209216

Committed r154399: <http://trac.webkit.org/changeset/154399>
Comment 8 WebKit Commit Bot 2013-08-21 11:02:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Simon Fraser (smfr) 2013-08-21 11:57:08 PDT
This caused bug 120126.
Comment 10 WebKit Commit Bot 2013-08-21 12:39:54 PDT
Re-opened since this is blocked by bug 120129
Comment 11 Robert Hogan 2013-09-14 04:11:10 PDT
Created attachment 211641 [details]
Patch
Comment 12 Dave Hyatt 2013-09-17 11:40:50 PDT
Comment on attachment 211641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211641&action=review

r-

> Source/WebCore/rendering/RenderBlockFlow.cpp:643
> +LayoutUnit RenderBlockFlow::marginOffsetForSelfCollapsingBlock()
> +{
> +    RenderBlock* current = this;
> +    while (current && current->isSelfCollapsingBlock()) {
> +        if (current->style()->clear() && current->getClearDelta(current, LayoutUnit()))
> +            return current->collapsedMarginBeforeForChild(current);
> +        current = current->containingBlock();
> +    }
> +    return LayoutUnit();
> +}

Doesn't look right that you're crossing objects that could be block formatting contexts (e.g., positioned/floating ancestors). I still don't understand why this information isn't coming down properly. Having to crawl up through ancestors seems like the wrong approach.
Comment 13 Robert Hogan 2013-09-17 12:09:21 PDT
Comment on attachment 211641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211641&action=review

>> Source/WebCore/rendering/RenderBlockFlow.cpp:643
>> +}
> 
> Doesn't look right that you're crossing objects that could be block formatting contexts (e.g., positioned/floating ancestors). I still don't understand why this information isn't coming down properly. Having to crawl up through ancestors seems like the wrong approach.

In this situation we're laying out a float at the top of a self-collapsing block that's nested inside one or more self-collapsing blocks, the topmost of which has clearance with a preceding float. The clearance rules  mean that the margin top of this top-most self-collapsing block intrudes into the preceding float. 

So as we lay out a float that is contained in the self-collapsing block (in our case in another self-collapsing block nested inside it) it's the margin position of the topmost block we're interested in. It *has* been passed down to the float's direct parent, that's why the parent is positioning itself up at the margin-top position that intrudes into the float and that's why it will end up trying to position the float beside it unless we do something. 

What the parent doesn't know how much that margin top value is so that's why we have to go up to the ancestor to find it out and use it to temporarily give us the real bottom of the preceding float and place our own float below it.
Comment 14 Robert Hogan 2013-09-17 14:18:46 PDT
Created attachment 211939 [details]
Patch
Comment 15 Dave Hyatt 2013-09-18 12:24:51 PDT
Comment on attachment 211939 [details]
Patch

r- 

I still think this is too late. The logicalTop() of the blocks just needs to be set correctly. You're compensating after the fact for a mistake that was already made earlier. I think the bug is just in collapseMargins somewhere.
Comment 16 Robert Hogan 2013-10-19 05:48:57 PDT
Created attachment 214652 [details]
Patch
Comment 17 Robert Hogan 2013-10-21 10:43:54 PDT
Created attachment 214749 [details]
Patch
Comment 18 Dave Hyatt 2013-10-21 11:09:48 PDT
When we last talked about this, I thought we had worked out that the position of the self-collapsing block was just wrong, i.e., that a mistake had been made in collapseMargins. Are you saying you believe the position of the block is correct?

It's just not clear to me why you would have to compensate after-the-fact like this unless the position of the block was set to something wrong by mistake.
Comment 19 Dave Hyatt 2013-10-21 11:33:11 PDT
Comment on attachment 214749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214749&action=review

> Source/WebCore/rendering/RenderBlockFlow.cpp:821
> -            if (!marginInfo.canCollapseWithMarginBefore())
> +            if (!marginInfo.canCollapseWithMarginBefore() || (marginInfo.atBeforeSideOfBlock() && style()->clear() && getClearDelta(this, logicalTop)))

Minusing for this bit. collapseMargins is computing the logical top prior to clearance, so checking for clearance inside collapseMargins is a no-no. (Look at the call site of collapseMargins.)
Comment 20 Dave Hyatt 2013-10-21 11:53:55 PDT
I posted this in IRC, but my suggestion would be to put outlines on the elements. The outline of the child should paint over the outline of the parent, and this should expose the position of these self-collapsing blocks in a way that will make them easier to see.

In the example we talked about (the second test), the self-collapsing child should be at the same absolute position as its container, so its outline should paint over the parent. If it is ending up in a different place, then that's the bug you need to fix.

In general this patch is still trying to "correct" for a mispositioning as far as I can tell, and there are many ways to expose this mispositioning besides the ones you're trying to correct for.  Outlines, clipping, specified height of 0 with children, etc. are all ways to expose the wrong position besides just the positioning of floating children.

In other words if the bug is that the self-collapsing block is in the wrong place, all of these other things that depend on the position being correct should be malfunctioning too, so let's find that out.
Comment 21 Robert Hogan 2013-11-11 12:46:56 PST
Created attachment 216601 [details]
Patch
Comment 22 Robert Hogan 2013-11-11 12:59:33 PST
Created attachment 216603 [details]
Patch
Comment 23 EFL EWS Bot 2013-11-11 13:06:27 PST
Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22599449
Comment 24 EFL EWS Bot 2013-11-11 13:11:16 PST
Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/22369436
Comment 25 Build Bot 2013-11-11 13:20:08 PST
Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22589405
Comment 26 Build Bot 2013-11-11 13:29:00 PST
Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22719402
Comment 27 Build Bot 2013-11-11 13:31:14 PST
Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22369439
Comment 28 kov's GTK+ EWS bot 2013-11-11 13:36:07 PST
Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/22699526
Comment 29 Robert Hogan 2013-11-12 10:13:30 PST
Created attachment 216692 [details]
Patch
Comment 30 EFL EWS Bot 2013-11-12 10:23:50 PST
Comment on attachment 216692 [details]
Patch

Attachment 216692 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/21249765
Comment 31 Build Bot 2013-11-12 11:00:06 PST
Comment on attachment 216692 [details]
Patch

Attachment 216692 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22639742

New failing tests:
fast/block/margin-collapse/block-inside-inline/025.html
fast/block/margin-collapse/025.html
fast/block/float/024.html
accessibility/aria-help.html
fast/block/margin-collapse/empty-clear-blocks.html
Comment 32 Build Bot 2013-11-12 11:00:09 PST
Created attachment 216695 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 33 Dave Hyatt 2013-11-12 11:01:29 PST
Comment on attachment 216692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216692&action=review

> Source/WebCore/rendering/RenderBlockFlow.cpp:221
> -    if (parentHasFloats)
> +    if (parentHasFloats || parentBlock->lowestFloatLogicalBottom() > logicalTopOffset)

This feels a little weird to me. It seems like it could catch cases other than self-collapsing stuff. Did you mean for this to be qualified with isSelfCollapsingBlock perhaps? I'm wondering also if there's a way to just get parentHasFloats set correctly during the loop, but I can see the trickiness if you did not want to update prev.

> Source/WebCore/rendering/RenderBlockFlow.cpp:1183
> +    if (lastBlock && lastBlock->isRenderBlockFlow())

I think this would read better as:

if (lastBlock && lastBlock->isSelfCollapsingBlock())
Comment 34 Build Bot 2013-11-12 14:00:07 PST
Comment on attachment 216692 [details]
Patch

Attachment 216692 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22539750

New failing tests:
fast/block/margin-collapse/block-inside-inline/025.html
fast/block/float/024.html
fast/block/margin-collapse/025.html
fast/block/margin-collapse/empty-clear-blocks.html
Comment 35 Build Bot 2013-11-12 14:00:11 PST
Created attachment 216721 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 36 Build Bot 2013-11-12 14:39:30 PST
Comment on attachment 216692 [details]
Patch

Attachment 216692 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22539760

New failing tests:
fast/block/margin-collapse/block-inside-inline/025.html
fast/block/float/024.html
fast/block/margin-collapse/025.html
fast/block/margin-collapse/empty-clear-blocks.html
Comment 37 Build Bot 2013-11-12 14:39:33 PST
Created attachment 216727 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 38 Robert Hogan 2013-11-15 11:14:10 PST
Created attachment 217066 [details]
Patch
Comment 39 Dave Hyatt 2013-11-19 11:39:32 PST
Comment on attachment 217066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review

Something to possibly consider in a follow-up patch would be to add isSelfCollapsingBlock as a virtual function so that you don't have to ask isRenderBlockFlow and then cast to ask isSelfCollapsingBlock... would be nice to just be able to ask any child if they are self-collapsing block without having to call other methods first.

> Source/WebCore/rendering/RenderBlockFlow.cpp:221
> +    if (parentHasFloats || (parentBlock->lowestFloatLogicalBottom() > logicalTopOffset && prev && toRenderBlockFlow(prev)->isSelfCollapsingBlock()))

Minor quibble. I'd move prev && prev->isSelfCollapsingBlock before parentBlock->lowestFloatLogicalBottom() > logicalTopOffset.

> Source/WebCore/rendering/RenderBlockFlow.cpp:714
> +    ASSERT(isSelfCollapsingBlock());

Good call.
Comment 40 WebKit Commit Bot 2013-11-20 11:23:50 PST
Comment on attachment 217066 [details]
Patch

Clearing flags on attachment: 217066

Committed r159575: <http://trac.webkit.org/changeset/159575>
Comment 41 WebKit Commit Bot 2013-11-20 11:23:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Antti Koivisto 2013-11-20 17:03:53 PST
Comment on attachment 217066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review

> LayoutTests/platform/mac/fast/block/margin-collapse/empty-clear-blocks-expected.txt:43
> -        RenderText {#text} at (100,0) size 268x18
> -          text run at (100,0) width 268: "This text should be to the right of the float."
> +        RenderText {#text} at (0,0) size 268x18
> +          text run at (0,0) width 268: "This text should be to the right of the float."

This is wrong. Text now overlaps the float.

The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path.
Comment 43 Robert Hogan 2013-11-21 11:24:52 PST
(In reply to comment #42)
> (From update of attachment 217066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review
> 
> > LayoutTests/platform/mac/fast/block/margin-collapse/empty-clear-blocks-expected.txt:43
> > -        RenderText {#text} at (100,0) size 268x18
> > -          text run at (100,0) width 268: "This text should be to the right of the float."
> > +        RenderText {#text} at (0,0) size 268x18
> > +          text run at (0,0) width 268: "This text should be to the right of the float."
> 
> This is wrong. Text now overlaps the float.
> 
> The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path.

Ow. This doesn't happen in blink and I didn't notice it when porting the patch. We need to reset the line layout path away from simple when we add a float. I'll open a separate bug for this.
Comment 44 Robert Hogan 2013-11-21 11:32:38 PST
(In reply to comment #43)

> > The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path.
> 
> Ow. This doesn't happen in blink and I didn't notice it when porting the patch. We need to reset the line layout path away from simple when we add a float. I'll open a separate bug for this.

See https://bugs.webkit.org/show_bug.cgi?id=124728
Comment 45 Robert Hogan 2013-11-21 12:28:22 PST
(In reply to comment #42)
> (From update of attachment 217066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217066&action=review
> 
> > LayoutTests/platform/mac/fast/block/margin-collapse/empty-clear-blocks-expected.txt:43
> > -        RenderText {#text} at (100,0) size 268x18
> > -          text run at (100,0) width 268: "This text should be to the right of the float."
> > +        RenderText {#text} at (0,0) size 268x18
> > +          text run at (0,0) width 268: "This text should be to the right of the float."
> 
> This is wrong. Text now overlaps the float.
> 
> The test result was updated back in r159589 as this case now takes simple line path (after r159579) which produces the correct original result. However the bug from this patch remains and needs to be fixed. We also need a version of this test case that is forced to line box path.

OK, I now understand you. r159579 fixed the temporary regression caused by r159589. However from talking on IRC empty-clear-blocks.html doesn't work in the complex line layout path. So I'll look into that and see what I can do.