1163119 - top and bottom percent margins are not respected for flex items with a computed height

Status: RESOLVED DUPLICATE of bug 958714

(Core :: Layout, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150415140819

Steps to reproduce:

I created a series of tiles displayed in flex. They have an explicit width and a computed height. Left & Right margins are respected, Top & Bottom margins are ignored. 

http://codepen.io/romanp/pen/JdGRem


Actual results:

http://codepen.io/romanp/pen/JdGRem


Expected results:

top and bottom margins should be respected if there is a computed height.

Comment 1

[Gingerbread Man]

I don't see a problem. The margin is equal on all sides, as specified.

Can you reproduce the issue in a brand new profile?
https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles

If yes, please attach a screenshot of the issue.

Comment 2

[[e-mail]]

Attachment: Screen Shot 2015-05-10 at 5.56.53 PM.png

Here's a screenshot of the issue. Please try the Codepen again. Firefox is ignoring the computed heights of the elements and assigning 0 to the top and bottom margin values.

Comment 3

[Boris Zbarsky [:bzbarsky]]

That screenshot doesn't match what I'm seeing, in either a current nightly or Firefox 37.  In both of those I see the vertical spacing between the tiles ending up the same size as the horizontal spacing between them...

Comment 4

[[e-mail]]

Attachment: FirefoxBug.JPG

Please take a look at the screenshot showing the issue in the Codepen. There is no vertical margin between the tiles, because their height is computed based on width.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  Now I see what your screnshot shows at <http://codepen.io/romanp/pen/JdGRem>.  I wonder what changed.

Daniel, what's the story with vertical percentage margins on flex items?  What's the percentage base for them?  I thought it got changed from flex container width to flex container height or something...

Comment 6

[Daniel Holbert [:dholbert]]

Yup -- vertical percent margins/padding on flex items are resolved against the *height* of the flex container. If the flex container has auto-height (as it does here), they've got nothing to resolve against, so they end up at 0.

(This is different from blocks; on children of a block, horizontal *and* vertical percent margins/padding are resolved against the *width* of the container, which is always a definite value even with width:auto.)

Comment 7

[Daniel Holbert [:dholbert]]

This is the same issue discussed in this twitter conversation:
 https://twitter.com/CodingExon/status/446114954731458561
and in other duplicates of bug 958714.

Unfortunately, Chrome hasn't implemented this yet ( http://code.google.com/p/chromium/issues/detail?id=333533 ).  But they hopefully will soon.

There was CSSWG discussion last week (partly due to pushback from Chrome folks) about whether to revert the spec on this point, & make flexbox & block consistent. But they ended up resolving to keep the spec as-is, and my impression is that the Chrome folks will abide by that.

Start of discussion: http://logs.csswg.org/irc.w3.org/css/2015-05-19/#e554871
Resolution: http://logs.csswg.org/irc.w3.org/css/2015-05-19/#e555153

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to [e-mail] from comment #0)
> Expected results:
> 
> top and bottom margins should be respected if there is a computed height.

(To clarify this point from comment 0: it sounds like you're expecting the percent margin on an element to resolve against *that same element's* height value.

That's not what happens. Percent margins are resolved against the container. As discussed in comment 6: if the container is a block, the percent margin is always resolved against the block's width. But if the container is a flex container, then the percent margin is resolved against the width or height, depending on if it's a vertical or horizontal margin. And percentages against auto-height is resolved to 0.)

Comment 9

[[e-mail]]

@ dholbert It's hard to believe this is expected behavior on Firefox's part. Explorer, Opera, Safari, and Chrome respect the vertical and bottom margins of computed height elements in flex. How do you suggest one adds vertical margins to a computed flex child in Firefox? If it's not possible using conventional methods, something is wrong with Firefox.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to [e-mail] from comment #9)
> @ dholbert It's hard to believe this is expected behavior on Firefox's part.
> Explorer, Opera, Safari, and Chrome respect the vertical and bottom margins
> of computed height elements in flex.

Opera, Safari, and Chrome all share the same underlying implementation (flexbox implemented in WebKit which was forked as Blink). Their behavior is a bug, and it's unsurprising that they share this bug, because they share the same rendering engine (basically).

The current IE release came out before this spec-change was made. Microsoft's next browser, "Edge", matches Firefox on this. Chrome etc. presumably will as well, once they fix the bug I linked to above.

> How do you suggest one adds vertical
> margins to a computed flex child in Firefox?

Use pixel-valued margins, instead of percent-valued margins? Or if you want them to be a percent, then give the flex container a fixed height so that the percent has something definite to resolve against.

> If it's not possible using
> conventional methods, something is wrong with Firefox.

I'm sorry you feel that way. Our implementation matches the spec, in any case, and I believe the Blink team has committed to changing to match as well (and as noted above, MS Edge already does too), so you'll have to be upset with all of us, not just Firefox. :)

Comment 11

[[e-mail]]

Fluid design requires percentages in layouts, not pixels, and this includes vertically computed items. Can you explain to me why the spec would ignore vertical percentage margins (but not pixel ones) when dealing with a computed height?

Comment 12

[Boris Zbarsky [:bzbarsky]]

The problem is that if the height is not specified then it depends on the vertical margins, no?  And if those vertical margins themselves depend on the height, then what is the correct height?

For block layout this is solved in a somewhat bizarre way: you use the _width_ as the percentage base for the vertical margins.  So the width is a known input, then you compute your margins, then you figure out the height.

For flex layout, the working group decided to use the height as the percentage base because for cases in which the height _is_ known this makes a lot more sense for the use cases flex is used for.  But that does mean that when your flex container's height is not fixed you can't define percentage vertical margins on the flex items.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Not doing reviews right now from comment #12)
> The problem is that if the height is not specified then it depends on the
> vertical margins, no?  And if those vertical margins themselves depend on
> the height, then what is the correct height?

This, precisely. Put another way: there's a circular dependency in your codepen, between the height:auto container on the one hand (whose height depends on its children) & the vertical % margin values on its children (which are supposed to be resolved against the container's height -- but the container's height depends on them!).

CSS resolves such circular dependencies by making vertical percentages behave like "auto" when their percent basis depends on them.  In this case, it means vertical margins on a flex item, this means those percentages end up at 0.

RE your question about "why the spec would do this" (and differ from block margins), here's a brief history:
 - During the development of the CSS *Grid* spec, the working group got feedback from webapp developers who were using grid -- these webapp developers were confused about why *vertical* percent margins & padding were being resolved against the container's *width*. They expected it to resolve against the height, as it does in other (non-web) UI toolkits.
 - In response to this feedback, the working group decided to change the Grid spec to make percent margin & padding apply against the same dimension (so, vertical margin/padding applies against height).
 - Since CSS Grid & CSS Flexbox share a lot of characteristics and it's nice for them to be consistent, the working group made the same change for flexbox.
 - This has the unfortunate side effect that vertical percentages now only resolve to something nonzero if the container has a specified height (and it does not, in your case).

The working group has received some pushback on this, but they've resolved to keep this the way it is. (in part based on the fact that the innitial change was to address feedback from webapp developers as well.)

Comment 14

[winston]

Just a quick note to say that as of today Chrome hasn't fallen in line, but I'm glad this thread existed to explain the circular dependency and Mozilla's adherence to the spec. Respect.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to winston from comment #14)
> Just a quick note to say that as of today Chrome hasn't fallen in line

Actually, a few months ago, Microsoft and Mozilla (and then the CSS Working group) ended up having to adopt the Chrome behavior on this, for compatibility reasons. See bug 958714 comment 50 and later. (note that this bug is marked as a duplicate of that bug)

We'll be shipping this change in Firefox 60 (currently in our beta channel).

> I'm glad this thread existed to explain the circular dependency and
> Mozilla's adherence to the spec. Respect.

Thanks!