616722 - -moz-column-count on parent element with border causes its margins to collapse with the kids' margins

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Reduced testcase

Not sure if this is related to bug 388666.

If you have a parent element (e.g. <div>) with '-moz-column-count' set to something higher than 0, the margins of the (last?) child element (e.g. <p>) will bleed through, even if the parent element's margins are 0.

This does not appear to happen with Safari (which, of course, uses '-webkit-column-count').

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, this seems like a bug.

Presumably the problem here is that the border ends up on the columnset, not on the columns themselves or something?

Comment 2

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Reduced testcase, with border tests

(In reply to comment #1)
> Yeah, this seems like a bug.
> 
> Presumably the problem here is that the border ends up on the columnset, not on
> the columns themselves or something?

I'm not sure that it is directly related to the borders, though. Removing them (and using, e.g., a background color to demarcate the two divs) actually makes things worse, as I understand it. Without the borders, the margins collapse on both divs.

Here's a new testcase that shows that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Without the borders, the margins collapse on both divs.

See <http://www.w3.org/TR/CSS2/box.html#collapsing-margins>.

Comment 4

[Gordon P. Hemsley [:GPHemsley]]

Comment on attachment 495268 [details]
Reduced testcase

(In reply to comment #3)
> > Without the borders, the margins collapse on both divs.
> 
> See <http://www.w3.org/TR/CSS2/box.html#collapsing-margins>.

Yeah, I thought I might've been mistaken. I take it you're referring to this part?

"The bottom margin of an in-flow block-level element with a 'height' of 'auto' is adjoining to its last in-flow block-level child's bottom margin if the element has no bottom padding or border."

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yep.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

Change nsBlockFrame::BlockIsMarginRoot() to return separate values
for top/bottom.  Currently there is a special handling for blocks where
the parent frame is a ColumnSetFrame that currently returns false
unconditionally -- make that return true if there is a non-zero
border+padding instead.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Here's an interesting edge case.  When the column is empty, (how) should the
margin self-collapse...  when there's one column? more than one column?

My patch makes the height of the one column case 20px, ie. like a normal
block would do.  The three column case has 0px height.  I would expect it
to be 20px.

Comment 8

[Daniel.S]

Bug 388666 is a different issue. It's about the margin-top of the first child of a multi column element that is not being rendered if the multi-column element has a border-top.

This issue is about the the margin-bottom of the last-child of a multi-column element (that has border-bottom) which apparently collapses with the margin-bottom of the multi-column element.

The spec says that "a multi-column element establishes a new block formatting context". See the bottom of section 2, The multi-column model:
http://www.w3.org/TR/2011/CR-css3-multicol-20110412/#the-multi-column-model

That means the margins of a multi-column element do not collapse with themselves nor with the margins of their descendants.

Gecko and WebKit apparently do not implement the spec. Microsoft claims to implement the spec in IE10 Preview 1.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Make ColumnSetFrame a block margin root.  Make the first column a margin root at the top edge, and the last column at the bottom.  (The 368020-1 test change is
to account for its margin which previously was ignored)

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=f0cde7c76d05

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 644313 [details] [diff] [review]
Patch rev. 2 (diff -w)

Review of attachment 644313 [details] [diff] [review]:
-----------------------------------------------------------------

::: inbound-opt.9b8e56ada40e/layout/generic/nsBlockFrame.cpp
@@ +7020,5 @@
>  }
>  
> +void
> +nsBlockFrame::IsMarginRoot(bool* aTopMarginRoot,
> +                                bool* aBottomMarginRoot)

Fix indent

Comment 13

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad2c23374bc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ad2c23374bc