Bug 80394 – CSS 2.1 failure: margin-collapse-clear-012 fails

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Robert Hogan]

At least one issue revealed by this test is that the top-margin of a child does not contribute height to the parent when computing clearance.

[Robert Hogan]

Created attachment 130316 [details]
Reduction

[Robert Hogan]

Created attachment 130359 [details]
Corrected Reduction

[Robert Hogan]

When a self-collapsing block has margin-top and needs to clear a float, the clearance calculated is the height of the float minus the margin-top and this should get added to the height of the parent. If the element has a margin-bottom (and it's the last element in the block) then this needs to get added to the height of the parent. 

At the moment, the clearance calculated for a self-collapsing block is the top of the block minus its bottom margin and this is what gets added to the height of the parent in clearFloatsIfNeeded(). The bottom margin of the last block is not getting added to the height of the parent.

[Robert Hogan]

Created attachment 140536 [details]
Patch

[Dave Hyatt]

Comment on attachment 140536 [details]
Patch

Looks like you have an incorrect rendering in

http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/block/margin-collapse/empty-clear-blocks-expected.png#L0

Also, it makes no sense to me that you'd only remove the block-inside-inline version of a test (025.html), which should be the same as the normal block version of the test.

[Robert Hogan]

Created attachment 150455 [details]
Patch

[Robert Hogan]

Created attachment 153695 [details]
Patch

[Robert Hogan]

Created attachment 153702 [details]
Patch

[Dave Hyatt]

Comment on attachment 153702 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2061
> +        marginInfo.setCanCollapseMarginAfterWithChildren(false);

This line seems wrong to me. What if another sibling is encountered? You've permanently turned off collapsing the parent with the child. If another normal block child comes along after this self-collapsing one, you'll mess up because you set this bit, won't you?

That's the reason for the lookahead to figure out if we're really at the bottom of the block.. the lookahead you removed.

Basically to fix the bug it seems like all you had to fix was the setLogicalHeight line, no? The other code was fine.

[Robert Hogan]

Created attachment 160478 [details]
Patch

[Robert Hogan]

(In reply to comment #9)
> (From update of attachment 153702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153702&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2061
> > +        marginInfo.setCanCollapseMarginAfterWithChildren(false);
> 
> This line seems wrong to me. What if another sibling is encountered? You've permanently turned off collapsing the parent with the child. If another normal block child comes along after this self-collapsing one, you'll mess up because you set this bit, won't you?

Indeed I will - I've fixed this and added a test - margin-collapse-top-margin-clearance-with-sibling.html.

> 
> That's the reason for the lookahead to figure out if we're really at the bottom of the block.. the lookahead you removed.
> 
> Basically to fix the bug it seems like all you had to fix was the setLogicalHeight line, no? The other code was fine.

No I still have to fix the other code and collapse the margins of the self-collapsing block together and with any subsequent self-collapsing blocks, but avoid collapsing them with the parent if no normal block child intervenes.

[WebKit Review Bot]

Comment on attachment 160478 [details]
Patch

Attachment 160478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13592317

New failing tests:
fast/css/margin-collapse-013-reduction.html
fast/css/margin-collapse-top-margin-clearance.html

[WebKit Review Bot]

Created attachment 160495 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Robert Hogan]

Created attachment 160500 [details]
Patch

[Dave Hyatt]

Comment on attachment 160500 [details]
Patch

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

Couple of comments:

> Source/WebCore/rendering/RenderBlock.cpp:2059
> +        bool wouldCollapseMarginsWithParent = true;

Minor optimization, but it seems like you could initialize this to canCollapseMarginAfterWithChildren. That way if the bit is already set to false, you won't waste time doing the lookahead.

> Source/WebCore/rendering/RenderBlock.cpp:2089
> +    LayoutUnit logicalTop = yPos + heightIncrease;
> +    // After margin collapsing, one of our floats may now intrude into the child.
> +    if (containsFloats() && child->isRenderBlock() && lowestFloatLogicalBottom() > logicalTop)
> +        toRenderBlock(child)->addIntrudingFloats(this, logicalLeftOffsetForContent(), logicalTop);

I'm not following this change. It doesn't seem like it should be necessary.

[Robert Hogan]

Comment on attachment 160500 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:2089
>> +        toRenderBlock(child)->addIntrudingFloats(this, logicalLeftOffsetForContent(), logicalTop);
> 
> I'm not following this change. It doesn't seem like it should be necessary.

The new clearance calculation combined with margin-collapsing can result in the child moving up past the bottom of the lowest float. If that happens it will need to avoid it. This is tested in the third paragraph of 'empty-clear-blocks.html' below - without it the text overlaps the float rather than avoiding it and moving to the right.

[Robert Hogan]

Created attachment 161267 [details]
Patch

[Robert Hogan]

Created attachment 161276 [details]
Patch

[Dave Hyatt]

Comment on attachment 161276 [details]
Patch

r=me

[Robert Hogan]

Committed r127163: <http://trac.webkit.org/changeset/127163>

[Jessie Berlin]

(In reply to comment #20)
> Committed r127163: <http://trac.webkit.org/changeset/127163>

This started causing failures on Apple Mountain Lion

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r127166%20(431)/results.html

I know you added a line to the platform/mac/TestExpectations file, but you said you expected IMAGE+TEXT, whereas the actual result is only TEXT. Was that a mistake?

[Jessie Berlin]

(In reply to comment #21)
> (In reply to comment #20)
> > Committed r127163: <http://trac.webkit.org/changeset/127163>
> 
> This started causing failures on Apple Mountain Lion
> 
> http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r127166%20(431)/results.html
> 
> I know you added a line to the platform/mac/TestExpectations file, but you said you expected IMAGE+TEXT, whereas the actual result is only TEXT. Was that a mistake?

Changed to expect only TEXT in order to get the bots greener. Let me know if this was in error. http://trac.webkit.org/changeset/127192