Implement the -webkit-margin-collapse properties correct rendering

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Bucur]

The -webkit-margin-collapse properties were implemented initially to emulate quirks mode for block containers. However, they should be valid for all the situations where margin collapsing can appear (e.g. between block siblings). The values are as follows:
collapse: keep the CSS2.1 box model behaviour for margin collapsing
discard: the margin and any other margin collapsing with it are discarded
separate: the margin will not collapse with another margin, like there's a non-zero distance between them

[Andrei Bucur]

Created attachment 186064 [details]
WIP

[Andrei Bucur]

The patch needs work like:
- see how I can minimize the use of extra bitfields
- see how it interacts with float clearance
- solve block directionality for SEPARATE as well

[WebKit Review Bot]

Attachment 186064 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/block/margin-collapse/webkit-margin-collapse-siblings.html', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/style/RenderStyle.h']" exit_code: 1
Source/WebCore/rendering/RenderBlock.cpp:2007:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderBlock.cpp:2028:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderBlock.cpp:2052:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderBlock.cpp:2054:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/RenderBlock.cpp:6897:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/rendering/RenderBlock.cpp:6907:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrei Bucur]

Also, I need to see how to reset the margin computed values when discarding. I guess I need to set them to 0 somehow.

[Build Bot]

Comment on attachment 186064 [details]
WIP

Attachment 186064 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16295867

New failing tests:
fast/block/margin-collapse/webkit-margin-collapse-siblings.html

[WebKit Review Bot]

Comment on attachment 186064 [details]
WIP

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

New failing tests:
fast/block/margin-collapse/webkit-margin-collapse-siblings.html

[Andrei Bucur]

Created attachment 186401 [details]
WIP v2

[Build Bot]

Comment on attachment 186401 [details]
WIP v2

Attachment 186401 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16374311

New failing tests:
fast/regions/text-region-split-vertical-rl.html
fast/regions/autoheight-allregions.html
fast/regions/text-region-split-after-resize.html
fast/regions/autoheight-secondregion-breakoutside.html
printing/css2.1/page-break-after-003.html
fast/regions/autoheight-middleregion.html

[Andrei Bucur]

(In reply to comment #8)
> (From update of attachment 186401 [details])
> Attachment 186401 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/16374311
> 
> New failing tests:
> fast/regions/text-region-split-vertical-rl.html
> fast/regions/autoheight-allregions.html
> fast/regions/text-region-split-after-resize.html
> fast/regions/autoheight-secondregion-breakoutside.html
> printing/css2.1/page-break-after-003.html
> fast/regions/autoheight-middleregion.html

This is because forced break after actually works now :).

[Dave Hyatt]

Comment on attachment 186401 [details]
WIP v2

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

r-

> Source/WebCore/rendering/RenderBlock.cpp:1966
> +    childDiscardMarginBefore = childDiscardMarginBefore || (childDiscardMarginAfter && child->isSelfCollapsingBlock());

I would store child->isSelfCollapsingBlock() in a local so that you don't have to call it multiple times.

> Source/WebCore/rendering/RenderBlock.cpp:2051
> +        // FIXME: Take writing direction into account for SEPARATE.
>          if (child->style()->marginBeforeCollapse() == MSEPARATE) {

Good observation. Go ahead and fix it! Just need helper methods.... mustSeparateMarginBeforeForChild... etc.

> Source/WebCore/rendering/RenderBlock.cpp:-2423
> -    // Do not allow a collapse if the margin-before-collapse style is set to SEPARATE.
> -    RenderStyle* childStyle = child->style();
> -    if (childStyle->marginBeforeCollapse() == MSEPARATE) {
> -        marginInfo.setAtBeforeSideOfBlock(false);
> -        marginInfo.clearMargin();
> -    }

Where did this code go?

> Source/WebCore/rendering/RenderBlock.h:474
> +            // FIXME: This function doesn't get called for table cells. Does it matter?

It doesn't matter.

[Andrei Bucur]

Created attachment 186860 [details]
Patch

[Andrei Bucur]

(In reply to comment #11)
> Created an attachment (id=186860) [details]
> Patch

This patch doesn't include the fix for the after forced break. This one will be submitted for https://bugs.webkit.org/show_bug.cgi?id=104944 .

[Andrei Bucur]

Created attachment 186894 [details]
Patch v2

I forgot some code commented out after an experiment. Fixed now.

[Andrei Bucur]

Created attachment 186898 [details]
Patch v3

Ooops... forgot a parameter.

[Dave Hyatt]

Comment on attachment 186898 [details]
Patch v3

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

r=me

> Source/WebCore/rendering/RenderBlock.cpp:6921
> +bool RenderBlock::mustDiscardMarginBeforeForChild(const RenderBox* child) const

You should probably assert that the child does not need a layout here, since you are asking for a RenderBlock bit that is set by the layout process.  Same for all four of these methods.

> Source/WebCore/rendering/RenderBlock.cpp:6927
> +    if (child->isHorizontalWritingMode() == isHorizontalWritingMode())
> +        return child->isRenderBlock() ? toRenderBlock(child)->mustDiscardMarginAfter() : (child->style()->marginAfterCollapse() == MDISCARD);
> +    return false;

This warrants commenting I think. It's pretty subtle that you just return false for orthogonal blocks. Really the only reason we do so is because the properties are only specified for "before" and "after". If we did implement physical properties correctly (and this is something I could see us doing), then you would need to check the correct physical side. For now this is correct, though, but I think it warrants a comment.

[Andrei Bucur]

Comment on attachment 186898 [details]
Patch v3

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

>> Source/WebCore/rendering/RenderBlock.cpp:6921
>> +bool RenderBlock::mustDiscardMarginBeforeForChild(const RenderBox* child) const
> 
> You should probably assert that the child does not need a layout here, since you are asking for a RenderBlock bit that is set by the layout process.  Same for all four of these methods.

I use this method to estimate the child top position pre-layout. See estimateLogicalTopPosition where I've removed the fix-me stating we should also take into account -webkit-margin-collapse. Not sure such an assert could work if I use these methods there.

[Mihai Maerean]

Comment on attachment 186898 [details]
Patch v3

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

> Source/WebCore/rendering/RenderBlock.cpp:6923
> +    if (!child->isWritingModeRoot())

You should assert that the "child" RenderBox is actually a child of "this".

[Andrei Bucur]

Created attachment 187101 [details]
Patch v4

[Andrei Bucur]

(In reply to comment #17)
> (From update of attachment 186898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186898&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:6923
> > +    if (!child->isWritingModeRoot())
> 
> You should assert that the "child" RenderBox is actually a child of "this".

I don't think an assertion is necessary. There are many "forChild" functions in WebKit's layout that don't have the assertion. In my opinion it would generate a lot of noise that's unnecessary because if such a mistake occurs it's easily spotted in a code review.

[Andrei Bucur]

(In reply to comment #18)
> Created an attachment (id=187101) [details]
> Patch v4

I've changed a bit how margin estimation is made before the layout for a child. I've stopped using the forChild methods in marginBeforeEstimateForChild() because they were relying on the rare data bits which are not cleared at that point.
I've added assertions only for selfNeedsLayout() because I need to use the flag value in estimateLogicalTopPosition().

[Dave Hyatt]

Comment on attachment 187101 [details]
Patch v4

r=me

[WebKit Review Bot]

Comment on attachment 187101 [details]
Patch v4

Clearing flags on attachment: 187101

Committed r142974: <http://trac.webkit.org/changeset/142974>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.