Closed Bug 1221524 Opened 9 years ago Closed 8 years ago

[css-flexbox] Implement Flexbox layout for align|justify-self:baseline|last-baseline

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files)

testcase
1.96 KB, text/html
Details
Bug 1221524 Part 1: Map align-self:"baseline" to "flex-start" when FlexItem writing mode is orthogonal to container main axis.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1221524 Part 2: Added a new reftest to check align-items:baseline against orthogonal writing modes.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1221524 Part 3: Implement align-self:last baseline behavior in flex containers.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1221524 Part 4: Add align-items:last baseline to flexbox baseline tests.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1221524 Part 5: Add a new reftest to confirm non-alignment of baseline and last baseline flex items.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Attached file testcase — 
Our baseline alignment here seems wrong (compared to Chrome).
The vertical items shouldn't participate in baseline alignment
at all here, but they somehow influence the position of the
first two items (that should baseline align).
(The "x" item should align its start to the start content edge)

(I suspect this is a case where falling back to GetLogicalBaseline
is wrong)

Attaching this testcase randomly to this bug so it isn't lost...
(In reply to Mats Palmgren (:mats) from comment #1)
I'd like to take this on, but I need some help to understand the root cause.

> The vertical items shouldn't participate in baseline alignment
> at all here

I assume this is the case because it violates this part of the spec at https://drafts.csswg.org/css-align/#baseline-terms:

> A baseline-sharing group is composed of boxes that participate in baseline alignment together.
> This is possible only if they share an alignment context whose axis is parallel to their inline
> axis either have the same block flow direction and baseline alignment preference, or have opposite
> block flow direction and opposite baseline alignment preference (in other words, the baselines
> that want to align are on the same side of the > alignment context).

I don't see the code modelling the baseline-sharing group concept. Are we relying on all children of an nsFlexContainerFrame to implicitly be in the same baseline-sharing group? If so, I can see two approaches to solving the problem:

1) Add the baseline-sharing group concept into the container, and partition the children accordingly.
2) Truncate the flex container when the writing-mode of a child differs from the container, and create a new container to hold the remaining children.
Flags: needinfo?(mats)
> I assume this is the case because it violates this part of the spec ...

Yes.  (I think the spec is wrong about this, but I haven't been able to
convince the spec editor about it yet.)

For your other questions: I don't know, ask dholbert. :-)

I think this bug is lower priority than some other flexbox Align bugs though
(well at least the part of this bug that's about the vertical writing-mode
items influencing the baseline; implementing 'last-baseline' seems a little
higher).

Fwiw, I'd rather see bug 1306894 and bug 1235922 implemented first.
Then the 'last-baseline' part here.  Then bug 1297774.
Flags: needinfo?(mats)
Comment on attachment 8801409 [details]
Bug 1221524 Part 1: Map align-self:"baseline" to "flex-start" when FlexItem writing mode is orthogonal to container main axis.

This is an attempt to apply an appropriate intervention to model the baseline sharing group concept in https://drafts.csswg.org/css-align/#baseline-terms. I would appreciate feedback whether this logic properly implements the concept of enforcing that baseline alignment is only applied when flex items "share an alignment context whose axis is parallel to their inline axis".
Attachment #8801409 - Flags: feedback?(dholbert)
Comment on attachment 8801409 [details]
Bug 1221524 Part 1: Map align-self:"baseline" to "flex-start" when FlexItem writing mode is orthogonal to container main axis.

(In reply to Brad Werth [:bradwerth] from comment #5)
> I would appreciate
> feedback whether this logic properly implements the concept of enforcing
> that baseline alignment is only applied when flex items "share an alignment
> context whose axis is parallel to their inline axis".

This is not quite correct.

Two things:
 (1) It looks like you're comparing the item's writing mode to the flex container's writing mode, to see if they're parallel. That's the right comparison to do, but *only if the flex container is row-oriented* ("flex-direction:row"). If the flex container is column-oriented (flex-direction:column), then its flex children flow in its block axis, and you'd want to do the opposite writing-mode comparison -- i.e. in that case, we want the child to perform baseline alignment if its wm *is orthogonal* to the container's writing mode.

 (2) I'd like to see all of this logic abstracted away inside of the FlexItem class. Specifically, we can & probably-should just figure out whether we're doing baseline-alignment in the FlexItem constructor, and convert away the align-self value (to NS_STYLE_ALIGN_FLEX_START) if we've got the wrong sort of writing-mode. (We sort of do this right now, except with stupid/simple logic which assumes the parent & child both have horizontal writing-modes:
https://dxr.mozilla.org/mozilla-central/rev/7c8216f48c38a8498f251fe044509b930af44de6/layout/generic/nsFlexContainerFrame.cpp#1760 )

So -- that logic in the FlexItem constructor just needs to be made more general -- probably something like:
  if (aAxisTracker.IsRowOriented() ==
      aAxisTracker.GetWritingMode().IsOrthogonal(mWM)) {
    if (mAlignSelf == NS_STYLE_ALIGN_BASELINE) {
      mAlignSelf = NS_STYLE_ALIGN_FLEX_START;
    }
  }

...and then eventually we'll want to add an "else if (mAlignSelf == NS_STYLE_ALIGN_LAST_BASELINE) { mAlignSelf = NS_STYLE_ALIGN_FLEX_END; }" inner clause there, too.  (That's why I'm splitting it up into two separate "if" checks in my suggested code here.)

With that, you won't need to add writing-mode checks to all of the (aFlexItem.GetAlignSelf() == NS_STYLE_ALIGN_BASELINE) comparisons in this file -- the writing-mode checks will already have been done in the FlexItem constructor, and if NS_STYLE_ALIGN_BASELINE is still our FlexItem::mAlignSelf value, then we can trust that it really does want baseline-alignment.
Attachment #8801409 - Flags: feedback?(dholbert)
Comment on attachment 8801409 [details]
Bug 1221524 Part 1: Map align-self:"baseline" to "flex-start" when FlexItem writing mode is orthogonal to container main axis.

The feedback in comment 6 makes it easier to start with a new Part 1 patch.
Attachment #8801409 - Attachment is obsolete: true
Comment on attachment 8801409 [details]
Bug 1221524 Part 1: Map align-self:"baseline" to "flex-start" when FlexItem writing mode is orthogonal to container main axis.

https://reviewboard.mozilla.org/r/86154/#review85606

r=me with a comment nit (assuming existing tests pass)

::: layout/generic/nsFlexContainerFrame.cpp:1752
(Diff revision 3)
>    }
>  #endif // DEBUG
>  
> -  // If the flex item's inline axis is the same as the cross axis, then
> -  // 'align-self:baseline' is identical to 'flex-start'. If that's the case, we
> -  // just directly convert our align-self value here, so that we don't have to
> +  // Map align-self 'baseline' value to 'start' when baseline alignment
> +  // is not possible because the FlexItem's writing mode is orthogonal to
> +  // the inline axis of the container. If that's the case, we just directly

s/inline axis of the container/main axis of the container/

(We don't care about the container's inline axis, except to the extent that it influences what the container's main axis is [together with "flex-direction"])
Attachment #8801409 - Flags: review?(dholbert) → review+
(Sorry for the review lag here -- I've been focusing most of my attention on getting some patches finished up on another bug, and today I'm traveling to a friend's wedding over the weekend, through ~Tuesday. I'll be back in the office on Wednesday, though I'll hopefully get to this during some spare time before then.)
Note that I'm changing the 'last-baseline' keyword to 'last baseline' over in
bug 1313254, so one of us will have to update our patches.  :-)

It's just a CSS syntax change though, the value will still show up as
NS_STYLE_ALIGN_LAST_BASELINE internally, but you'll need to update
the reftests if I land first.
Comment on attachment 8802345 [details]
Bug 1221524 Part 2: Added a new reftest to check align-items:baseline against orthogonal writing modes.

https://reviewboard.mozilla.org/r/86756/#review88646

r=me, one nit:

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-001b.xhtml:60
(Diff revision 2)
> +      .brown  { background: brown;  }
>     </style>
>    </head>
>    <body>
>      <div class="flexbox">
> +      <div class="brown ortho end">ortho</div>

The "align-self:end" here (from the added "end" class) is out of place in this testcase, since this testcase is entirely about testing "align-self:baseline" (via "align-items:baseline" on the parent).  This "end" will explicitly stomp on that value.

I *do* see why "align-self: baseline" can't quite work here -- it'll fall back to start alignment, which in this case means snapping to the bottom of the flex container, due to the reversed cross axis (from "wrap-reverse") in this testcase.

BUT: I think/expect "last-baseline" *should* work here, once we have that implemented in a later patch.  (because "last-baseline" should fall back to "end" when there's no baseline that we can use)

SO: I think I'm OK with this "end", *if* some later patch on this bug changes it from "end" to "last-baseline" *and* we also include an HTML comment explaining the special-case, with something like the following:
"""
NOTE: this flex item's text flows orthogonally to the flex container's main axis, so it can't be baseline-aligned & uses the fallback alignment instead. We really want it to be "top"-aligned, to match the corresponding item in the "-001a" version of this reftest.  Since our container here has a bottom-to-top cross-axis (from "wrap-reverse"), that means "top" is "end", so we use "last-baseline" which should fall back to end alignment.
"""

Alternately, you could just directly change it to "last-baseline" in this patch (and add the ^^ comment), and tag this test as "fails" in reftest.list, and then remove the "fails" in the later patch that gives us last-baseline support in flexbox.
Attachment #8802345 - Flags: review?(dholbert) → review+
Comment on attachment 8802752 [details]
Bug 1221524 Part 3: Implement align-self:last baseline behavior in flex containers.

https://reviewboard.mozilla.org/r/87048/#review88648

r- for the crossStartToFurthestBaseline concerns -- I think we need a bit more here.

Nits/notes below:

::: layout/generic/nsFlexContainerFrame.cpp:1665
(Diff revision 1)
>    if (aFlexItem.Frame() == mFrames.FirstChild() ||
> -      aFlexItem.GetAlignSelf() == NS_STYLE_ALIGN_BASELINE) {
> +      aFlexItem.GetAlignSelf() == NS_STYLE_ALIGN_BASELINE ||
> +      aFlexItem.GetAlignSelf() == NS_STYLE_ALIGN_LAST_BASELINE) {
>      aFlexItem.SetAscent(childDesiredSize.BlockStartAscent());

(You can just revert your changes here [i.e. leave this piece of nsFlexContainerFrame.cpp unmodified]  -- we'll be calling SetAscent unconditionally here now, per bug 1313421 part 1.)

::: layout/generic/nsFlexContainerFrame.cpp:1762
(Diff revision 1)
> +  // We are treating this case as one where it is appropriate to use the
> +  // fallback values defined at https://www.w3.org/TR/css-align-3/#baseline.
>    // https://www.w3.org/TR/css-align-3/#baseline-terms
>    if (aAxisTracker.IsRowOriented() ==

Two nits:
 (1) Drop (or add a space before) the period at the end of the first URL here, "#baseline."  That will produce a broken URL if someone tries to copypaste.

 (2) I'm not sure the "#baseline-terms" link [added in part 1] makes sense here -- particularly now that this patch is adding a different spec URL directly before it.  Unless I'm misunderstanding & there's a reason for the #baseline-terms spec link, let's just remove it in this patch (or just don't add it in the first place, in part 1).

::: layout/generic/nsFlexContainerFrame.cpp:2998
(Diff revision 1)
>        nscoord crossStartToBaseline =
>          item->GetBaselineOffsetFromOuterCrossEdge(eAxisEdge_Start,
> -                                                  aAxisTracker);
> +          aAxisTracker,
> +          (item->GetAlignSelf() == NS_STYLE_ALIGN_BASELINE));
>        nscoord crossEndToBaseline = curOuterCrossSize - crossStartToBaseline;
>  
>        // Now, update our "largest" values for these (across all the flex items
>        // in this flex line), so we can use them in computing the line's cross
>        // size below:
>        crossStartToFurthestBaseline = std::max(crossStartToFurthestBaseline,
>                                                crossStartToBaseline);
>        crossEndToFurthestBaseline = std::max(crossEndToFurthestBaseline,

It looks to me like this patch makes us treat "last-baseline" as just a different form of "baseline".  In particular, it looks like this code will *include* "last-baseline" items when determining the alignment position for our normal "baseline"-aligned items, and line them all up together.

I don't think that's what we're supposed to do.  IIUC, we're supposed to align the two groups *independently*, with the "baseline" group snapped up against the start edge, and the "last-baseline" group snapped up against the end edge.  This might mean the two groups are aligned quite far apart, or quite close together, depending on how much extra space there is (e.g. from other tall items) in the FlexLine.

SO: I think we really need to keep track of *a new set* of "crossStartToFurthestBaseline" & "crossEndToFurthestBaseline" values here, for the last-baseline aligned items.  Maybe these new versions should be named "...ToFurthestLastBaseline" (adding "Last"). And the last-baseline items should be determining those new values, not the original values.  And then we'll need downstream usages of those variables for the last-baseline-aligned items, just like the usages of the current variables for baseline-aligned variables.

::: layout/generic/nsFlexContainerFrame.cpp:3811
(Diff revision 1)
>    if (aItem.Frame() == mFrames.FirstChild() ||
> -      aItem.GetAlignSelf() == NS_STYLE_ALIGN_BASELINE) {
> +      aItem.GetAlignSelf() == NS_STYLE_ALIGN_BASELINE ||
> +      aItem.GetAlignSelf() == NS_STYLE_ALIGN_LAST_BASELINE) {
>      aItem.SetAscent(childDesiredSize.BlockStartAscent());
>    }

(As above, you can revert your chagnes here, since we'll be calling SetAscent unconditionally here now, per bug 1313421 part 1.)
Attachment #8802752 - Flags: review?(dholbert) → review-
Comment on attachment 8802753 [details]
Bug 1221524 Part 4: Add align-items:last baseline to flexbox baseline tests.

https://reviewboard.mozilla.org/r/87050/#review88656

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-001-ref.xhtml:73
(Diff revision 1)
> +    <div class="flexbox">
> +      <div class="brown ortho left">ortho</div
> +      ><div class="yellow">blk<br/>2lines</div

Right now, in this new section of the reference case, "ortho" is top-aligned, while the rest of the items are last-baseline-aligned (and collectively lined up against the bottom edge).

If we're actually testing orthogonal items in this testcase, then we should actually be testing their alignment under "[last-]baseline" -- and so this part of the reference case should be snapped to the bottom (not the top), to actually represent the last-baseline alignment of "ortho" in the corresponding testcases.

I'm not sure the best way to do this bottom-snapping (in the reference case) offhand... Perhaps with a "margin-top" value on this element? (you'd have to give it and its flex-container a hardcoded "height", too, in order to know the correct "margin-top" value to use)

Alternately: maybe it'd be best to just leave this -001 reftest untouched (and avoid adding any new hardcoded sizes/margins just for hacky bottom snapping), and instead test orthogonal baseline interactions in a *new* dedicated reftest? (which *can* have hardcoded sizes, and hence would be a bit more constrained & easier to create a reference for)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-001a.xhtml:47
(Diff revision 1)
> +      .start {
> +        align-self: flex-start;
> +      }
> +      .end {
> +        align-self: flex-end;
> +      }

As noted for part 2: the explicit "start"/"end" alignment of the ortho elements in the testcases here are out-of-place -- they prevent this test from usefully testing what the "baseline" (& "last-baseline") value does in those scenarios.

You should be using "baseline" and/or "last-baseline" on those elements.
We also need a new reftest that has a single flex container with a mix of "baseline" & "last-baseline"-aligned items -- and this new reftest should verify that the two groups of items align independently of each other.  (I suspect such a test would fail right now, per comment 20 -- but once comment 20 is addressed, it'd presumably pass.)
Comment on attachment 8802753 [details]
Bug 1221524 Part 4: Add align-items:last baseline to flexbox baseline tests.

https://reviewboard.mozilla.org/r/87050/#review88660
Attachment #8802753 - Flags: review?(dholbert) → review-
Comment on attachment 8802345 [details]
Bug 1221524 Part 2: Added a new reftest to check align-items:baseline against orthogonal writing modes.

https://reviewboard.mozilla.org/r/86756/#review88646

> The "align-self:end" here (from the added "end" class) is out of place in this testcase, since this testcase is entirely about testing "align-self:baseline" (via "align-items:baseline" on the parent).  This "end" will explicitly stomp on that value.
> 
> I *do* see why "align-self: baseline" can't quite work here -- it'll fall back to start alignment, which in this case means snapping to the bottom of the flex container, due to the reversed cross axis (from "wrap-reverse") in this testcase.
> 
> BUT: I think/expect "last-baseline" *should* work here, once we have that implemented in a later patch.  (because "last-baseline" should fall back to "end" when there's no baseline that we can use)
> 
> SO: I think I'm OK with this "end", *if* some later patch on this bug changes it from "end" to "last-baseline" *and* we also include an HTML comment explaining the special-case, with something like the following:
> """
> NOTE: this flex item's text flows orthogonally to the flex container's main axis, so it can't be baseline-aligned & uses the fallback alignment instead. We really want it to be "top"-aligned, to match the corresponding item in the "-001a" version of this reftest.  Since our container here has a bottom-to-top cross-axis (from "wrap-reverse"), that means "top" is "end", so we use "last-baseline" which should fall back to end alignment.
> """
> 
> Alternately, you could just directly change it to "last-baseline" in this patch (and add the ^^ comment), and tag this test as "fails" in reftest.list, and then remove the "fails" in the later patch that gives us last-baseline support in flexbox.

Moved the new test-against-ortho behavior into a new reftest (with fixed height container and items) to avoid polluting existing reftests with special-case stuff to simulate flex-start and flex-end.
Comment on attachment 8802345 [details]
Bug 1221524 Part 2: Added a new reftest to check align-items:baseline against orthogonal writing modes.

https://reviewboard.mozilla.org/r/86756/#review90516

Please update the commit message for "part 2". Right now it says:
> Bug 1221524 Part 2: Updated several of the flexbox-align-self-baseline-horiz-* reftests [...]
...but now it's adding a new reftest instead of updating existing ones, as you noted in comment 29.

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-006.xhtml:7
(Diff revision 3)
> +     align-self, implicitly). This test baseline-aligns various types of
> +     content, and the flexbox's vertical size depends on the aggregate
> +     post-alignment height of its children.

Please remove this last clause (" and the flexbox's vertical size depends on the aggregate post-alignment height of its children") -- it's not true, in this test. The flex container here has an explicit "height: 50px" -- its height does not depend on its children's sizes. (It's merely testing alignment, not sizing.)

(Looks like this is just a copypaste typo from basing this test on flexbox-align-self-baseline-horiz-001.xhtml.)
Comment on attachment 8802752 [details]
Bug 1221524 Part 3: Implement align-self:last baseline behavior in flex containers.

https://reviewboard.mozilla.org/r/87048/#review90520

This looks really good -- thanks! Just a few nits:

::: layout/generic/nsFlexContainerFrame.cpp:939
(Diff revision 2)
> +  nscoord GetLastBaselineOffset() const {
> +    return mLastBaselineOffset;

Please add a brief comment here, particularly explaining what this distance is relative to (since it's different from GetFirstBaselineOffset).

e.g.:
  // Same as above, but for the "last baseline" alignment position. The
  // returned value is usually an offset from the line's cross-end edge
  // (the opposite edge from the one GetFirstBaselineOffset() uses).

::: layout/generic/nsFlexContainerFrame.cpp:943
(Diff revision 2)
> +
> +  nscoord GetLastBaselineOffset() const {
> +    return mLastBaselineOffset;
>    }
>  
> -  // Runs the "Resolving Flexible Lengths" algorithm from section 9.7 of the
> +   // Runs the "Resolving Flexible Lengths" algorithm from section 9.7 of the

Nit: looks like you inserted a stray space character at the start of this line. Please revert that.

::: layout/generic/nsFlexContainerFrame.cpp:3149
(Diff revision 2)
>        nscoord crossStartToBaseline =
>          item->GetBaselineOffsetFromOuterCrossEdge(eAxisEdge_Start,
> -                                                  aAxisTracker);
> +          aAxisTracker,
> +          useFirst);

Please don't do this extreme-deindentation-of-args unless it's absolutely necessary to avoid hugely long (over 80 character) lines.

Doesn't look like there's any risk of the line being too long here, so please keep the args to the right of the open-paren (as they were before this change).

::: layout/generic/nsFlexContainerFrame.cpp:3351
(Diff revision 2)
>          aItem.GetBaselineOffsetFromOuterCrossEdge(baselineAlignEdge,
> -                                                  aAxisTracker);
> +          aAxisTracker,
> +          useFirst);

As above: please don't do this extreme-deindentation-of-args.  Let's indent "aAxisTracker" and "useFirst" to be directly underneath the first arg (baselineAlignEdge).
Attachment #8802752 - Flags: review?(dholbert) → review+
Comment on attachment 8802753 [details]
Bug 1221524 Part 4: Add align-items:last baseline to flexbox baseline tests.

https://reviewboard.mozilla.org/r/87050/#review90524

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-006-ref.xhtml:27
(Diff revision 2)
>        div   { display: inline-block; }
>        table { display: inline-table; }

Now that we're using a flex container in this reference case, these "inline-block" / "inline-table" styles serve no purpose and have no effect.

Please remove them (from the testcase and reference case).

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-006-ref.xhtml:49
(Diff revision 2)
>        ><table cellspacing="0" cellpadding="0"
> -              class="orange">two<br/>lines</table
> +              class="orange offset start">two<br/>lines</table

Now that we're changing this reference case to have a flex container instead of a block, we don't need this <table> here anymore. (Per comment at the top, it's just a gimmick to emulate the correct baseline alignment of flex items, without using a flex container.)

SO: please replace this table with just a <div> (to remove a now-irrelevant difference between testcase & reference case), and remove the explanatory comment about tables from the top of this reference case as well.

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-006.xhtml:31
(Diff revision 2)
> -      table { display: inline-table; }
> +      .last-base { align-items: last-baseline; }
>  
>        .ortho  { writing-mode: vertical-rl;
> -                width: 17px; }
> +                width: 17px;
> +                height: 40px; }
>        .offset { margin-top: 10px; }

You should probably add e.g. "margin-bottom: 3px" to .offset (in the testcase and reference case), so that this test will verify that we're correctly lining up the items' *margin-boxes* with the bottom of the flex container, in last-baseline alignment.

(This will mean you'll need to add the "offset" class to the various divs in the second half of your reference case, just like you do in the first half of your reference case.)
Attachment #8802753 - Flags: review?(dholbert) → review+
Comment on attachment 8802752 [details]
Bug 1221524 Part 3: Implement align-self:last baseline behavior in flex containers.

https://reviewboard.mozilla.org/r/87048/#review90520

> Please don't do this extreme-deindentation-of-args unless it's absolutely necessary to avoid hugely long (over 80 character) lines.
> 
> Doesn't look like there's any risk of the line being too long here, so please keep the args to the right of the open-paren (as they were before this change).

Ah, sorry for this one. I had a hasty parameter replacement when I defined the useFirst variable, and didn't notice I had previously made major indentation concessions to make room for the inline code.
Comment on attachment 8807377 [details]
Bug 1221524 Part 5: Add a new reftest to confirm non-alignment of baseline and last baseline flex items.

https://reviewboard.mozilla.org/r/90532/#review90526

Commit message nit:
> Bug 1221524 Part 5: Add a mochitest to confirm non-alignment of baseline and last-baseline flex items. r?dholbert

s/mochitest/reftest/

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-007-ref.xhtml:24
(Diff revision 2)
> +    <style>
> +      .container {
> +        display: flex;
> +        border: 1px dashed blue;
> +        font: 14px sans-serif;
> +        height: 51px;

Could we just make this 50px rather than 51px, to make it consistent with the previous test (-006) and to avoid the appearance of oddly-specific-size-tuning? (This applies to the testcase as well as the reference case -- both of them, 007 & 007-ref, have "51px".)

If it's better as 51px for some reason, that's fine; but let's make it round & consistent-with-other-tests if we can.

(When I first saw this extra 1px, I panicked & thought it might be a hack to accommodate some off-by-one pixel-rounding issue. :)  It looks like it's just arbitrary, though, I think?)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-baseline-horiz-007-ref.xhtml:27
(Diff revision 2)
> +        border: 1px dashed blue;
> +        font: 14px sans-serif;
> +        height: 51px;
> +      }
> +
> +      div   { display: inline-block; }

As with the previous test: this "display: inline-block" style serves no purpose here (since we're not actually using inline-blocks to create our reference case).

Please remove this from the reference case and testcase.
Attachment #8807377 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Note that I've landed the change that deprecates 'last-baseline' now,
so you need to update the tests here before landing.  Sorry.
Keywords: checkin-needed
Assignee: dholbert → bwerth
Flags: needinfo?(bwerth)
(In reply to Mats Palmgren (:mats) from comment #45)
> Note that I've landed the change that deprecates 'last-baseline' now,
> so you need to update the tests here before landing.  Sorry.

Not a problem; I've taken care of it in the updated patches.
Flags: needinfo?(bwerth)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/40c184b44a5a
Part 1: Map align-self:"baseline" to "flex-start" when FlexItem writing mode is orthogonal to container main axis. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c43961ed7881
Part 2: Added a new reftest to check align-items:baseline against orthogonal writing modes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/9e51cd695083
Part 3: Implement align-self:last baseline behavior in flex containers. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/428ede5e855b
Part 4: Add align-items:last baseline to flexbox baseline tests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/951f657c5399
Part 5: Add a new reftest to confirm non-alignment of baseline and last baseline flex items. r=dholbert
Keywords: checkin-needed
Setting this to dev-doc-complete, as I believe this is now covered by 

https://bugzilla.mozilla.org/show_bug.cgi?id=1335106

We are covering this in a separate piece of work.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1384266
No longer depends on: 1384266
Regressions: 1384266
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: