1235922 - [css-flexbox] Implement flexbox layout for "justify-content: space-evenly" & "align-content: space-evenly"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Spinning this off of bug 1207698, to cover "justify-content:space-evenly" support for flexbox.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: Introduce a helper-function to share code for justify-content:space-around & space-between (and space-evenly, in next patch)

This patch doesn't change behavior -- it just refactors existing behavior into a helper-function, in a way that lets us implement "space-evenly" easily in the next patch.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: add support for "space-evenly"

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 3: update justify-content reftests to test "space-evenly"

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8703019 [details] [diff] [review]
part 1:  Introduce a helper-function to share code for justify-content:space-around & space-between (and space-evenly, in next patch)

Oops, I just realized this value applies to "align-content", too -- so I should really do this in a way where I can share code between "justify-content" & "align-content" here.

Canceling review; will repost a patch-stack with that fixed.

Comment 5

[Daniel Holbert [:dholbert]]

Sorry for dropping the ball here.  IIRC, in comment 4, I was planning on rewriting "part 1" with C++ templates so that it could be shared for "justify-content" (which operates on a single FlexLine, i.e. a list of FlexItems) and "align-content" (which operates on a list of FlexLines).

The simplest way to share code is to probably adjust part 1's "SetupPackingSpaceForSpaceValues" method so that it simply:
  (1) is a static function which doesn't live on MainAxisPositionTracker
  (2) takes argument "uint32_t aNumThingsToAlign" instead of "const FlexLine* aLine"
  (3) returns mNumPackingSpacesRemaining and mPackingSpaceRemaining as outparams (since it won't be a class method anymore & hence won't have a "this" pointer)
  (4) takes argument "uint8_t aAlignVal" instead of using mJustifyContent (again, since we won't have a "this" pointer)

Then (still in Part 1) we'd invoke it like this:
  SetupPackingSpaceForSpaceValues(aLine->NumItems(), mJustifyContent,
                                  &mNumPackingSpacesRemaining,
                                  &mPackingSpaceRemaining);

And we should update part 1 to add a similar invocation in CrossAxisPositionTracker's constructor, to replace its existing space-between/space-around cases (all without changing behavior).

WITH THAT, parts 2 and 3 should still work just fine and should get this new value implemented for "justify-content" -- and you could add parts 3 and 4 which would get this new value implemented for "align-content".

Brad has volunteered to take this on, so I'll reassign this to him.  Brad, feel free to ping me if any of the above doesn't make sense. Mats should probably be the reviewer here, since I've had enough of a hand in crafting the existing & upcoming patches that I wouldn't be able to be an unbiased/effective 3rd-party reviewer. :)

Comment 6

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

Review commit: https://reviewboard.mozilla.org/r/85348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85348/

Comment 7

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1235922 Part 2: Add support for "justify-content: space-evenly" to flexbox layout.

Review commit: https://reviewboard.mozilla.org/r/85350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85350/

Comment 8

[Brad Werth [:bradwerth] | Out until Feb 2025]

Pushed updated patches for Part 1 (refactoring for justify-space-between and justify-space-around) and Part 2 (add support for justify-space-evenly) to MozReview with r?mats. I'll soon push a Part 2b that adds support for align-content-*, and then finally the original Part 3 (tests).

Comment 9

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1235922 Part 3: Extend flexbox "justify-content" reftests to test new "space-evenly" value.

Review commit: https://reviewboard.mozilla.org/r/85604/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85604/

Comment 10

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85348/diff/1-2/

Comment 11

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800456 [details]
Bug 1235922 Part 2: Add support for "justify-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85350/diff/1-2/

Comment 12

[Brad Werth [:bradwerth] | Out until Feb 2025]

Re-read comment 5 and structured the patches as dholbert suggests. Part 1 now also refactors existing behavior for align-content space-between and space-around. Forthcoming Parts 4 and 5 will add support for align-content space-evenly and add tests for that value.

Comment 13

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1235922 Part 4: Add support for "align-content: space-evenly" to flexbox layout.

Review commit: https://reviewboard.mozilla.org/r/85682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85682/

Comment 14

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1235922 Part 5: Extend flexbox "align-content" reftests to test new "space-evenly" value.

Review commit: https://reviewboard.mozilla.org/r/85684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85684/

Comment 15

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85348/diff/2-3/

Comment 16

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800456 [details]
Bug 1235922 Part 2: Add support for "justify-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85350/diff/2-3/

Comment 17

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800751 [details]
Bug 1235922 Part 3: Extend flexbox "justify-content" reftests to test new "space-evenly" value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85604/diff/1-2/

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

https://reviewboard.mozilla.org/r/85348/#review84346

This looks fine so far, but perhaps it would be better to make it a member
function on PositionTracker instead?
It looks like both Main/CrossAxisPositionTracker have these members:
mAlign/JustifyContent, mNumPackingSpacesRemaining, mPackingSpaceRemaining
so it seems we could just move those up to PositionTracker instead since
at least the code here is identical in both axis.

(I guess that would waste a little space for SingleLineCrossAxisPositionTracker
which I assume don't need these members, but I suspect that's OK since
we only have one of those per (single-line) FlexContainerFrame per reflow.)

(We could even go on and extend it to handle start/end/center the same too,
but let's do that in a separate patch or follow-up bug if so.)

WDYT?

Comment 19

[Mats Palmgren (inactive)]

BTW, we tend to prefer using the NS_STYLE_ALIGN_* constants for common code
that handles both align/justify properties.  These constants are defined
to always (and forever) be the same values.

Comment 20

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

https://reviewboard.mozilla.org/r/85348/#review84346

I had started on a superclass-based solution and it started to generate ugly code. It felt weird to have some member variables that were unused by one of the subclasses, and although mAlignContent and mJustifyContent have the same type and values assigned, they have different semantic meanings and it didn't feel great to mash them together. The member function was going to need some values passed in (things to count and possibly the alignment constant) so the benefit of accessing SOME of the data as members and some of it as parameters was iffy. dholbert and I had an exchange on the matter in #layout:

<•dholbert> bradwerth, hi! hopefully the comment I just left on https://bugzilla.mozilla.org/show_bug.cgi?id=1235922 makes sense.  I mentioned some intentions around C++ templates at the beginning, but to be clear, I don't think we should actually bother using templates
(i think I had some imagined vision for how we could share code using templates and/or inheritance from the PositionTracker superclass -- but I think it's simpler to just use a file-scope static helper-function, which just takes all of the data/outparam that it needs as args)
<bradwerth> dholbert: thanks for the additional guidance. I had just tentatively started on a subclass-based solution and decided it had a lot of problems
static function sounds great. I'll pursue what you suggest 
<•dholbert> bradwerth, great! thanks
bradwerth, yeah, the subclass-based solution is tempting there, but the problems gave me a mental block and then the bug never rose back to the top of my queue :)

All that said, the static function could be made to live on PositionTracker if that tidies things up better.

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

https://reviewboard.mozilla.org/r/85348/#review84662

r=mats with those addressed

::: layout/generic/nsFlexContainerFrame.h:89
(Diff revision 3)
>  
> +  static void CalculatePackingSpace(uint32_t aNumThingsToPack,
> +                                    uint8_t aAlignVal,
> +                                    nscoord& aFirstSubjectOffset,
> +                                    uint32_t& aNumPackingSpacesRemaining,
> +                                    nscoord& aPackingSpaceRemaining);

Fair enough.  I'd definitely use a base class method instead
since the code for both axis is pretty much identical, but
I'll defer to dholbert on that.  Anyway, that can happen
later if we decide it's worth it after all.

These three out params should be pointers, not references,
to force the call sites to use &variable to make it clearer
which things are being modified by the call.

Please also write a short doc comment saying that it modifies
the out param values for space-between/around/evenly.

::: layout/generic/nsFlexContainerFrame.cpp:2874
(Diff revision 3)
>          MOZ_ASSERT(mPackingSpaceRemaining >= 0,
> -                   "negative packing space should make us use 'flex-start' "
> +                   "negative packing space should have been handled already");

It looks like we can move this assertion into the function, right?
If so, I think I'd prefer that.

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 8800456 [details]
Bug 1235922 Part 2: Add support for "justify-content: space-evenly" to flexbox layout.

https://reviewboard.mozilla.org/r/85350/#review84674

::: layout/generic/nsFlexContainerFrame.cpp:4003
(Diff revision 3)
> +    aAlignVal == NS_STYLE_JUSTIFY_SPACE_AROUND ?
> +    1 : 2;

minor nit: I'd put these on the same line

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8800855 [details]
Bug 1235922 Part 4: Add support for "align-content: space-evenly" to flexbox layout.

https://reviewboard.mozilla.org/r/85682/#review84676

::: layout/generic/nsFlexContainerFrame.cpp:2862
(Diff revision 1)
>        case NS_STYLE_ALIGN_SELF_START:
>        case NS_STYLE_ALIGN_SELF_END:
> -      case NS_STYLE_ALIGN_SPACE_EVENLY:
>        case NS_STYLE_ALIGN_BASELINE:
>        case NS_STYLE_ALIGN_LAST_BASELINE:
>          NS_WARNING("NYI: align-self:left/right/self-start/self-end/space-evenly/baseline/last-baseline");

nit: remove "space-evenly/" here

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 8800856 [details]
Bug 1235922 Part 5: Extend flexbox "align-content" reftests to test new "space-evenly" value.

https://reviewboard.mozilla.org/r/85684/#review84680

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-content-horiz-001-ref.xhtml:133
(Diff revision 1)
> +    <!-- space-evenly -->
> +    <div class="flexbox">
> +      <div class="a"/>
> +    </div>
> +    <div class="flexbox">
> +      <div class="a" style="margin-top: 53.333333px"/>

Maybe "calc(160px / 3)" makes more sense?

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 8800855 [details]
Bug 1235922 Part 4: Add support for "align-content: space-evenly" to flexbox layout.

https://reviewboard.mozilla.org/r/85682/#review84682

Comment 26

[Mats Palmgren (inactive)]

Probably worth adding a MOZ_ASSERT(aNumThingsToPack > 0) in the function too,
since it actually depends on that.

Comment 27

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85348/diff/3-4/

Comment 28

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800456 [details]
Bug 1235922 Part 2: Add support for "justify-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85350/diff/3-4/

Comment 29

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800751 [details]
Bug 1235922 Part 3: Extend flexbox "justify-content" reftests to test new "space-evenly" value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85604/diff/2-3/

Comment 30

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800855 [details]
Bug 1235922 Part 4: Add support for "align-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85682/diff/1-2/

Comment 31

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800856 [details]
Bug 1235922 Part 5: Extend flexbox "align-content" reftests to test new "space-evenly" value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85684/diff/1-2/

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 8800751 [details]
Bug 1235922 Part 3: Extend flexbox "justify-content" reftests to test new "space-evenly" value.

https://reviewboard.mozilla.org/r/85604/#review84742

Comment 33

[Brad Werth [:bradwerth] | Out until Feb 2025]

Whoops, just now noticing the issues raised in comment 22, comment 23, comment 24, and comment 26. Will address, re-push, and then set checkin-needed.

Comment 34

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85348/diff/4-5/

Comment 35

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800456 [details]
Bug 1235922 Part 2: Add support for "justify-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85350/diff/4-5/

Comment 36

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800751 [details]
Bug 1235922 Part 3: Extend flexbox "justify-content" reftests to test new "space-evenly" value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85604/diff/3-4/

Comment 37

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800855 [details]
Bug 1235922 Part 4: Add support for "align-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85682/diff/2-3/

Comment 38

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800856 [details]
Bug 1235922 Part 5: Extend flexbox "align-content" reftests to test new "space-evenly" value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85684/diff/2-3/

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #21)
> Fair enough.  I'd definitely use a base class method instead
> since the code for both axis is pretty much identical, but
> I'll defer to dholbert on that.  Anyway, that can happen
> later if we decide it's worth it after all.

(I agree that this would be preferable, and I think it probably would be worth doing -- but I think that involves a lot of refactoring/renaming (probably including collapsing some axis-specific member variables together in the PositionTracker superclass, adding convenience accessors in the base class, etc).  And I don't want to gate the changes here on that refactoring -- that's why I suggested Brad go with a simpler static-method superclass.

Thanks for taking the reviews here! [and thanks Brad for taking this])

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Autoland couldn't rebase this for landing.

Comment 41

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800455 [details]
Bug 1235922 Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85348/diff/5-6/

Comment 42

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800456 [details]
Bug 1235922 Part 2: Add support for "justify-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85350/diff/5-6/

Comment 43

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800751 [details]
Bug 1235922 Part 3: Extend flexbox "justify-content" reftests to test new "space-evenly" value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85604/diff/4-5/

Comment 44

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800855 [details]
Bug 1235922 Part 4: Add support for "align-content: space-evenly" to flexbox layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85682/diff/3-4/

Comment 45

[Brad Werth [:bradwerth] | Out until Feb 2025]

Comment on attachment 8800856 [details]
Bug 1235922 Part 5: Extend flexbox "align-content" reftests to test new "space-evenly" value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85684/diff/3-4/

Comment 46

[Brad Werth [:bradwerth] | Out until Feb 2025]

Rebased and retested, hopefully reaady to go now.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af377e9d9ddb
Part 1: Introduce a helper-function to share code for justify-content and align-content space-around and space-between in flexbox layout. r=mats
https://hg.mozilla.org/integration/autoland/rev/c6b2288a20ec
Part 2: Add support for "justify-content: space-evenly" to flexbox layout. r=mats
https://hg.mozilla.org/integration/autoland/rev/ae5fd2247144
Part 3: Extend flexbox "justify-content" reftests to test new "space-evenly" value. r=mats
https://hg.mozilla.org/integration/autoland/rev/e709bed8b918
Part 4: Add support for "align-content: space-evenly" to flexbox layout. r=mats
https://hg.mozilla.org/integration/autoland/rev/b827ae5bb9e3
Part 5: Extend flexbox "align-content" reftests to test new "space-evenly" value. r=mats

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af377e9d9ddb
https://hg.mozilla.org/mozilla-central/rev/c6b2288a20ec
https://hg.mozilla.org/mozilla-central/rev/ae5fd2247144
https://hg.mozilla.org/mozilla-central/rev/e709bed8b918
https://hg.mozilla.org/mozilla-central/rev/b827ae5bb9e3

Comment 49

[yisibl]

Test Case 

data:text/html,<html><body><div class="box"> <div class="list">1</div> <div class="list">2</div> <div class="list">3</div> <div class="list">4</div> </div> <style>.box{display:flex;border:1px solid blue;width:300px;height:200px;align-content:space-evenly;justify-content:space-evenly;flex-wrap:wrap;}.list{margin-right:10px;width:100px;height:30px;line-height:30px;text-align:center;background-color:lightblue;}</style></body></html>

Comment 50

[Chris Mills [:cmills]]

I've added these new values to the relevant property ref pages, making sure the descriptions of these and similar properties make sense, and updating browser support info:
https://developer.mozilla.org/en-US/docs/Web/CSS/align-content
https://developer.mozilla.org/en-US/docs/Web/CSS/justify-content

I've also made sure that it is mentioned in the Fx 52 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#CSS

Let me know if this is OK. Thanks!

Follow up question too - where is this value specced? It doesn't appear to be in the editor's draft.

Comment 51

[Brad Werth [:bradwerth] | Out until Feb 2025]

The space- values are defined in the css-align spec at https://drafts.csswg.org/css-align-3/#distribution-values.

Comment 52

[Chris Mills [:cmills]]

(In reply to Brad Werth [:bradwerth] from comment #51)
> The space- values are defined in the css-align spec at
> https://drafts.csswg.org/css-align-3/#distribution-values.

Ah, that's great - thanks Brad. I've added the correct spec reference in the spec table for the new value.

Comment 53

[Mats Palmgren (inactive)]

The authoritative spec for all the align-/justify- properties is css-align now.
These properties originated in the flexbox spec and only applied there, but were
later generalized to apply to Grid, Block, Table, and Column layout and thus
were moved into the dedicated spec (css-align) that specifies the behavior for all
layout modes (we only support them in Flexbox and Grid layout so far though).

It seems these properties are not listed at all for Grid:
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout

'justify-self' is missing (presumably b/c it doesn't apply to Flexbox, but
it does apply to Grid though):
https://developer.mozilla.org/en-US/docs/Web/CSS/justify-self

Also, the 'first baseline' / 'last baseline' values are missing.

I'm not sure how to organize this best in MDN.  I'm guessing the most
convenient for the reader is to list them in each layout mode, but that
means some duplication.  The landing page for each property though, needs
to describe the behavior for all layout modes (or at least the ones we
support).  For example,
https://developer.mozilla.org/en-US/docs/Web/CSS/align-self
talks about flex items and flex lines, which only makes sense for flexbox
layout and would be rather confusing for someone that wants to know what
the property does in a grid layout.

You could also look at how css-align is organized to solve this problem.
It basically describes the behavior using abstract concepts, like "alignment
context / alignment subject", and then documents what a "context" and
"subject" is in each mode.  See for example:
https://drafts.csswg.org/css-align-3/#self-alignment
and then 6.1.2 - 6.1.6

Comment 54

[Mats Palmgren (inactive)]

I spawned off the last comment as a separate bug instead: bug 1335106.
Please reply there instead about MDN content.