Bug 119979 – REGRESSION(r127163): Respect clearance set on ancestors when placing floats

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Robert Hogan]

Created attachment 209028 [details]
Reduction

..

[Robert Hogan]

Created attachment 209029 [details]
Patch

[Dave Hyatt]

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.

[Robert Hogan]

Created attachment 209127 [details]
Patch

[Dave Hyatt]

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.

[Robert Hogan]

Created attachment 209216 [details]
Patch

[Dave Hyatt]

Comment on attachment 209216 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 209216 [details]
Patch

Clearing flags on attachment: 209216

Committed r154399: <http://trac.webkit.org/changeset/154399>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This caused bug 120126.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 120129

[Robert Hogan]

Created attachment 211641 [details]
Patch

[Dave Hyatt]

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.

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 211939 [details]
Patch

[Dave Hyatt]

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.

[Robert Hogan]

Created attachment 214652 [details]
Patch

[Robert Hogan]

Created attachment 214749 [details]
Patch

[Dave Hyatt]

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.

[Dave Hyatt]

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.)

[Dave Hyatt]

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.

[Robert Hogan]

Created attachment 216601 [details]
Patch

[Robert Hogan]

Created attachment 216603 [details]
Patch

[EFL EWS Bot]

Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22599449

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 216603 [details]
Patch

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

[Build Bot]

Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22369439

[kov's GTK+ EWS bot]

Comment on attachment 216603 [details]
Patch

Attachment 216603 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/22699526

[Robert Hogan]

Created attachment 216692 [details]
Patch

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

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())

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Robert Hogan]

Created attachment 217066 [details]
Patch

[Dave Hyatt]

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.

[WebKit Commit Bot]

Comment on attachment 217066 [details]
Patch

Clearing flags on attachment: 217066

Committed r159575: <http://trac.webkit.org/changeset/159575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

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.

[Robert Hogan]

(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.

[Robert Hogan]

(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

[Robert Hogan]

(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.