Daniel Holbert [:dholbert] Reporter
	Description • 8 years ago

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

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 8 years ago

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.

	Daniel Holbert [:dholbert] Reporter
	Comment 4 • 8 years ago

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.

	Daniel Holbert [:dholbert] Reporter
	Comment 5 • 8 years ago

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. :)

	Brad Werth [:bradwerth] Assignee
	Comment 8 • 8 years ago

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).

	Brad Werth [:bradwerth] Assignee
	Comment 12 • 8 years ago

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.

	Mats Palmgren (inactive)
	Comment 18 • 8 years ago
mozreview-review

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?

	Mats Palmgren (inactive)
	Comment 19 • 8 years ago

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.

	Brad Werth [:bradwerth] Assignee
	Comment 20 • 8 years ago
mozreview-review-reply

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.

	Mats Palmgren (inactive)
	Comment 21 • 8 years ago
mozreview-review

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.

	Mats Palmgren (inactive)
	Comment 22 • 8 years ago
mozreview-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

	Mats Palmgren (inactive)
	Comment 23 • 8 years ago
mozreview-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

	Mats Palmgren (inactive)
	Comment 24 • 8 years ago
mozreview-review

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?

	Mats Palmgren (inactive)
	Comment 25 • 8 years ago
mozreview-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

	Mats Palmgren (inactive)
	Comment 26 • 8 years ago

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

	Mats Palmgren (inactive)
	Comment 32 • 8 years ago
mozreview-review

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

	Brad Werth [:bradwerth] Assignee
	Comment 33 • 8 years ago

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.

	Daniel Holbert [:dholbert] Reporter
	Comment 39 • 8 years ago

(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])

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 8 years ago

Autoland couldn't rebase this for landing.

	Brad Werth [:bradwerth] Assignee
	Comment 46 • 8 years ago

Rebased and retested, hopefully reaady to go now.

	Pulsebot
	Comment 47 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 48 • 8 years ago
bugherder

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

	yisibl
	Comment 49 • 8 years ago

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>

	Chris Mills [:cmills]
	Comment 50 • 7 years ago

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.

	Brad Werth [:bradwerth] Assignee
	Comment 51 • 7 years ago

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

	Chris Mills [:cmills]
	Comment 52 • 7 years ago

(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.

	Mats Palmgren (inactive)
	Comment 54 • 7 years ago

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