Bug 99923

Summary: margin-top/bottom has no effect for child nodes of flex items
Product: WebKit Reporter: Shaofei Cheng <csf178>
Component: CSSAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jchaffraix, kennyluck, ojan, tabatkins, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62048    
Attachments:
Description Flags
reduced testcase with webkit prefixes
none
margin-collapsing without flexbox
none
margin-collapsing without flexbox ('overflow: hidden')
none
Patch none

Description Shaofei Cheng 2012-10-21 01:16:05 PDT
When set margin property to a child node of flex item, it does not effect.

See the test case here

https://test.csswg.org/shepherd/repository/tip/contributors/ttwf_bj/winter/submitted/flex-flexitem-childmargin.html
Comment 1 Ojan Vafai 2012-10-21 09:30:25 PDT
Created attachment 169804 [details]
reduced testcase with webkit prefixes
Comment 2 Ojan Vafai 2012-10-21 09:31:40 PDT
Presumably this is margin-collapsing doing the wrong thing. I'm not actually sure this is technically wrong though. margin-collapsing is just kind of crazy.
Comment 3 Ojan Vafai 2012-10-21 09:36:17 PDT
Created attachment 169805 [details]
margin-collapsing without flexbox
Comment 4 Ojan Vafai 2012-10-21 09:37:42 PDT
This looks to me as working as intended. Please reopen if I'm wrong. margin-collapsing is just very confusing and complicated. Tab, didn't you have some idea for limiting margin-collapsing that might simplify some of the more confusing situations (like this one)?
Comment 5 Kang-Hao (Kenny) Lu 2012-10-21 16:55:59 PDT
Created attachment 169818 [details]
margin-collapsing without flexbox ('overflow: hidden')

The spec says a flex container establishes a block formatting context and so I attached a version of the test case with 'overflow: hidden;' set on the corresponding element. An earlier version (two months ago?) of spec says each flex item establishes a BFC so the first example in my test case is set with such, which I consider the expected behavior.

But even without the reason above, setting the 'margin-top' of the element in question (the first child of a flex item) to, say, 20000em would not push the element out of the window. That is, I don't think the top margin of the element is collapsed with anything else. It is just always zero.

For what it's worth, both Opera Next and Firefox nightly work as expected but I don't have IE10 at hands.

So yes, I think this should be reopened. Unfortunately I don't have editbug.
Comment 6 Kang-Hao (Kenny) Lu 2012-10-21 16:57:25 PDT
(In reply to comment #5)
>  An earlier version (two months ago?) of spec says each flex item establishes a BFC so the first example in my test case is set with such, which I consider the expected behavior.

the second, I mean.
Comment 7 Ojan Vafai 2012-10-21 21:27:52 PDT
I think you're right. Reopening.
Comment 8 Ojan Vafai 2012-10-21 21:32:26 PDT
I think we just need to update RenderBlock::MarginInfo::MarginInfo to know about flexboxes.
Comment 9 Tab Atkins 2012-10-22 11:03:18 PDT
(In reply to comment #5)
> Created an attachment (id=169818) [details]
> margin-collapsing without flexbox ('overflow: hidden')
> 
> The spec says a flex container establishes a block formatting context and so I attached a version of the test case with 'overflow: hidden;' set on the corresponding element. An earlier version (two months ago?) of spec says each flex item establishes a BFC so the first example in my test case is set with such, which I consider the expected behavior.

Hm, I didn't intend to lose that text about the flex items being BFCs.  I'll bring it up on the list, I suspect it was an editting mistake.
Comment 10 Tab Atkins 2012-10-22 11:04:00 PDT
But yeah, Ojan, intention is definitely that children of a flex item do *not* collapse their margins with the flex item.  Flex item margins are intended to be fully independent.
Comment 11 Tony Chang 2012-10-22 16:19:52 PDT
Created attachment 170016 [details]
Patch
Comment 12 Tony Chang 2012-10-22 16:21:13 PDT
+julien, who may know other situations where a RenderObject doesn't have a parent.
Comment 13 Julien Chaffraix 2012-10-22 16:47:03 PDT
(In reply to comment #12)
> +julien, who may know other situations where a RenderObject doesn't have a parent.

Your check is right as the tree is stable and properly attached during layout.
Comment 14 WebKit Review Bot 2012-10-22 17:04:07 PDT
Comment on attachment 170016 [details]
Patch

Clearing flags on attachment: 170016

Committed r132164: <http://trac.webkit.org/changeset/132164>
Comment 15 WebKit Review Bot 2012-10-22 17:04:13 PDT
All reviewed patches have been landed.  Closing bug.