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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({testcase})

Trunk
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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?
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.
(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.)
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.)
(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.
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.
Depends on: 1313068
Posted patch fix (obsolete) — Splinter Review
This is a stop-gap fix for Flexbox similar to bug 1313068.
Feel free to take it from here if you want it.
Posted file testcase
It fixes this testcase.
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.
"
Assignee: nobody → mats
Attachment #8805820 - Attachment is obsolete: true
Attachment #8816878 - Flags: review?(dholbert)
Posted patch reftestsSplinter Review
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.)
Attachment #8816878 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
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.
https://hg.mozilla.org/mozilla-central/rev/e02278e4834d
https://hg.mozilla.org/mozilla-central/rev/3193d14c8e02
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8816878 [details] [diff] [review]
[css-flexbox] Synthesize the flex container baseline per alignment context when needed.

css-grid fix for aurora52
Attachment #8816878 - Flags: approval-mozilla-aurora+
Flags: in-testsuite+
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')
Flags: needinfo?(mats)
Landed in Aurora revs: 99babeef8659, 2148950fcfa4
Flags: needinfo?(mats)
You need to log in before you can comment on or make changes to this bug.