Closed Bug 1472843 Opened 6 years ago Closed 6 years ago

[css-flexbox] implement new align-self/content/items & justify-content values, for flexbox

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dholbert, Assigned: mihir)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

(Spinning a piece off of bug 1207698, per bug 1207698 comment 4)

This bug covers implementing the remaining new alignment values for flexbox.  


These are largely flagged with "NYI:" warnings in nsFlexContainerFrame.cpp, e.g. for justify-content:
>        NS_WARNING("NYI: justify-content:left/right/baseline/last baseline");
https://dxr.mozilla.org/mozilla-central/rev/23885c14f025b61bb74d85845ac4018f05eb39f8/layout/generic/nsFlexContainerFrame.cpp#3009


...and for align-self (and implicitly align-items):
>       NS_WARNING("NYI: align-items/align-self:left/right/self-start/self-end");
https://dxr.mozilla.org/mozilla-central/rev/23885c14f025b61bb74d85845ac4018f05eb39f8/layout/generic/nsFlexContainerFrame.cpp#3522


...and for align-content (note the warning message incorrectly mentions align-items/self but this one's really about align-content):
>        NS_WARNING("NYI: align-items/align-self:left/right/self-start/self-end/baseline/last baseline");
https://dxr.mozilla.org/mozilla-central/rev/23885c14f025b61bb74d85845ac4018f05eb39f8/layout/generic/nsFlexContainerFrame.cpp#3220


On top of those, there are also "start" / "end" which need to have differentiated behavior from flex-start and flex-end, for scenarios where we have row-reverse / column-reverse / wrap-reverse (which flip the main or cross axis with respect to the writing-mode).
Priority: -- → P3
mihir is probably going to take this, since he's been looking at flexbox alignment code recently.

Spec link, for reference: https://drafts.csswg.org/css-align/#positional-values
Assignee: dholbert → miyer
Adding justify-content:start/end to this bug. Fixing this will result in justify-content:left/right working properly.
Note: this reference case will need a tweak as part of this fix (in order to keep passing):
https://dxr.mozilla.org/mozilla-central/rev/3cb90f16402bc5e1203e2771dc93553b8377fa40/layout/reftests/flexbox/flexbox-justify-content-horizrev-001-ref.xhtml#124-138

Live version:
https://hg.mozilla.org/mozilla-central/raw-file/3cb90f16402bc5e1203e2771dc93553b8377fa40/layout/reftests/flexbox/flexbox-justify-content-horizrev-001-ref.xhtml
(Its testcase is the same URL, without the "-ref")

Per the comment inside the reference case, it is "expecting" an incorrect result right now, as a way of documenting our current (incorrect) behavior and catching any inadvertant changes.
In particular: I suspect the last two chunks of that reference case (for "left" and "right") just need to be swapped.
See Also: → 1480850
We've realized that align-content:justify is a bit trickier and probably deserves its own bug - I spun off bug 1480850 for that one.
Attached patch Patch to avoid using Phabricator (obsolete) — Splinter Review
Attachment #8999753 - Flags: review?(dholbert)
Comment on attachment 8999753 [details] [diff] [review]
Patch to avoid using Phabricator

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

Haven't looked at tests yet (hence not setting r+ yet), but the code looks great!

One commit-message nit: you should add some additional lines (beyond the first[1]) to your commit message, to justify/explain the code removal.  Possible sample text in [2].

[1] This extra chunk should be separated by a linebreak in the commit message (and linewrapped at ~80 characters, unlike the first line).  Mercurial gives special treatment to the first line of the commit message (up to the first linebreak) -- that's what shows up in "hg log" and treeherder, for example. And then we use subsequent lines to provide prose to explain/contextualize what's changing in the patch, if it's needed - and this shows up when people view the commit directly in hgweb or via 'hg export', for example.

[2] Possible/suggested wording:
===================
This commit also removes some cases & warnings about unsupported
values that have now been removed from the css alignment spec.
Specifically: "justify-content:[last] baseline" and
"align-self/align-content: left/right".
Comment on attachment 8999753 [details] [diff] [review]
Patch to avoid using Phabricator

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

::: layout/generic/nsFlexContainerFrame.cpp
@@ +2948,5 @@
> +  // If our main axis is (internally) reversed, swap the justify-content
> +  // "flex-start" and "flex-end" behaviors:
> +  // NOTE: This must happen before we resolve any other justify values to
> +  // flex-start/end.
> +  if (aAxisTracker.AreAxesInternallyReversed()) {

This "This must happen" text is not entirely correct, as it turns out - and there's a bug in this patch, as a result. tl;dr, this chunk needs to insert a bit lower down.

This comment *is* true for "start"/"end" (we need to do the swapping before that part), but it's not true for other values that we simplify, like "space-between", which falls back to "flex-start".  If our axes are internally reversed (e.g. we have column-reverse and we're pretending that the top edge is the flex-start edge even though really it's not), then we want "justify-content:space-between" for the single-child case to snap children to the bottom (the REAL flex-start side), not to the top (the side that we're pretending is flex-start).


So: this comment needs to be adjusted to refer specifically to "start/end values" (I think those are the only ones that are entirely writing-mode dependent? That's the key category that we want to handle after we've done this switcheroo).  And this chunk needs to not be moved up this high -- it needs to remain after the "space-between" special-case value simplification.

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-vert-006-ref.xhtml
@@ +77,3 @@
>      </div>
>  
>      <!-- space-between -->

This reference case chunk (for space-between for a single flex item) is the what tipped me off to the code issue quoted above, BTW.

Right now this reference case is wrong -- for a single flex item, space-between is supposed to render exactly like "flex-start".  But this reference case has flex-start snapped to the bottom, vs. space-between snapped to top.

::: layout/reftests/w3c-css/submitted/flexbox/reftest.list
@@ +1,1 @@
> +#Tests for absolutely-positioned children of a flex container

Please revert the tweak to this line (looks like this patch is deleting a space between "#" and "T" here).
The "This must happen" comment might want to be clarified to something like:

  // NOTE: This must happen...
  //  - *after* we've done value-simplification for values that are dependent
  //    on our flex-axis reversedness; e.g. for "space-between" which
  //    specifically behaves like to "flex-start" in some cases (per spec),
  //    and hence depends on the reversedness of flex axes.
  //  - *before* we've done simplification for values that don't care about
  //    flex-relative axis directions; e.g. for "start" which purely depends
  //    on writing-mode and isn't affected by reversedness of flex axes.

The patch seems to get this correct (in the code) for align-content, but just not for justify-content.  You might want to include a long comment like the above in one of the places (say, for justify-content where you've already got an earlier version of this comment), and then hint at the comment from the other place, e.g.
  // NOTE: It matters precisely when we do this; see comment alongside
  // MainAxisPositionTracker's AreAxesInternallyReversed check.
Comment on attachment 8999753 [details] [diff] [review]
Patch to avoid using Phabricator

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

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-vert-005.xhtml
@@ +65,5 @@
> +        align-self: self-end;
> +      }
> +      .dirrtl {
> +        direction: rtl;
> +        justify-content: left;

This "justify-content: left" probably wants to be removed...  It doesn't seem to affect the rendering (this is a class that's set on flex-items which typically don't care about "justify-content"), and it doesn't seem to be part of what this CSS class is meaning to impose (its name "dirrtl" sounds like it's just meant to encompass direction:rtl).

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-vert-rtl-005.xhtml
@@ +66,5 @@
> +        align-self: self-end;
> +      }
> +      .dirrtl {
> +        direction: rtl;
> +      }

You should remove the "dirrtl" class from this test (here and in the HTML markup below, e.g. "<div class="self-start dirrtl">self-start</div>")

The "direction" property is inherited, and you're already setting "direction: rtl" on the flex container.  So every element that you're giving "dirrtl" here *already has* direction:rtl by default, via CSS inheritance.

Or / maybe better: if you like, it might be interesting to test "direction: ltr" on these items (with a dirltr class) - that might be an interesting scenario to test here (it'd trigger item-vs-container disagreement on "start" side, which maybe (?) is what you wanted to be testing here?)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8999753 - Attachment is obsolete: true
Attachment #8999753 - Flags: review?(dholbert)
Attachment #8999777 - Flags: review?(dholbert)
Comment on attachment 8999777 [details] [diff] [review]
Patch v2

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

::: layout/generic/nsFlexContainerFrame.cpp
@@ +3210,4 @@
>    // If our cross axis is (internally) reversed, swap the align-content
>    // "flex-start" and "flex-end" behaviors:
> +  // NOTE: It matters previsely when we do this; see comment alongside
> +  // MainAxisPositionTracker's AreAxesInternallyReverse check.

Typos:
 (1) s/previsely/precisely/
 (2) Missing final letter of of "AreAxesInternallyReversed"

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-content-horiz-002.xhtml
@@ +12,2 @@
>      <link rel="help" href="http://www.w3.org/TR/css-flexbox-1/#align-content-property"/>
> +    <link rel="match" href="flexbox-align-content-horiz-002-ref.xhtml"/>

It looks like this test (flexbox-align-content-horiz-002.xhtml) is identical to -001a (the file it's copied from), except that it's just got a bit of extra content.

So I think this is just a longer/more-comprehensive copy of -001.  And that makes -001 itself become unnecessary/redundant, basically.

Rather than making this longer copy, can we just edit -001a (and 001b) in place to be longer/more comprehensive?

This goes for several of the tests here, which are just copies-with-extra-content-added:
 - flexbox-align-content-horiz-002.xhtml (this test)
 - flexbox-align-content-vert-002.xhtml
 - flexbox-align-self-horiz-006.xhtml
 - flexbox-align-self-vert-005.xhtml
 - Maybe some of the justify-content tests, too - haven't looked through those in detail yet but I'm guessing they might have the same scenario.

(Generally, it looks like the "wmvert" tests are legitimately new/independent tests, but maybe most/all of the others are just copies-with-extra-content that should really just be morphed in as extending the current tests that they're currently copying from.)

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-content-wmvert-001.xhtml
@@ +14,4 @@
>      <style>
>        div.flexbox {
> +        width: 200px; /* Skinny, to force us to wrap */
> +        height: 20px;

This comment is incorrect -- should be one line down (alongside the 'height' decl) and should say "Short, to force us to wrap".

(It should match flexbox-align-content-vert-001a.xhtml.  In a perfect world, it'd probably be better for this testcase to be an "hg cp" of that file -- right now it's a copy of the -horiz file instead -- but it's not a huge deal & not worth worrying about changing at this point.)
Attachment #8999777 - Flags: review?(dholbert) → review-
Attached patch Bug1472843v3.patch (obsolete) — Splinter Review
Attachment #8999777 - Attachment is obsolete: true
Attachment #9001310 - Flags: review?(dholbert)
Comment on attachment 9001310 [details] [diff] [review]
Bug1472843v3.patch

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

Looks great! Thanks for the patch.

r=me with 4 lines removed from reftest.list, I think, per below.

Ping me to land this once your final Try run is complete, assuming it looks green (aside from failures from those 4 extra lines in reftest.list not finding their files)

::: layout/reftests/w3c-css/submitted/flexbox/reftest.list
@@ +13,3 @@
>  == flexbox-align-content-vert-001a.xhtml  flexbox-align-content-vert-001-ref.xhtml
>  == flexbox-align-content-vert-001b.xhtml  flexbox-align-content-vert-001-ref.xhtml
> +== flexbox-align-content-vert-002.xhtml  flexbox-align-content-vert-002-ref.xhtml

Looks like reftest.list needs a tweak to remove the lines for the files that you were previously creating but have now removed (the ones you merged back into the original test per middle of comment 12).

In particular, these files no longer exist:
 flexbox-align-content-horiz-002*
 flexbox-align-content-vert-002*
 flexbox-align-self-horiz-006*
 flexbox-align-self-vert-005*
Attachment #9001310 - Flags: review?(dholbert) → review+
Attachment #9001310 - Attachment is obsolete: true
Attachment #9001370 - Flags: review+
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/809eca6fc4b7
Implement justify-content:left/right/start/end, align-content:start/end, align-self:self-start/self-end for flexbox. r=dholbert
Comment on attachment 9001370 [details] [diff] [review]
Bug1472843v4.patch

>diff --git a/layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-horiz-005.xhtml b/layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-horiz-006.xhtml
>copy from layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-horiz-005.xhtml
>copy to layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-horiz-006.xhtml
>--- a/layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-horiz-005.xhtml
>+++ b/layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-horiz-006.xhtml
>@@ -7,13 +7,16 @@
>      property, in an auto-sized horizontal flex container. -->
> <html xmlns="http://www.w3.org/1999/xhtml">
>   <head>
>-    <title>CSS Test: Testing 'justify-content' in an auto-sized horizontal flex container</title>
>-    <link rel="author" title="Daniel Holbert" href="mailto:dholbert@mozilla.com"/>
>+    <title>CSS Test: Testing 'justify-content' in an auto-sized reversed horizontal flex container</title>
>+    <link rel="author" title="Mihir Iyer" href="mailto:miyer@mozilla.com"/>
>     <link rel="help" href="http://www.w3.org/TR/css-flexbox-1/#justify-content-property"/>
>-    <link rel="match" href="flexbox-justify-content-horiz-005-ref.xhtml"/>
>+    <link rel="match" href="flexbox-justify-content-horiz-006-ref.xhtml"/>
>     <style>
>       div.flexbox {
>         display: flex;
>+        flex-direction: row-reverse;
>+        width: 200px;
>+        margin-bottom: 2px;
>       }

I just noticed that the two new justify-content tests (this one and its "vert" equivalent) are effectively duplicates of the pre-existing (previously-"failing" with phony reference cases) tests that live in layout/reftests/flexbox -- the ones with "horizrev" and "vertrev" in the name whose reference cases needed tweaking in this commit.

I'll push a followup commit to delete the older non-w3c-submitted copies of these reftests. (I was originally going to file a bug on moving them to the w3c-css/submitted directory, but then I realized we've basically we've already got a new copy of them here.)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f499acc5b5
followup: Remove flexbox-justify-content-horizrev & vertrev reftest files, as they now have duplicates in another directory. (no review, test-tweak only)
Blocks: 1483685
https://hg.mozilla.org/mozilla-central/rev/809eca6fc4b7
https://hg.mozilla.org/mozilla-central/rev/b0f499acc5b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: