851379 - Make flex item vertical margin/padding resolve percentages against height of the flex container

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Tab recently suggested on www-style that we make "vertical margin/padding [on flex items] resolve their percentages against the height of the [flex container]", rather than its width, to be consistent with grid layout.[1]

I'm not sure that the spec-change is going to go through (Ojan raised a possible objection[2]), but I'm filing this bug as a reminder to address this if it does go through.

[1] http://lists.w3.org/Archives/Public/www-style/2013Mar/0143.html
  w/ clarifications in http://lists.w3.org/Archives/Public/www-style/2013Mar/0148.html

[2] http://lists.w3.org/Archives/Public/www-style/2013Mar/0159.html

Comment 1

[Daniel Holbert [:dholbert]]

Note that this is marked as blocking bug 851379.  If this spec-change happens soon enough, then this is something we should fix before enabling this on release branches (or rather, before bug 851379's patch makes it to an actual release branch).

If it doesn't happen soon enough, we can probably just fix this in a future release, too; so this isn't a hard "block", but rather a soft/tracking block.

Comment 2

[Daniel Holbert [:dholbert]]

From latest CSSWG Telcon:
  - RESOLVED: We copy the behaviour of % margins/paddings from grid to
              flexbox: they are relative to their respective dimension,
              not always the inline dimension (as in block layout)

http://lists.w3.org/Archives/Public/www-style/2013Mar/0688.html

Comment 3

[Daniel Holbert [:dholbert]]

...and the (current) spec text for this is:
    Percentage margins and paddings on flex items are always resolved
    against their respective dimensions; unlike blocks, they do not always
    resolve against the inline dimension of their containing block.
http://dev.w3.org/csswg/css-flexbox/#item-margins

Comment 4

[Florian Bender]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Note that this is marked as blocking bug 851379.

Did you mean bug 841876? It already landed for v22. I guess there's not enough time left before branching, so maybe fix this ASAP and uplift to Aurora!?

Comment 5

[Daniel Holbert [:dholbert]]

Yeah, sorry -- I meant to say bug 841876.  bug 851379 is this bug here, of course. :)

> fix this ASAP and uplift to Aurora!?

That's my current plan, yeah.

(However: if this isn't approved to be uplifted for some reason, it wouldn't be the end of the world. This is a case where the spec changed after already having been at Candidate Recommendation level (and hence encouraging unprefixed implementations) for months.  It's unsurprising that this could result in a behavior-change between two browser-versions.)

Comment 6

[Florian Bender]

Thanks for the clarification. This should really be uplifted, as it may lead to confusion of developers, and should be a rather non-intrusive(?) patch (thus little risk and, at least for devs, high gain). Otherwise the release notes for v22 should clearly state the current and expected v23 behaviour. 

(Yeah, I know it's not your call, just wanted to stress my opinion ;-).)

Comment 7

[Daniel Holbert [:dholbert]]

So this ended up being slightly more complicated than I thought, but not too much.

Currently, we don't have the containing-block height available, in the code that resolves percent margin values.  We have a few levels up the stack, but we just need to pass it down (and rename a few functions & variables accordingly, and add a new parameter here and there).

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 1: s/ComputeWidthDependentValue/ComputeCBDependentValue/

So, the code that resolves percent margin/padding uses "nsLayoutUtils::ComputeWidthDependentValue()" to do the actual percent-resolution (passing in the containing block width).

There's nothing really width-specific about that function's internals, though, so this patch renames it to "ComputeCBDependentValue", and renames its parameter to "aPercentBasis" (which can be either the CB width or height -- whichever the caller wants to use).

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 732594 [details] [diff] [review]
part 1: s/ComputeWidthDependentValue/ComputeCBDependentValue/

Changing review to mats, since dbaron's about to go on vacation, and since this is in general layout code anyway (not in flexbox code).

(Mats, see comment 2 & comment 3 for the csswg description of the change, and see comment 8 for the description of this first helper-patch.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 2: Make InitOffsets, ComputeMargin, ComputePadding take both a horizontal and vertical basis

This patch makes InitOffsets and its helper-methods ComputeMargin & ComputePadding take *both* a horizontal and vertical basis for resolving percent margins & padding against.  Usually, the horizontal and vertical basis will be the same -- the containing block width -- but this lets us optionally make them different for e.g. flex items, and this patch *does* make them different (using the containing block height) in that case.

(When we implement CSS grid, we'll want to do the same thing for grid items, too.)

I'll post a few notes about specific things in this patch, in the next comment.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 734188 [details] [diff] [review]
part 2: Make InitOffsets, ComputeMargin, ComputePadding take both a horizontal and vertical basis

>@@ -1835,17 +1835,19 @@ nsHTMLReflowState::InitConstraints(nsPre
>   DISPLAY_INIT_CONSTRAINTS(frame, this,
>                            aContainingBlockWidth, aContainingBlockHeight,
>                            aBorder, aPadding);
> 
>   // If this is the root frame, then set the computed width and
>   // height equal to the available space
>   if (nullptr == parentReflowState) {
>     // XXXldb This doesn't mean what it used to!
>-    InitOffsets(aContainingBlockWidth, aFrameType, aBorder, aPadding);
>+    InitOffsets(aContainingBlockWidth,
>+                aContainingBlockWidth,
>+                aFrameType, aBorder, aPadding);

(Note that we don't have to worry about needing the special flex-item behavior here, because this is the "root frame" case, and by definition flex items have a parent (their flex container) and so will never be the root frame.)
>+nsCSSOffsetState::InitOffsets(nscoord aHorizontalPercentBasis,
>+                              nscoord aVerticalPercentBasis,
>                               nsIAtom* aFrameType,
>                               const nsMargin *aBorder,
>                               const nsMargin *aPadding)
> {
>-  DISPLAY_INIT_OFFSETS(frame, this, aContainingBlockWidth, aBorder, aPadding);
>+  // XXXdholbert This macro probably needs to take aVerticalPercentBasis too
>+  DISPLAY_INIT_OFFSETS(frame, this, aHorizontalPercentBasis, aBorder, aPadding);

I'll fix this debugging macro to also accept aVerticalPercentBasis in a subsequent patch on this bug.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: part 3: reftest patch

Here's a reftest patch. As noted in the header-comment in this patch's reference case, the reference case is the same as the testcase, except that I've replaced the vertical percent-valued margins & padding with the pixel-values that they should compute to.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: part 4 wip: fix debug macro

This is what I've got currently for fixing the debug macro, FWIW. (haven't tested this part yet)

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 732594 [details] [diff] [review]
part 1: s/ComputeWidthDependentValue/ComputeCBDependentValue/

r=mats

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 734188 [details] [diff] [review]
part 2: Make InitOffsets, ComputeMargin, ComputePadding take both a horizontal and vertical basis

r=mats, with a few minor nits:


(In reply to Daniel Holbert [:dholbert] from comment #11)
> >   // If this is the root frame, then set the computed width and
> >   // height equal to the available space
> >   if (nullptr == parentReflowState) {
> >     // XXXldb This doesn't mean what it used to!
> >-    InitOffsets(aContainingBlockWidth, aFrameType, aBorder, aPadding);
> >+    InitOffsets(aContainingBlockWidth,
> >+                aContainingBlockWidth,
> >+                aFrameType, aBorder, aPadding);
> 
> (Note that we don't have to worry about needing the special flex-item
> behavior here, because this is the "root frame" case, and by definition flex
> items have a parent (their flex container) and so will never be the root
> frame.)

I started my review before seeing this comment and it jumped out
at me so I think it deserves a code comment, or better still an
assertion that also serves as documentation.  Something like this
might help the reader see why 2 x aContainingBlockWidth is ok here:

MOZ_ASSERT(!frame->IsFlexItem(), "the root frame can't be a flex item");


>layout/generic/nsHTMLReflowState.cpp
>+    if (frame->IsFlexItem()) {
>+      verticalPercentBasis =
>+        (aContainingBlockHeight == NS_AUTOHEIGHT) ?
>+        0 : aContainingBlockHeight;
>+    }

Odd indentation and the parenthesis are redundant.
I think the ?: expression actually fits on the same line:

verticalPercentBasis =
  aContainingBlockHeight == NS_AUTOHEIGHT ? 0 : aContainingBlockHeight;


>nsCSSOffsetState::ComputeMargin
>+  bool isContainerSizeDependent = !styleMargin->GetMargin(mComputedMargin);
>+  if (isContainerSizeDependent) {
>     // We have to compute the value
>     mComputedMargin.left = nsLayoutUtils::
>-      ComputeCBDependentValue(aContainingBlockWidth,
>+      ComputeCBDependentValue(aHorizontalPercentBasis,
>                               styleMargin->mMargin.GetLeft());

I'd prefer isCBSizeDependent (or isCBDependent even).
Container usually means parent, whereas Containing Block (CB) can be
any ancestor.  Also, isCBSizeDependent harmonizes better with the
ComputeCBDependentValue naming.


>nsCSSOffsetState::ComputePadding

same nit


>layout/generic/nsHTMLReflowState.h
>+   * @param aVerticalPercentBasis
>+   *    Length to use for resolving percentage margin values in the vertical
>+   *    axis.  Usually the containing block width, per CSS21 sec 8.4, but may
>+   *    be the containing block *height* in e.g. CSS3 Flexbox and Grid.
>+   * @return true if the padding is dependent on the containing block size.
>    */
>+   bool ComputePadding(nscoord aHorizontalPercentBasis,

s/margin/padding/

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 734190 [details] [diff] [review]
part 3: reftest patch

It's a nice suite of flex-box tests you have here.
Any plans to submit them to the CSS test suite?

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: part 2 v2: Make InitOffsets, ComputeMargin, ComputePadding take both a horizontal and vertical basis

Addressed review comments on part 2 - thanks!  In particular:
- I added a comment and an assertion for the root-frame special case (per your first bit of review-feedback)
- I renamed isContainerSizeDependent to "isCBDependent", to make it match the "ComputeCBDependentValue" naming as you pointed out.

(In reply to Mats Palmgren [:mats] from comment #16)
> It's a nice suite of flex-box tests you have here.
> Any plans to submit them to the CSS test suite?

Ultimately, yeah... I need to get around to that. :-/

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: part 4: Fix debug macro to account for InitOffsets taking a horizontal & vertical percent basis, instead of just a containing-block width

This fixes the InitOffsets logging macro that's output by GECKO_DISPLAY_REFLOW_RULES_FILE, as described here:
https://mxr.mozilla.org/mozilla-central/source/layout/doc/frame_reflow_debug.html

The old output looked like this:
     block 0x7f61af8aabf0 InitOffsets cbw=5220 b=(nil) p=(nil)
     block 0x7f61af8aabf0 InitOffsets= m=480,480,480,480 p=0,0,0,0 p+b=0,0,0,0

The new output looks like this:
     block 0x7f100acd4bf0 InitOffsets pct_basis=5220,5220 b=(nil) p=(nil)
     block 0x7f100acd4bf0 InitOffsets= m=480,480,480,480 p=0,0,0,0 p+b=0,0,0,0

So, this replaces "cbw" with "pct_basis" and makes that display two values instead of one. (though they'll usually be the same value)

(I couldn't think of a better abbreviation... I initially thought "pb" for "percent basis", but that'd be confusing since then "p" and "b" would have multiple meanings on the same log-line. (b=(nil) p=(nil) are for border and padding)

Aside: I sort of doubt that anyone's been relying on this logging system recently, given that it's had a shutdown crash for years (see bug 860333).

Comment 19

[Daniel Holbert [:dholbert]]

(The bulk of this patch is just changing intermediate function signatures -- there are a few levels of intermediate macros / functions between the actual InitOffsets call and the logging printfs, and those all need to take the two percent basis values instead of cb-width now.)

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 735904 [details] [diff] [review]
part 4: Fix debug macro to account for InitOffsets taking a horizontal & vertical percent basis, instead of just a containing-block width

>+    DR_state->PrettyUC(aHorizontalPercentBasis, horizPctBasisStr);
>+    DR_state->PrettyUC(aHorizontalPercentBasis, vertPctBasisStr);

The 2nd line should use aVerticalPercentBasis.

r=mats with that.

Comment 21

[Daniel Holbert [:dholbert]]

Oops, good catch! Clearly I didn't test the logging with flex items. :) I'll fix that and make sure it prints the right values before landing. Thanks!

Comment 22

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb63e38eeaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4434a4e7f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de3dbbec45d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b16ed870d536

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/2cb63e38eeaf
https://hg.mozilla.org/mozilla-central/rev/0e4434a4e7f3
https://hg.mozilla.org/mozilla-central/rev/8de3dbbec45d
https://hg.mozilla.org/mozilla-central/rev/b16ed870d536

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: rollup patch for aurora approval

Requesting aurora approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
 A late-breaking CSS working group resolution to change the flexbox spec (comment 2).  This bug is bringing us into alignment with that change.

User impact if declined: 
 Web developer confusion, if they use flexbox to code up their websites in Firefox 22 and then the behavior changes in Firefox 23. (Note that Firefox 22, currently on Aurora, is slated to be the first release where flexbox support is default-enabled, per bug 841876.) 

Testing completed (on m-c, etc.): 
 Reftests included; been on inbound / central since yesterday. 

Risk to taking this patch (and alternatives if risky):
  Low risk.
  The 4-part nature of this bug makes it look deceptively complex, but that's purely for reviewability.  Part 1 is purely a function rename, so no functional change. Part 2 is the only "interesting" bit -- it adds an additional argument to our margin/padding-resolving functions, in a way that keeps behavior the same except inside of a flexbox where we're changing behavior.  Part 3 is tests, and part 4 is in #ifdef DEBUG code.
  So part 2 is the only real functional change, and its behavior-change is flexbox-specific.

String or IDL/UUID changes made by this patch:
  None.

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 736860 [details] [diff] [review]
rollup patch for aurora approval

Taking this sort of change on Aurora makes total sense.

Comment 26

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd5bf3279cc9

Comment 27

[Daniel Holbert [:dholbert]]

Thanks for the approval & aurora landing! (I was actually going to land the patches as separate csets, to more closely correspond to the m-c history - I mostly posted the rollup patch for easier one-click approval - but the rollup works for landing too. :))

Comment 28

[Daniel Holbert [:dholbert]]

FWIW, Tab is proposing that we revert this part of the spec, because Blink is pretty adamant about not implementing it:
http://www.w3.org/mid/[e-mail]

Comment 29

[Jean-Yves Perrier [:teoli]]

Adding dev-doc-needed here, not for the way flexbox is currently calculating percentage (vertical percentages relating to the height), but to be sure that the percentage calculation (vertical percentage relating to the width) is adequately documented.

If we revert this change, I guess a new bug will be opened, please don't forget a dev-doc-needed on it.