785468 - Computing the baseline of a flex container correctly, per spec - use baseline of align-self:baseline items, or failing that, the first flex item's baseline

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Spec reference:
 http://dev.w3.org/csswg/css3-flexbox/#flex-baselines

Filing this followup on computing the baseline of a flex container correctly.

(Right now, bug 666041's patches just set the baseline to the bottom edge of the content-box, which is correct for some circumstances, but not all.)

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

This is now significantly more straightforward to do, now that bug 851607 has made flex containers always reflow their children.

Taking.

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(Marking as depending on bug 853946 & bug 851607, since its patch will overlap with & stack on top of those bugs' patches)

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v1

Here's a patch for this.

I want to add a few more reftests (in particular, testing that we correctly generate a flex container's baseline from the baseline used for any "align-self:baseline" flex items, if we have any such items) -- but I've tested that w/ one-off tests, and I think this is otherwise ready for review.

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v1a

(whitespace tweak -- fixed 2 instances of back-to-back blank lines)

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v2

fixed a few things in prev. patch:
 - typo in comment
 - Now querying for first-child's baseline *after* we've called FinishReflowFrame on it, to fix assertion-failures from GetBaseline on a child while dirty bit was still set.
 - added another reftest and tweaked a few things in an existing reftest

(A few more reftests still to come)

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v3

Updated (finished) patch. Changes from previous version:
 - Downgraded an assertion added in the patch to be NS_WARN_IF_FALSE, since it's possible to trigger it with extremely large content. (and in fact one of our existing crashtests already does)
 - Added reftests with baseline-aligned flex items, which establish the baseline for the flex container
 - Cleanup/tweaks to the patch's other reftests.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=942b7c7d19a6

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 731261 [details] [diff] [review]
fix v3

So http://dev.w3.org/csswg/css-flexbox/#flex-baselines has separate rules for main-axis and cross-axis baselines; I'm not sure how that corresponds to this patch.

Comment 9

[Daniel Holbert [:dholbert] (PTO until 2/24)]

AFAIK, we only support horizontal-axis baselines currently -- we don't have any sort of vertical baseline.

So the baseline support all assumes that there's no such thing as a vertical baseline, basically.  Regardless of whether we're vertical or horizontal, we're computing the flex container's horizontal baseline. (which is the cross-axis baseline, in a vertical flex container)

Note also that we have existing code to disregard "align-self: baseline" in a vertical flex basis (internally replacing it with "flex-start"):
  https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#803
(This is what the spec calls for, under the definition of the "baseline" value for align-self:  http://dev.w3.org/csswg/css-flexbox/#align-self )

That makes things simpler here. It means that if we end up with a baseline-position for "align-self:baseline"-aligned children, we can just go ahead and use that as our container baseline.

Comment 10

[Daniel Holbert [:dholbert] (PTO until 2/24)]

As a result, the main-axis and cross-axis baseline-computation chunks end up being the same, basically. Given our no-such-thing-as-a-vertical-baseline assumption, the spec that you linked to can be collapsed into the following:

HOW TO COMPUTE HORIZONTAL BASELINE OF A FLEX CONTAINER:
 1) If you're a horizontal flexbox and have align-self:baseline, then use their baseline-alignment position.
 2) Otherwise, if you have a flex item, use your first flex item's baseline
 3) Otherwise, use the bottom of your content box.

(The spec has some language about "if something has a baseline". I'm not immediately clear how to tell whether something does not have a baseline, or if that's even a meaningful concept in gecko, so for now I'm assuming everything has a baseline, and the baseline of last resort is nsIFrame::GetBaseline).

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> As a result, the main-axis and cross-axis baseline-computation chunks end up
> being the same, basically. Given our no-such-thing-as-a-vertical-baseline
> assumption, the spec that you linked to can be collapsed into the following:
> 
> HOW TO COMPUTE HORIZONTAL BASELINE OF A FLEX CONTAINER:
>  1) If you're a horizontal flexbox and have align-self:baseline, then use
> their baseline-alignment position.

er, I meant to say <<...and have flex items that are aligned with "align-self:baseline">>

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 731261 [details] [diff] [review]
fix v3

ok, r=dbaron

Comment 13

[Daniel Holbert [:dholbert] (PTO until 2/24)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bafd95fdf1ac

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bafd95fdf1ac