Bug 108168 - Implement the -webkit-margin-collapse properties correct rendering
Summary: Implement the -webkit-margin-collapse properties correct rendering
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrei Bucur
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 104944
  Show dependency treegraph
 
Reported: 2013-01-29 01:24 PST by Andrei Bucur
Modified: 2013-02-15 02:24 PST (History)
9 users (show)

See Also:


Attachments
WIP (24.62 KB, patch)
2013-02-01 09:35 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
WIP v2 (45.39 KB, patch)
2013-02-04 09:34 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (59.35 KB, patch)
2013-02-06 08:26 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch v2 (59.34 KB, patch)
2013-02-06 12:31 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch v3 (59.34 KB, patch)
2013-02-06 12:39 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch v4 (61.36 KB, patch)
2013-02-07 07:15 PST, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Bucur 2013-01-29 01:24:47 PST
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
Comment 1 Andrei Bucur 2013-02-01 09:35:45 PST
Created attachment 186064 [details]
WIP
Comment 2 Andrei Bucur 2013-02-01 09:37:46 PST
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
Comment 3 WebKit Review Bot 2013-02-01 09:38:40 PST
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.
Comment 4 Andrei Bucur 2013-02-01 09:39:52 PST
Also, I need to see how to reset the margin computed values when discarding. I guess I need to set them to 0 somehow.
Comment 5 Build Bot 2013-02-01 11:33:04 PST
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
Comment 6 WebKit Review Bot 2013-02-01 12:11:20 PST
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
Comment 7 Andrei Bucur 2013-02-04 09:34:49 PST
Created attachment 186401 [details]
WIP v2
Comment 8 Build Bot 2013-02-04 13:11:18 PST
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
Comment 9 Andrei Bucur 2013-02-04 13:16:40 PST
(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 :).
Comment 10 Dave Hyatt 2013-02-04 13:48:03 PST
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.
Comment 11 Andrei Bucur 2013-02-06 08:26:56 PST
Created attachment 186860 [details]
Patch
Comment 12 Andrei Bucur 2013-02-06 08:30:04 PST
(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 .
Comment 13 Andrei Bucur 2013-02-06 12:31:35 PST
Created attachment 186894 [details]
Patch v2

I forgot some code commented out after an experiment. Fixed now.
Comment 14 Andrei Bucur 2013-02-06 12:39:30 PST
Created attachment 186898 [details]
Patch v3

Ooops... forgot a parameter.
Comment 15 Dave Hyatt 2013-02-06 13:33:24 PST
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.
Comment 16 Andrei Bucur 2013-02-07 04:28:04 PST
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.
Comment 17 Mihai Maerean 2013-02-07 05:17:10 PST
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".
Comment 18 Andrei Bucur 2013-02-07 07:15:12 PST
Created attachment 187101 [details]
Patch v4
Comment 19 Andrei Bucur 2013-02-07 07:24:14 PST
(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.
Comment 20 Andrei Bucur 2013-02-07 07:31:00 PST
(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().
Comment 21 Dave Hyatt 2013-02-14 11:34:28 PST
Comment on attachment 187101 [details]
Patch v4

r=me
Comment 22 WebKit Review Bot 2013-02-15 02:24:08 PST
Comment on attachment 187101 [details]
Patch v4

Clearing flags on attachment: 187101

Committed r142974: <http://trac.webkit.org/changeset/142974>
Comment 23 WebKit Review Bot 2013-02-15 02:24:14 PST
All reviewed patches have been landed.  Closing bug.