1313811 - [css-flexbox] Baseline for empty flexbox container seems wrong

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase: empty boxes on a line

So it seem Grid synthesizing a baseline from its border-box,
Block from its margin-box, and Flexbox from... WAT????

I think the relevant spec text for Grid is:
https://drafts.csswg.org/css-grid/#grid-baselines
third bullet: "one is synthesized if needed according to the rules
of its alignment context"

for Flexbox:
https://drafts.csswg.org/css-flexbox/#flex-baselines
(same text)

so it seems one or the other must be wrong, right?

Given:
https://lists.w3.org/Archives/Public/www-style/2016Sep/0000.html
  - The proposals of what to standardize around were:
      A. Make boxes without a natural baseline and boxes
          establishing an orthogonal flow synthesize a baseline.
          A.1 Base this synthesized baseline on the margin box edges
          A.2 Base this synthesized baseline on some other box edge
      B. Make boxes without a natural baseline and boxes
          establishing an orthogonal flow use start alignment
          instead of trying to synthesize a baseline.
      C. Something else.
  - RESOLVED: Accept option A2 with the modification that we're
              talking about the border box.

I think using the border-box is what they want, assuming an empty
grid/flexbox container is a "box without a natural baseline".

Maybe there is something in "according to the rules of its
alignment context" I'm missing?

Comment 1

[Daniel Holbert [:dholbert]]

Our behavior is based on an older version of the spec, which was quite explicit in requiring what we're doing (using the bottom of the content-box):

> Otherwise, the flex container's baseline is the "after" edge of its content box.
https://www.w3.org/TR/2012/WD-css3-flexbox-20120612/#flex-containers

That language later evolved to the following, which I think basically means the same thing
> Otherwise, the flex container’s main-axis baseline is synthesized from [...] the flex container’s content box.
https://www.w3.org/TR/2014/WD-css-flexbox-1-20140925/#flex-baselines

That language is still there in the latest WD:
https://www.w3.org/TR/2016/CR-css-flexbox-1-20160301/#flex-baselines

It looks like only the very latest draft has the vaguer "rules of its alignment context" text quoted in comment 0, so this must be a somewhat recent change.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #0)
> Maybe there is something in "according to the rules of its
> alignment context" I'm missing?

(I don't know precisely what that phrase means, but I think Comment 1 answers your underlying question -- our current behavior on this was not coded to that spec-text.)

Comment 3

[Daniel Holbert [:dholbert]]

Safari 10 & Chrome 55 (dev) both match us on the flexbox part of the testcase here, too, FWIW. (though maybe that's just because we're all following the same old spec language)

(Edge does not match us -- Edge renders inline-flex the same way they (and we) render inline-block.)

Comment 4

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Our behavior is based on an older version of the spec, which was quite
> explicit in requiring what we're doing (using the bottom of the content-box):

OK, the flexbox testcase makes sense then.

Comment 5

[Mats Palmgren (inactive)]

I've read the minutes of the discussion (and the background info in the URL below)
more carefully now, and I think I understand what they want.
https://github.com/w3c/csswg-drafts/issues/373#issuecomment-242863486

They want synthesized baselines to be consistent *per context*.  So, in an inline
context, as the attached testcase is, they want *all* box types to synthesize based
on the margin-box.  In a grid/flexbox-level context, i.e. for grid/flex items, they
want *all* box types to synthesize baselines from the border-box.  The criteria for
*when* to synthesize appears to be the same everywhere though: empty block/grid/flex
containers (and probably tables too) and orthogonal boxes.

So, we need to make an empty grid/flex container return a margin-box baseline in
some cases, and a border-box baseline in other cases (when its a flex/grid item).

I'll take a stab at this in bug 1313068 which I think is about this problem
for Grid.  I'll try to make it work for Flexbox too.

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

This is a stop-gap fix for Flexbox similar to bug 1313068.
Feel free to take it from here if you want it.

Comment 7

[Mats Palmgren (inactive)]

Attachment: testcase

It fixes this testcase.

Comment 8

[Mats Palmgren (inactive)]

Note that the Flexbox spec is now clear (and says basically what the Grid spec says):
https://drafts.csswg.org/css-flexbox/#flex-baselines
"
Otherwise, the flex container has no first/last main-axis baseline set, and one is synthesized if needed according to the rules of its alignment context.
"

Comment 9

[Mats Palmgren (inactive)]

Attachment: [css-flexbox] Synthesize the flex container baseline per alignment context when needed.

Comment 10

[Mats Palmgren (inactive)]

Attachment: reftests

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8816878 [details] [diff] [review]
[css-flexbox] Synthesize the flex container baseline per alignment context when needed.

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

r=me, with the following:

::: layout/generic/nsFlexContainerFrame.cpp
@@ +4263,5 @@
>                      aStruts, aAxisTracker,
>                      placeholderKids, lines);
>  
> +  if (lines.getFirst()->NumItems() > 0) {
> +    RemoveStateBits(NS_STATE_FLEX_SYNTHESIZE_BASELINE);

Two things:
 (1) Please use the !IsEmpty() accessor here (a bit clearer & shorter).
 (2) Please also check that there's only one line, to be sure our flex items aren't just all sitting in a later flex line. (IIRC this situation is possible with "break-before", or something similar.... and even if it's not technically possible, it's still nice for completeness / future-proofing.)

So, addressing both of those things -- I think this should be replaced with:
 if (lines.getFirst()->IsEmpty() &&
     !lines.getFirst()->getNext()) {
  // We have no flex items, our parent should synthesize a baseline if needed.
  AddStateBits(...)
 } else {
  RemoveStateBits(...)
 }

(Note this suggested-code has the order of the cases swapped w.r.t. the current patch, too -- i.e. I'm putting "AddStateBits" first instead of second -- because that matches the ordering of the grid code in bug 1313811, and also because the "if" condition is more grokkable to me in this configuration. Feel free to keep the ordering you've got now though if you prefer that ordering/if-logic)

@@ +4531,5 @@
>    }
>  
>    // XXXdholbert flexContainerAscent needs to be in terms of
>    // our parent's writing-mode here. See bug 1155322.
> +  if (HasAnyStateBits(NS_STATE_FLEX_SYNTHESIZE_BASELINE)) {

Please move the contextual XXXdholbert comment here into the new "else" clause. (It goes with the SetBlockStartAscent call that you're now wrapping in logic.)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e02278e4834d
[css-flexbox] Synthesize the flex container baseline per alignment context when needed.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/3193d14c8e02
[css-flexbox] Tweak reftests due to change in baseline alignment of empty flex containers in inline-level alignment context.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e02278e4834d
https://hg.mozilla.org/mozilla-central/rev/3193d14c8e02

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8816878 [details] [diff] [review]
[css-flexbox] Synthesize the flex container baseline per alignment context when needed.

css-grid fix for aurora52

Comment 15

[Carsten Book [:Tomcat]]

needs also rebasing for aurora

warning: conflicts while merging layout/generic/nsFrameStateBits.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 16

[Mats Palmgren (inactive)]

Landed in Aurora revs: 99babeef8659, 2148950fcfa4