Closed Bug 958714 Opened 10 years ago Closed 6 years ago

Flex items treat percentage padding & margin values in vertical axis as 0, if the flex container lacks an explicit height (was allowed by spec, but perhaps not interoperable?)

Categories

(Core :: Layout, defect)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: thetallweeks, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webcompat])

Attachments

(7 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

http://codepen.io/thetallweeks/pen/kEqwp

I created 1 flex container with 2 divs that have padding-bottom: 25%;

Then I created another flex container with 2 divs that have padding-bottom: 200px;

Lastly is an example of what should occur (using floats instead of flexbox).

I have tested this in Firefox 26, 28, and 29.


Actual results:

The % padding is ignored, while the pixel-value padding is used.


Expected results:

The divs should use a padding-bottom value equal to 25% of the width of their containers. This allows the divs to scale up or down as the window resizes while maintaining a specific aspect ratio.
Component: Untriaged → Layout
Product: Firefox → Core
Version: 29 Branch → Trunk
Attached file reporter's testcase
Attached file reduced testcase 1 (obsolete) —
Assignee: nobody → dholbert
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8358762 [details]
reduced testcase 1

Sorry, my "reduced testcase 1" is actually incorrect (though Chrome "passes" it).

For blocks in CSS, % padding values are always resolved against the containing block width -- but in flexbox, they're resolved against the length of the containing block's corresponding dimension (width or height).  Spec reference:
  http://dev.w3.org/csswg/css-flexbox/#item-margins

(I implemented this change in bug 851379.)

Given that the reporter's original testcase has no specified height on the flex container, it makes sense that the % vertical padding would resolve to 0 there.
Attachment #8358762 - Attachment is obsolete: true
Here's a testcase that demonstrates how this actually works.
Here's the reporter's original testcase again, tweaked in the following ways:
 - Removed all but the top flex container (the actual thing being tested), for simplicity
 - Gave a height to the flex container, so that the percent height has something to resolve against. (which it needs for it to be meaningful, per spec link in comment 3)
 - Made the flex container "align-items: flex-start", because otherwise the flex item just stretches up to the container's height regardless of its padding.

With these tweaks, you can see the 25% padding taking effect, in Firefox. The gray "test" regions are the height of the text, plus 25% of the height of the flex container.  (In Chrome, they're much taller, because Chrome apparently doesn't honor the spec chunk quoted in comment 3.)
Resolving as INVALID because our behavior matches the spec.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Comment on attachment 8358771 [details]
testcase demonstrating this working

>    .child {
>      padding-bottom: 50%; /* Applies to width of containing block */

(Sorry, disregard that -------^ incorrect comment in the testcase's source. That was left over from my original version of the testcase, before I remembered that percent padding in flexboxes is special.)
I filed http://code.google.com/p/chromium/issues/detail?id=333533 on this, to poke the Chrome folks to fix this.
Depends on: 851379
I also filed a WebKit bug, for good measure, since this affects them, too:
 https://bugs.webkit.org/show_bug.cgi?id=126809
Summary: Flexbox items do not accept percentage padding values → Flexbox items treat percentage padding values in vertical axis as 0, if the container lacks an explicit height
FWIW - the chrome (bug/feature) is actually useful.  this is not.
If you're frustrated with the currently-specced behavior (as implemented by Firefox), please take it up with the W3C / CSSWG -- not by making comments on Firefox bugs.

Though your case has been made on the www-style mailing list; see:
 http://lists.w3.org/Archives/Public/www-style/2014Apr/0193.html
and other messages in that thread.

The response from spec editor is here:
 http://lists.w3.org/Archives/Public/www-style/2014May/0015.html
...which I think makes it pretty clear that the spec isn't going to change on this.
Summary: Flexbox items treat percentage padding values in vertical axis as 0, if the container lacks an explicit height → Flex items treat percentage padding & margin values in vertical axis as 0, if the flex container lacks an explicit height
While Firefox does seem to implement the correct behavior, somewhat, vertical margins/paddings on flex items with explicit height still computes to zero in the latest nightly.

Also, as long as an element has any height (because of content, stretch, etc.), percentage padding/margin should be computed from that height, no? The spec doesn't say "always zero"...

Cheers!
(In reply to Bogdan Gribincea from comment #18)
> While Firefox does seem to implement the correct behavior, somewhat,
> vertical margins/paddings on flex items with explicit height still computes
> to zero in the latest nightly.

Not in e.g. this testcase:
data:text/html,<div%20style="display:flex;height:100px"><div%20style="margin-top:50%">hi

If you're really seeing this behavior, could you file a new bug (with a testcase demonstrating the problem) and CC me?
(Note that a flex item's percent margin/padding is supposed to resolve against the *flex container's* explicit size, not against the *item's* size. Maybe that's the source of confusion?)
Thank you for your reply.

I didn't find anything in the spec that mentions if the width/height taken into account is the container or the item so yes that did confuse me. Indeed it works fine with explicit height set on the container.

Another thing that confuses me about this is the following:
- a container with flex-direction: row with no explicit width set will respect horizontal percentage margins on the flex items and set it to that percentage of the container width.
- a container with flex-direction: column with no explicit height set will treat vertical percentage margins as zero on it's flex items.

Is this intentional and per spec?

Example: http://jsfiddle.net/npLzhgty/1/

Cheers,
Bogdan
In your jsfiddle, the flex container's width is effectively explicit, because it's a block-level element with "width:auto", and its parent (the body) is a block.  Its parent's block layout gives it the full available width. (That's how "width:auto" block-level elements behave in block layout.)

But you're onto something, & here's a more interesting example, where the flex container's width depends on the children (via "inline-flex"):
http://jsfiddle.net/npLzhgty/3/

In that case, we do seem to be inconsistent. With the horizontal container, we size the container based on the children's intrinsic sizes, and then we resolve the items' percent margins as a percent of the container's now-resolved width. This means the children have not-quite-enough-space and are forced to wrap. Whereas with the vertical container, we resolve the percent vertical margins to 0.

While this is inconsistent, I don't think it violates the spec -- CSS2.1 says about percent margins: "If the containing block's width depends on this element [the element with a percent margin], then the resulting layout is undefined in CSS 2.1."
 http://www.w3.org/TR/CSS2/box.html#margin-properties
We've apparently chosen (presumably for historical reasons) to define that layout such that these horizontal percent margin values do resolve against the container's computed width (after the container has been sized to its contents) -- both in block layout and in flexbox layout.

In contrast, CSS is more explicit about vertical percent-values (e.g. "height") being explicitly invalid when resolved against a container whose height depends on its children -- e.g. the "height" property says percent values are treated as "auto" if the container's height depends on this element:
 http://www.w3.org/TR/CSS2/visudet.html#propdef-height
I filed bug 1110305 on the inconsistency described in comment 22. Let's take any subsequent discussion on that topic over there.
See Also: → 1110305
For the record, the situation here with the spec has changed slightly -- the spec now allows either behavior (instead of explicitly requiring Edge & Firefox's current behavior).

That spec change was here: https://hg.csswg.org/drafts/rev/86681ff9c4e9
Link to this section in current spec draft: https://drafts.csswg.org/css-flexbox/#item-margins

(I don't think this changes the status of this bug, though, for the time being.)
Hey Daniel,
thanks a lot for your feedback on this issue.
Considering the spec update, could you please elaborate on how the status of this issue still invalid?

Thank you in advance.
Flags: needinfo?(dholbert)
I suppose I'll reopen and clarify the summary.

(I'm not sure we'd take a patch to change this behavior -- our current behavior does still match the spec.  We should probably come to an agreement with other implementors and the CSSWG before / around when this behavior changes, if it changes.)
Flags: needinfo?(dholbert)
Summary: Flex items treat percentage padding & margin values in vertical axis as 0, if the flex container lacks an explicit height → Flex items treat percentage padding & margin values in vertical axis as 0, if the flex container lacks an explicit height (allowed by current spec, but perhaps not interoperable?)
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Thanks Daniel, that's appreciated. So just to make sure I understand, the spec now has changed to accommodate both cases, adding:

1. their own axis (left/right percentages resolve against width, top/bottom resolve against height);
2. the inline axis (left/right/top/bottom percentages all resolve against width);

So in opposition to mislabelling as a bug, since the behaviour on Firefox clearly follows (1), wouldn't the possibility (2) then be lacking implementation in this case? — possibly adapting to the behaviour found in some other vendors?

Please forgive me if I am missing something.

Thanks again.
> wouldn't the possibility (2) then be lacking implementation in this case?

I'm not sure what you mean. Are you saying we should implement *both* behaviors? That's not what the spec is saying -- it currently allows browsers to implement *either* behavior.
> possibly adapting to the behaviour found in some other vendors?

(If this was the crux of your question: we might do that at some point, though I'm not sure, per my parenthesized comments at the end of comment 28.)
Thanks Daniel,
I didn't realise it was either one of the alternative, but now that you've mentioned, it makes sense since one would nullify the other.
It's interesting how two alternatives that would result in different behaviours can be accepted isn't it?
Anyway, hopefully we will all get to an agreement in this regard soon.

Thanks for your support and all the best.
It's my first time playing around with flex and I also stumbled into this browser inconsistency.
Using padding to assume a certain aspect ratio has worked well for me for years, till I tried using it within a flexbox. FF's current implementation throws that behavior - I am and probably others are familiar with - out of the window. At this point, it also differs from both chrome's and IE's implementation, making it harder to make it work properly crossbrowser.

I hope we can see a switch from 1 to 2 mentioned in comment 29.
Blocks: 1308517
Just run into this issue and wanted to add my perspective, even if it isn't novel to this thread.  To cut to the chase, I also believe FF should implement the 2nd option in comment 29.  

My particular use case is a grid of flex items with percent-based widths, and percent-based vertical and horizontal margins to ensure that a) the rows take up the full container width and b) the vertical and horizontal gutters are the same size.  Intuitively, based on float and inline-block behavior, I believed this strategy would work for flex-items.  The margins were calculated as I expected on Chrome and IE11, but I was surprised (and ended up on this thread) when the vertical margin mysteriously collapsed in FF.  

Many thanks!
Flags: webcompat?
Whiteboard: [webcompat]
The attached MozReview patch should be the minimal change that would be necessary to make us match Chrome (and restore the legacy block-layout behavior, for grid and flex items).

There's a bit of additional cleanup that can be done (which I'll do in an additional patch), and we'll need to update tests as well.

Mats, would you be OK with this?  From a theoretical perspective, I very slightly prefer our current (originally-spec-mandated) behavior, but given the amount of webcompat pain this has caused (and anecdotal reports from Chrome implementors that they haven't gotten complaints about their behavior), I personally think we should just bite the bullet and restore the legacy behavior for percent-handling here.

Spec links (with permissive "do whichever behavior you want" text):
  https://drafts.csswg.org/css-grid-1/#item-margins
  https://drafts.csswg.org/css-flexbox-1/#item-margins
Flags: needinfo?(mats)
The last update on the Chromium bug was in 2014. I added the Hotlist-Interop flag.
https://bugs.chromium.org/p/chromium/issues/detail?id=333533#c7
Personally, I also prefer our current (originally spec-mandated) behavior.
Both Firefox and Edge implements it.  It's clearly more logical to resolve
vertical %-units against the vertical measure IMO, especially for Grid
which is symmetrical in both axes.

It's quite clear to me that Chrome never intended to implement what the spec
said in the first place and just waited for the web to become so dependent on
the buggy layout in Chrome to establish a de-facto standard.  Predictably,
they then subverted the CSS specs to say "do whatever".

I don't feel that strongly about this issue from a technical point of view,
but using their dominant market position to subvert standards in this way
is despicable, IMHO.

Are the Edge devs willing to change?  If so, it's OK with me I guess.
If not, I tend to think that we together should argue for restoring
the original spec language again in the CSSWG.
Flags: needinfo?(mats)
I've been meaning to comment on this bug in light of the new webcompat cases popping up every few months.

tl;dr I concur with :mats conclusions, reasoning, and concerns about bowing to a standards-breaking pressure from a dominant market position.

That being said, the steady drip of webcompat cases seem to imply increasing pain being felt by webdevs and Firefox users of those sites, which eventually (when?) may provide sufficient justification to implement bugwards compat.

:karlcow - I'm curious about the rate of net growth (or shrinkage) of the webcompat cases. While we're seeing new cases, are old cases going away (e.g. webdevs giving up on using this proportional layout hack), or staying around?

Would changes in Firefox marketshare affect our thinking here at all? (to push for "the right thing" vs. giving up and doing the "practical thing"?), and if so, is there a marketshare threshold where we would argue in the CSSWG to restore the previous spec text (that we and Edge comply with), and then once again resolve this bug as invalid?

The Blink folks appear to be silent on this in their bug in the past few years: https://bugs.chromium.org/p/chromium/issues/detail?id=333533 when they said "unclear when/if we'll fix".

However the WebKit folks seem to be open to fixing their implementation to comply with the originally spec'd behavior (compat with Firefox and Edge) when they last said: https://bugs.webkit.org/show_bug.cgi?id=113519 "This should be an easy change to make."

:dholbert - would appreciate your latest summary opinions on this, as I'm definitely open to considering different options based on the answers to these questions.
Perhaps the spec is ill-conceived and needs an update based on real world demands. It's normal to create height values for elements based on width values (for example, padding-top: 50%). Why should Flexbox be an exception?
it seems there are feelings about the implementation here - specifically that firefox's imeplementation is "the right thing" - meaning chrome's must be the wrong thing.

chrome's implementation is A) now aligned with the spec.  B) more useful to web devs, and thus C) more useful to end users.

insisting on persisting firefox's current approach feels legalistic and petty ("we were right first!") etc.
fwiw Currently, Chrome team is working closely with us and is willing to adjust things, when they can too. Specifically with the good efforts of Rick Byers and Philip Jägenstedt (both at Google). So I would be careful when pointing fingers. 


Also note that Web compatibility team is knowing

1. What is being reported as bugs (we do not have an exhaustive knowledge of all bugs out there)
2. What is breaking, but not what is not breaking.

The point 2. is important to understand. Let's say we convince the Chrome team to change the behavior and we suddenly realize that it is breaking more sites than ever expected. It means we create a situation where:

- browser implementers have to contact web sites to fix them
- "old" (out of contract from the web agency) sites will never be fixed (because not maintained)


I will ask Rick/Philip what they are thinking about it.


For tantek,

> :karlcow - I'm curious about the rate of net growth (or shrinkage) of the webcompat cases. While we're seeing new cases, are old cases going away (e.g. webdevs giving up on using this proportional layout hack), or staying around?

I would say there is not enough volume of data vs time to have a meaningful opinion about that. Also CSS and often webpages are not dated. So the discovery doesn't necessary match the creation time of the website issue.
The real bug on Chromium is 
https://bugs.chromium.org/p/chromium/issues/detail?id=229568
and people are advocating for a fix.
(In reply to Tantek Çelik from comment #42)
> :dholbert - would appreciate your latest summary opinions on this, as I'm
> definitely open to considering different options based on the answers to
> these questions.

I don't have strong feelings about this, except that I want to address the webcompat pain one way or another, before another 4 years elapse (that's how long it's been since this bug's been filed).  There has been a steady trickle of webcompat issues coming in due to this interop issue, over time; and unfortunately, now that the spec's been made to be more wishy-washy about this, we can't quite take the high ground and point affected web-devs to the spec and say it *requires* our current behavior anymore.

Here are my takeaways from prior discussions with CSSWG/other-flex-implementors about this issue:

Points supporting the Chrome/Safari behavior (resolve-all-percent-margin-and-padding-against-ISize):
 1. There's a longstanding hack (in block layout) for faking a responsive aspect-ratio by using use percent padding & needing it to be resolved against the parent's width (er, ISize).  This works in all browsers in block layout, and webdevs have observed that this hack also works in flexbox *in Chrome and Safari*, and in some cases have come to depend on that (hence, webcompat issues).  Importantly, we don't currently have a good alternative to offer them to get this aspect-ratio effect, IIUC.  An "aspect-ratio" CSS property has been tossed around, but doesn't exist yet.  So I don't think we have any good "don't do this, instead do that" recommendations that we can make for the affected sites.
 2. The most intuitive use-case for margin/padding on flex items is to achieve some consistent spacing between items (e.g. in a grid of tiles, implemented via multiline flexbox or via an auto-layout grid).  And to get that consistent-spacing result, it makes sense that "padding: 5%" would apply the same spacing regardless of dimension. (Authors could just as easily use em-units or px-units, but they might want to use percent for responsiveness or somethingorother.)
 3. According to Chrome's developers, they haven't gotten many (any?) web developers asking for the Firefox behavior, aside from the you-should-match-the-spec-for-the-spec's-sake bug report, so they're unaware of real-world use-cases/demand for the originally-specced behavior.  (This point is tricky to reason about, though, because Chrome's dominant market-position means websites get tested in Chrome primarily, so it's less likely that websites will get written to expect any other behavior besides Chrome's.)
 4. Any solution which falls back to resolving to 0 in common cases is surprising/problematic. (And the Firefox/Edge behavior does fall back to resolving to 0 in common cases -- specifically for block-axis percentages in an auto-height container, but not for inline-axis percentages in an auto-width container.  [except maybe when it's floated/intrinsically-sized -- I forget])

Points supporting the Firefox/Edge behavior (resolve-percent-margin-and-padding-against-same-axis-size):
 A. flex/grid try to be axis-independent to the extent possible, and this achieves that goal & is nicely intuitive from a symmetry point of view.
 B. Native-app toolkits have this behavior (resolving vertical percent paddings against height, etc.), and we don't want to confuse or scare away native app developers when they try to write webapps. (Microsoft cares especially about this since they're using HTML/CSS/JS for "native" apps/tiles in Win10.)

And finally, one point supporting nihilism/it-doesn't-matter:
 i. Percent margin/padding is not used a ton (in flex at least) anyway -- so to some extent, this is all theoretical and it probably doesn't matter a ton, as long as we've got interop.

Bottom line, for me: if the Chrome/WebKit folks are willing to change, great -- but the last I heard from the Chrome flexbox implementors about this, they were strongly resistant to changing (for the reasons "1-4" & "i" stated above, plus inertia probably).  Hence, I'd marginally lean towards just matching their behavior, for the same reasons (minus inertia, plus webcompat wins).
Thanks dholbert.

The CSSWG is discussing this again in a new issue: https://github.com/w3c/csswg-drafts/issues/2085

tl;dr. I think dholbert's points supporting Firefox/Edge behavior (esp. native-app toolkits), and Karl's points about working with Rick/Philip are compelling enough to stick with the symmetrical solution and figure out a path forward together.
Update: Microsoft announced at the CSSWG telcon this morning that they're going to implement the Blink/Webkit behavior for compat reasons and ship it in the next version of Edge.

Details in the discussion on https://github.com/w3c/csswg-drafts/issues/2085
(In reply to Mats Palmgren (:mats) from comment #41)
> Personally, I also prefer our current (originally spec-mandated) behavior.
> Both Firefox and Edge implements it.
[...]
> Are the Edge devs willing to change?  If so, it's OK with me I guess.

Per comment 50, the Edge devs decided to change (leaving us as the only engine with our behavior, once their change ships).  So, let's go ahead with the change.

--> Patch stack posted. I still need to update tests accordingly (which I'll probably do in "part 1", which is the only part that changes behavior; the others are taking opportunities for simplification/clarification).
Comment on attachment 8895604 [details]
Bug 958714 part 1: Remove special case for flex & grid items' percent block-axis margin/padding resolution, to align with other browsers.

https://reviewboard.mozilla.org/r/166818/#review221756
Attachment #8895604 - Flags: review?(mats) → review+
Comment on attachment 8945580 [details]
Bug 958714 part 2: Simplify percent-margin/padding resolution code to pass around a single length as the percent basis.

https://reviewboard.mozilla.org/r/215710/#review221764

::: layout/generic/ReflowInput.cpp:178
(Diff revision 1)
>    LogicalSize cbSize(aContainingBlockWritingMode, aContainingBlockISize,
>                       aContainingBlockISize);
>    ReflowInputFlags flags;
> -  InitOffsets(aContainingBlockWritingMode, cbSize, mFrame->Type(), flags);
> +  InitOffsets(aContainingBlockWritingMode,
> +              cbSize.ISize(aContainingBlockWritingMode),
> +              mFrame->Type(), flags);

This can be simplified by removing cbSize and using aContainingBlockISize directly.
Attachment #8945580 - Flags: review?(mats) → review+
Comment on attachment 8945581 [details]
Bug 958714 part 3: Remove obsolete assertion & comment.

https://reviewboard.mozilla.org/r/215712/#review221768
Attachment #8945581 - Flags: review?(mats) → review+
Comment on attachment 8945580 [details]
Bug 958714 part 2: Simplify percent-margin/padding resolution code to pass around a single length as the percent basis.

https://reviewboard.mozilla.org/r/215710/#review221776

::: layout/generic/ReflowInput.cpp:178
(Diff revision 1)
>    LogicalSize cbSize(aContainingBlockWritingMode, aContainingBlockISize,
>                       aContainingBlockISize);
>    ReflowInputFlags flags;
> -  InitOffsets(aContainingBlockWritingMode, cbSize, mFrame->Type(), flags);
> +  InitOffsets(aContainingBlockWritingMode,
> +              cbSize.ISize(aContainingBlockWritingMode),
> +              mFrame->Type(), flags);

Good point! Fixed locally.
Test changes so far:
  https://reviewboard.mozilla.org/r/166816/diff/2-3/
(There are 2 more somewhat-complex grid tests that still need fixing, which I've marked as 'fails' for the moment so that I can sanity-check the rest via Try. I'm not intending to land this patch until I've fixed them up.)

Tweak to address review comment:
 https://reviewboard.mozilla.org/r/215710/diff/1-2/
Depends on: 1434380
Blocks: 1434397
Mats, would you mind sanity-checking that test changes (particularly the grid ones) look OK?  You can see just those changes by viewing the interdiff of "part 1" since the last revision you reviewed -- specifically, this URL:
 https://reviewboard.mozilla.org/r/166818/diff/2-4/

(Disregard MozReview's report of whitespace changes in ReflowInput -- I think that's from the base revision changing.  And beyond that, everything else at that ^^ interdiff will just be the test changes.)

Note that this is layered on top of bug 1434380, which refactored grid-auto-min-sizing-definite-001-ref.html so that it'd be easier to modify here.
Flags: needinfo?(mats)
I made a jsfiddle that shows the problem.  It works in Chrome, but not in Firefox.  Have a look:

https://jsfiddle.net/y3pxa0yL/1/

Mozilla, please fix! I'm having to redesign aspects of my website because of this annoying bug because I carelessly did all of my testing in Chrome and used Firefox testing as an afterthought.

-Thanks!
The test changes looks fine to me.
Flags: needinfo?(mats)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10163f95e43c
part 1: Remove special case for flex & grid items' percent block-axis margin/padding resolution, to align with other browsers. r=mats
https://hg.mozilla.org/integration/autoland/rev/42e9d54bd361
part 2: Simplify percent-margin/padding resolution code to pass around a single length as the percent basis. r=mats
https://hg.mozilla.org/integration/autoland/rev/ae4c9da146ee
part 3: Remove obsolete assertion & comment. r=mats
https://hg.mozilla.org/mozilla-central/rev/10163f95e43c
https://hg.mozilla.org/mozilla-central/rev/42e9d54bd361
https://hg.mozilla.org/mozilla-central/rev/ae4c9da146ee
Status: REOPENED → RESOLVED
Closed: 10 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Summary: Flex items treat percentage padding & margin values in vertical axis as 0, if the flex container lacks an explicit height (allowed by current spec, but perhaps not interoperable?) → Flex items treat percentage padding & margin values in vertical axis as 0, if the flex container lacks an explicit height (was allowed by spec, but perhaps not interoperable?)
You need to log in before you can comment on or make changes to this bug.