Closed Bug 785468 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: dholbert)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.)
This is now significantly more straightforward to do, now that bug 851607 has made flex containers always reflow their children.

Taking.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Marking as depending on bug 853946 & bug 851607, since its patch will overlap with & stack on top of those bugs' patches)
Depends on: 853946
Attached patch fix v1 (obsolete) — Splinter Review
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.
Attachment #730588 - Flags: review?(dbaron)
Attached patch fix v1a (obsolete) — Splinter Review
(whitespace tweak -- fixed 2 instances of back-to-back blank lines)
Attachment #730588 - Attachment is obsolete: true
Attachment #730588 - Flags: review?(dbaron)
Attachment #730589 - Flags: review?(dbaron)
Summary: Take "align-self:baseline" items' baselines into consideration when computing the baseline of a flexbox → 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
Attached patch fix v2 (obsolete) — Splinter Review
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)
Attachment #730589 - Attachment is obsolete: true
Attachment #730589 - Flags: review?(dbaron)
Attachment #730987 - Flags: review?(dbaron)
Attached patch fix v3Splinter Review
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.
Attachment #730987 - Attachment is obsolete: true
Attachment #730987 - Flags: review?(dbaron)
Attachment #731261 - Flags: review?(dbaron)
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.
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.
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).
(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 on attachment 731261 [details] [diff] [review]
fix v3

ok, r=dbaron
Attachment #731261 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/bafd95fdf1ac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.