Bug 80394

Summary: CSS 2.1 failure: margin-collapse-clear-012 fails
Product: WebKit Reporter: Robert Hogan <robert>
Component: CSSAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, hyatt, jberlin, jchaffraix, robert, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 47141    
Attachments:
Description Flags
Reduction
none
Corrected Reduction
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-03
none
Patch
none
Patch
none
Patch hyatt: review+

Description Robert Hogan 2012-03-06 00:35:41 PST
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.
Comment 1 Robert Hogan 2012-03-06 00:36:16 PST
Created attachment 130316 [details]
Reduction
Comment 2 Robert Hogan 2012-03-06 05:10:41 PST
Created attachment 130359 [details]
Corrected Reduction
Comment 3 Robert Hogan 2012-03-07 01:13:43 PST
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.
Comment 4 Robert Hogan 2012-05-07 09:18:29 PDT
Created attachment 140536 [details]
Patch
Comment 5 Dave Hyatt 2012-05-21 11:03:08 PDT
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.
Comment 6 Robert Hogan 2012-07-02 11:35:35 PDT
Created attachment 150455 [details]
Patch
Comment 7 Robert Hogan 2012-07-22 09:34:35 PDT
Created attachment 153695 [details]
Patch
Comment 8 Robert Hogan 2012-07-22 12:19:02 PDT
Created attachment 153702 [details]
Patch
Comment 9 Dave Hyatt 2012-08-22 14:09:42 PDT
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.
Comment 10 Robert Hogan 2012-08-24 12:42:43 PDT
Created attachment 160478 [details]
Patch
Comment 11 Robert Hogan 2012-08-24 12:47:27 PDT
(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.
Comment 12 WebKit Review Bot 2012-08-24 13:44:26 PDT
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
Comment 13 WebKit Review Bot 2012-08-24 13:44:29 PDT
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
Comment 14 Robert Hogan 2012-08-24 14:36:01 PDT
Created attachment 160500 [details]
Patch
Comment 15 Dave Hyatt 2012-08-27 13:32:04 PDT
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.
Comment 16 Robert Hogan 2012-08-27 14:17:25 PDT
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.
Comment 17 Robert Hogan 2012-08-29 10:48:38 PDT
Created attachment 161267 [details]
Patch
Comment 18 Robert Hogan 2012-08-29 11:38:48 PDT
Created attachment 161276 [details]
Patch
Comment 19 Dave Hyatt 2012-08-29 12:01:38 PDT
Comment on attachment 161276 [details]
Patch

r=me
Comment 20 Robert Hogan 2012-08-30 11:15:44 PDT
Committed r127163: <http://trac.webkit.org/changeset/127163>
Comment 21 Jessie Berlin 2012-08-30 14:27:31 PDT
(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?
Comment 22 Jessie Berlin 2012-08-30 14:39:10 PDT
(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