Closed Bug 1235922 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox46 --- affected
firefox52 --- fixed

People

(Reporter: dholbert, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 3 obsolete files)

58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
58 bytes, text/x-review-board-request
mats
: review+
Details
Spinning this off of bug 1207698, to cover "justify-content:space-evenly" support for flexbox.
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.
Attachment #8703019 - Flags: review?(mats)
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.
Attachment #8703019 - Flags: review?(mats)
Summary: [css-flexbox] Implement flexbox layout for "justify-content: space-evenly" → [css-flexbox] Implement flexbox layout for "justify-content: space-evenly" & "align-content: space-evenly"
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. :)
Assignee: dholbert → bwerth
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).
Attachment #8703019 - Attachment is obsolete: true
Attachment #8703021 - Attachment is obsolete: true
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.
Attachment #8703022 - Attachment is obsolete: true
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?
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 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 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.
Attachment #8800455 - Flags: review?(mats) → review+
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
Attachment #8800456 - Flags: review?(mats) → review+
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 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?
Attachment #8800856 - Flags: review?(mats) → review+
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
Attachment #8800855 - Flags: review?(mats) → review+
Probably worth adding a MOZ_ASSERT(aNumThingsToPack > 0) in the function too,
since it actually depends on that.
Attachment #8800751 - Flags: review?(mats)
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
Attachment #8800751 - Flags: review?(mats) → review+
Keywords: checkin-needed
Keywords: checkin-needed
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.
Keywords: checkin-needed
(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])
Autoland couldn't rebase this for landing.
Keywords: checkin-needed
Rebased and retested, hopefully reaady to go now.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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>
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.
The space- values are defined in the css-align spec at https://drafts.csswg.org/css-align-3/#distribution-values.
Flags: needinfo?(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.
Flags: needinfo?(cmills)
I spawned off the last comment as a separate bug instead: bug 1335106.
Please reply there instead about MDN content.
You need to log in before you can comment on or make changes to this bug.