Mats Palmgren (inactive) Assignee
	Description • 9 years ago

http://dev.w3.org/csswg/css-grid/#grid-align

	Pulsebot
	Comment 1 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a08426c6483

	Carsten Book [:Tomcat]
	Comment 2 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3a08426c6483

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 3 • 9 years ago

This shouldn't have been resolved by just that patch (which was posted/reviewed in bug 1176782).

	Mats Palmgren (inactive) Assignee
	Comment 4 • 9 years ago

I've mostly implemented this for grid items, but there is a remaining
regression when stretching (I think).  I've implemented stretching by
simply adjusting the block- or inline-size to fill the grid area.
The problem is that this offsets the children in some cases,
since they appear to be relative to the end edge(?)

Do I have to explicitly reposition the child frames of the grid item
after resizing it?  If so, when and how?  Do we have code that does
this already that I can use?

(I'll attach a screenshot from my local build -- the bogus rendering
is item 1.  I think it should look like item 3 instead.)

	Jonathan Kew [:jfkthame]
	Comment 6 • 9 years ago

If I'm understanding correctly, the issue here is that you're changing the block-size (width) of the container after having reflowed its children, right? Yes, that will cause this problem because although the positions of the child frames are computed logically during reflow, they are then stored (in their mRect) as physical coordinates relative to the physical (top-left) origin of the container.

The conversion to physical coordinates happens during ReflowChild/FinishReflowChild, IIRC, and depends on the containerSize parameter that's passed to these methods. I guess you're currently passing a containerSize that does not include the stretch amount.

If it's possible to calculate this and incorporate it into the containerSize by the time you get to FinishReflowChild on the grid items, I think that would fix things. But if (as seems likely) that's not possible -- because you can't calculate the stretch until after fully reflowing all the children -- then you'll need to explicitly fix up the positions of the child frames afterwards. We have a few places in the code where we do things like that: e.g. see nsBlockFrame::Reflow, after it calls ComputeFinalSize().

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 7 • 9 years ago

mats, is this worth a release note? I'm just scanning quickly through the fixed bug for aurora and this caught my eye. thanks.

	Mats Palmgren (inactive) Assignee
	Comment 8 • 9 years ago

No, there is nothing to see here yet.

	Mats Palmgren (inactive) Assignee
	Comment 9 • 9 years ago

I believe the 'start'/'end' additions here are actually the correct
implementation.  The other ones are not, obviously.  FYI, 'left'/'right'
and 'self-start'/'self-end' are easy to implement, the just map to
the start/end code you already have, depending on some writing mode
values (see my Grid implementation for those).

(I filed bug 1207698 on completing the flexbox layout.)

	Mats Palmgren (inactive) Assignee
	Comment 12 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af561fc4a380

	Daniel Holbert [:dholbert]
	Comment 13 • 9 years ago

Comment on attachment 8664998 [details] [diff] [review]
part 2 - [css-flexbox] Shim implemention for the new align/justify property values in flexbox layout (just to avoid fatal assertions).

r=me

(Ideally all of the XXX comments and NYI warnings here should include a mention of bug 1207698. Maybe not a big deal, though, since I expect you or I will fix that bug soon.)

	Daniel Holbert [:dholbert]
	Comment 14 • 9 years ago

Comment on attachment 8665013 [details] [diff] [review]
part 3 - [css-grid] Implement layout for the 'justify-content' and 'align-content' CSS properties.

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

Sorry for delay on this -- haven't made it all the way through AlignJustifyContent yet, but here's a partial review:

::: layout/generic/nsGridContainerFrame.cpp
@@ +2468,5 @@
>  }
>  
>  void
> +nsGridContainerFrame::Tracks::AlignJustifyContent(
> +  const nsStylePosition* aStylePosition,

This function is a bit long. Are there significant pieces of it that would make sense to refactor into helper functions?

For example: the entire first switch statement seems like it could maybe be factored into a
   alignment = FixupAlignJustifyContentEnum(alignment, isAlign);
one-liner, perhaps.

@@ +2489,5 @@
> +  switch (alignment) {
> +    case NS_STYLE_ALIGN_LEFT:
> +    case NS_STYLE_ALIGN_RIGHT: {
> +      if (isAlign) {
> +        alignment = NS_STYLE_ALIGN_START; // XXX style system should do this? (csswg)

Nit: Line >80 characters. (Shorten the comment, or maybe bump it to the previous line?)

Also: Maybe add a short explanatory comment for this conversion? Something like:
  // Grid's align-content axis is never parallel to inline axis,
  // so the used value of align-content:left|right is 'start'.

(This is comment-worthy IMO because it's specifically true for grid, but not for e.g. flexbox.  There, if we have 'flex-direction:column', align-content's axis will be parallel to the inline axis, so we'll actually have to honor left/right.)

(Also: the spec says the "computed value" of left/right is start, but based on an email from fantasai I'm assuming/hoping that's changing to "used value": https://lists.w3.org/Archives/Public/www-style/2015Sep/0209.html )

	Daniel Holbert [:dholbert]
	Comment 15 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #14)
> ::: layout/generic/nsGridContainerFrame.cpp
> This function is a bit long. Are there significant pieces of it that would
> make sense to refactor into helper functions?

Alternately/also: a one-liner comment describing each of the major sections would help break down this large function into more-understandable pieces & follow what's going on more easily.

Sample comments which I think describe the main chunks of this function:
 // Fix up alignment values whose primary behavior doesn't apply to grid.
 ...
 // Compute free space & count auto-sized tracks.
 ...
 // See if we need to distribute space to/between tracks; return early if not.
 ...
 // Decide where 1st track is placed, & how much space space goes where.
 ...
 // Distribute the space.

	Daniel Holbert [:dholbert]
	Comment 16 • 9 years ago

Comment on attachment 8665013 [details] [diff] [review]
part 3 - [css-grid] Implement layout for the 'justify-content' and 'align-content' CSS properties.

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

This largely looks good, but I have concerns about how we deal with division error, as noted in my last 2 notes below. (These are related to the discussion we had on bug 1176621 -- in particularly, my comment about goals (1) and (2) in bug 1176621 comment 9 is directly applicable.  And here, the spec doesn't as directly lead you down the wrong path -- and it's also easier to get things correct, since equal-distribution is easier than differential-flexing-per-thing. So, I think it's more important that we do it correctly here; hence my pushback.)

::: layout/generic/nsGridContainerFrame.cpp
@@ +2553,5 @@
> +  switch (alignment) {
> +    case NS_STYLE_ALIGN_STRETCH: {
> +      if (numAutoTracks == 0) {
> +        break; // fallback to 'start'
> +      }

If you like: it seems to me like this scenario ("stretch" with no auto tracks) would make more sense if we handled this as part of the switch statement directly above this one.  It'd be a bit truer to its comment, too -- since the switch statement before this one is where we actually handle 'start'.

I think the following sample-code should do it, inserted into the previous switch-statement. (Note that this truly makes us do exactly the 'start' codepath for this case, since 'start' just sets distribute to false):

 case NS_STYLE_ALIGN_STRETCH:
   if (numAutoTracks == 0) {
     distribute = false;
   }
   break;

And then in the final switch statement's stretch case, we could simply assert that numAutoTracks is nonzero just before we divide by it.

(On the other hand, your formulation is maybe 1 or 2 lines shorter, due to skipping the need for this case in the earlier switch statement; so it's also fine as-is if you prefer.)

@@ +2569,5 @@
> +        if (space == 0) {
> +          spacePerTrack = 0;
> +        }
> +      }
> +      MOZ_ASSERT(space == 0, "we didn't distribute all space?");

I think this assertion can be made to fail.  Suppose for example we have space=3 and numAutoTracks=2. "spacePerTrack" will be 1, so we'll increase the size of both auto tracks by 1. And we'll be left with space = 1, which will fail this assertion.  So -- this needs to be finessed a little bit, to avoid fatal assertion-failures.

One quick-and-dirty solution (which I don't recommend) is to e.g. distribute all the remaining space to the last track. The downside is, the final track may be arbitrarily larger than all the others. Consider e.g. space=300 with numAutoTracks=155.  300/151 is 1.99, which will be floored to spacePerTrack = 1.  So we'll give 1 app unit to each of  the first 150 tracks, and then 150 app units to the final track. This distribution hardly seems fair (and would result in a 2px difference in size between that track and all the others). With larger numbers of tracks, this error can be made arbitrarily large.

In Flexbox, we handle this elegantly (IMO) via a "chipping away" strategy, where for each stretched flex line (analogous to a grid track), we decide how much space it gets by dividing the remaining space by the number of remaining items that are going to get a share of the space. This has the happy effect that when there's one item left, we distribute space/1 = space, which means we end up distributing all of the remaining space to that last item, and we're guaranteed to distribute all the space.  Moreover, any remainder gets smoothed out across the items, instead of all being dumped on the last one. That happens here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?mark=2685-2695&rev=31a26ed44484#2677

I'd recommend going with that strategy -- it's a bit more expensive (requiring an integer-division in each loop iteration), but it gets us correctness, and at least it's not a float-division (which IIUC is more expensive).

@@ +2574,5 @@
> +      return;
> +    }
> +    case NS_STYLE_ALIGN_SPACE_BETWEEN:
> +      if (mSizes.Length() > 1) {
> +        between = std::max<nscoord>(space / (mSizes.Length() - 1), 1);

The above issues about "stretch" & rounding error are true here, too.

Your "max" expression works around cases where the space-per-track is less than 1 (by rounding up to 1, and then stopping distribution when we run out of space).  But in cases where the true 'between' is a fractional value that's *greater* than 1 (e.g. with an adaptation of my example above, using space=300 and 151 in-between-spaces to be distributed to), we'll still end up with error accumulating as we go from track to track, and we'll have >2px left un-allocated at the end.

And in this case, that'll mean we'll have awkward empty space between the final track and the end of the grid container, which is incorrect for "space-between".  (There's supposed to be *zero* space after the last grid track.)

	Daniel Holbert [:dholbert]
	Comment 17 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #16)
> Consider e.g. space=300 with numAutoTracks=155.  300/151 is 1.99

(Sorry, I meant numAutoTracks=151 there -- not 155. [I tweaked the values in my example to try to make it clearer, but I accidentally forgot to convert that 155 to 151 in the first sentence.])

	Mats Palmgren (inactive) Assignee
	Comment 18 • 9 years ago

Good catch on the rounding errors.  I totally should have thought about that
myself given our previous discussion on the topic. :-(

Chrome doesn't handle this very well.  We can do better!

	Mats Palmgren (inactive) Assignee
	Comment 19 • 9 years ago

> (In reply to Daniel Holbert [:dholbert] from comment #14)
> Are there significant pieces of it that would
> make sense to refactor into helper functions?

I think the code is clearer with the code inline.  It allows the reader
to see that 'flex-start' etc is handled upfront and need not be handled
in the switch-es that follows, etc.  I'll add a few comments to clarify
what each section does.

>   // Grid's align-content axis is never parallel to inline axis,
>   // so the used value of align-content:left|right is 'start'.

Added.


> (In reply to Daniel Holbert [:dholbert] from comment #15)

I added a comment for each section along those lines.


> (In reply to Daniel Holbert [:dholbert] from comment #16)
> I have concerns about how we deal with division
> error, as noted in my last 2 notes below.

OK, I think I've found a better algorithm this time.
It basically does (once):
  roundingError = space % N;
  spacePerTrack = space / N;
then add |spacePerTrack| to each track and chip away at |roundingError|
instead (by 1).  This leads to simpler code overall IMO.  It's easy to
see that we will distribute all space exactly.  This way there's also
no need to chip-away at |space|.  Also, I found div() in stdlib.h that
does the above divisions as one instruction (on most popular CPUs
I'd guess).

If you think this looks correct, I'll see if I can wrap this up in
a generator object and use that in the other places we distribute
space in Grid too.

> > +    case NS_STYLE_ALIGN_STRETCH: {
> > +      if (numAutoTracks == 0) {
> > +        break; // fallback to 'start'
> > +      }
> 
> If you like: it seems to me like this scenario ("stretch" with no auto
> tracks) would make more sense if we handled this as part of the switch
> statement directly above this one.

Yep, good point, did so.

	Mats Palmgren (inactive) Assignee
	Comment 20 • 9 years ago

(added a test to check rounding errors for 'stretch')

	Daniel Holbert [:dholbert]
	Comment 21 • 9 years ago

(In reply to Mats Palmgren (:mats) from comment #19)
> OK, I think I've found a better algorithm this time.
> It basically does (once):
>   roundingError = space % N;
>   spacePerTrack = space / N;
[...]
> If you think this looks correct, I'll see if I can wrap this up in
> a generator object and use that in the other places we distribute
> space in Grid too.

Nice, the roundingError solution sounds good, at least for cases where we're distributing space equally like with justify/align-content.  (RE using it in other places -- I suspect it may not be workable for flexible-track-sizing, because there, we've got float weights involved instead of just integer division with remainder.)

	Daniel Holbert [:dholbert]
	Comment 22 • 9 years ago

Comment on attachment 8666487 [details] [diff] [review]
part 3 - [css-grid] Implement layout for the 'justify-content' and 'align-content' CSS properties.

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

Nearly there - just one issue w/ roundingError handling, noted below. I'll hold off on r+ until that's addressed, since it's probably worth a sanity-check on whatever the solution ends up being.

::: layout/generic/nsGridContainerFrame.cpp
@@ +2548,5 @@
> +      alignment = NS_STYLE_ALIGN_START;
> +    }
> +  }
> +
> +  // Optimize the cases where we just need to set each tracks' position.

grammar nit: s/tracks'/track's/

("each track" is singular)

@@ +2596,5 @@
> +          continue;
> +        }
> +        nscoord stretch = spacePerTrack;
> +        if (roundingError) {
> +          roundingError -= 1;

I think you're assuming roundingError is positive here.  But that's not guaranteed -- if 'space' is negative, then roundingError will be negative (or zero) too.  I just tested with this demo C++ program, which prints out a remainder of -2:

#include <stdio.h>
#include <stdlib.h>

int main() {
  div_t result = div(-8, 3);
  printf("8 divided by 3 remainder: %d\n", result.rem);
}

@@ +2603,5 @@
> +        nscoord newBase = sz.mBase + stretch;
> +        sz.mBase = newBase;
> +        pos += newBase;
> +      }
> +      MOZ_ASSERT(pos == space, "we didn't distribute all space?");

Probably worth asserting that roundingError is 0 here, too. (We expect to have distributed all of it by this point.)

@@ +2630,5 @@
> +    nscoord spacing = between;
> +    if (roundingError) {
> +      roundingError -= 1;
> +      spacing += 1;
> +    }

As above, this is incorrectly assuming that roundingError is positive.  (Please be sure you've got testcases for "align-content" & "justify-content" values with stuff that doesn't fit, which I expect should trigger this scenario.)

@@ +2632,5 @@
> +      roundingError -= 1;
> +      spacing += 1;
> +    }
> +    pos += sz.mBase + spacing;
> +  }

As above, probably worth asserting that roundingError has reached 0 when we're done here.

	Mats Palmgren (inactive) Assignee
	Comment 23 • 9 years ago

Ah, good catch on the negative space.  So then I have a question: does the
"When the alignment subjects cannot be distributed in this way"[1] apply when
we overflow?  Fwiw, it seems Chrome does apply fallback alignment in this case,
'start' for 'space-between', 'center' for 'space-around' etc.  The spec doesn't
really define when the "cannot be distributed in this way" condition is true.
What does Flexbox do?
[1] https://drafts.csswg.org/css-align-3/#fallback-alignment

	Daniel Holbert [:dholbert]
	Comment 24 • 9 years ago

(In reply to Mats Palmgren (:mats) from comment #23)
> So then I have a question: does the
> "When the alignment subjects cannot be distributed in this way"[1] apply when
> we overflow?

Yes -- I think that just means there's negative leftover space.

> Fwiw, it seems Chrome does apply fallback alignment in this
> case, 'start' for 'space-between', 'center' for 'space-around' etc.

Right, that's expected.

> The spec
> doesn't
> really define when the "cannot be distributed in this way" condition is true.
> What does Flexbox do?
> [1] https://drafts.csswg.org/css-align-3/#fallback-alignment

I agree that terminology is a bit vague.  The flexbox spec is clearer about this -- it explicitly says e.g. for space-between: "If the leftover free-space is negative [...]  this value is identical to flex-start."

Here are some flexbox testcases that exercise this behavior for 'justify-content', for reference:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-horiz-003.xhtml
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/flexbox-justify-content-vert-003.xhtml

	Daniel Holbert [:dholbert]
	Comment 25 • 9 years ago

(Here's a link to the older [and clearer on this point] 'justify-content' property definition, in the flexbox spec:
 https://drafts.csswg.org/css-flexbox-1/#justify-content-property )

	Daniel Holbert [:dholbert]
	Comment 26 • 9 years ago

Ah, I only just noticed that css-align lets authors explicitly specify the fallback alignment -- so e.g. 'space-around' falls back to 'center', but authors can also specify e.g. 'space-around end' to get end-aligned behavior when space is constrained. Or even 'space-around end safe' to get start-aligned behavior (I think?), though that's a bit odd because the 'end' value [& any alternative placed there] has no effect.

That ability to specify fallback is a new thing since flexbox's definition of these properties -- we'll need to implement that in bug 1207698.

In any case -- since the "negative space" scenario is a bit complex and author-customizeable, we probably want to handle it separately from the positive-space scenario.  Might be best to handle it as an additional "alignment"-variable fix-up step, where we correct the 'alignment' enum to start|end|center depending on the sign of "space" and (if negative) the author's specified [ <overflow-position>? && <content-position> ] expression and the default fallback for each value.

This means your code's assumption about nonnegative remainder will probably end up being valid for the enums that use it (stretch/space-*), given that these cases w/ remainder should only end up running in a nonnegative-space scenario.

	Daniel Holbert [:dholbert]
	Comment 27 • 9 years ago

fantasai clarified on IRC that the "fallback alignment" stuff is a bit more nuanced - it's not just negative space.  It includes e.g. how to handle "space-between" when there's just 1 track.  The "space-between" value is supposed to make the first track be flush with the start edge, and the last track flush with the end edge -- but that can't be done in general if there's nonzero free space (either positive or negative).  So the fallback alignment says what to do in that case.

So e.g.:
 * "justify-content: space-between end" means if there's only a single track, snap it to the end edge.
 * "justify-content: space-between end safe" means if there's negative free-space [no matter how many tracks there are], snap the tracks to the start edge. (and "safe" is stronger than "end" in this case)

	Mats Palmgren (inactive) Assignee
	Comment 28 • 9 years ago

Yeah, I think you're right.  I'm adding the fallback path now, it looks quite easy
to integrate, and a nice side effect is that we don't need to deal with negative
space in the distribution code.

<rant>
Sigh, them CSS specs and their vague and ill-defined prose!  I really wish
they were more like WHATWG specs: a set of clearly defined states, with
pseudo-code describing the state transitions => 100% unambiguous!
</rant>

	Daniel Holbert [:dholbert]
	Comment 29 • 9 years ago

(Also, heads-up: fantasai says on IRC she's a bit concerned about the complexity of these properties right now & is wondering whether they should be simplified to be single-valued for the moment. http://log.csswg.org/irc.w3.org/css/2015-09-28/#e601144  So it's possible there will be a simplifying shakeup sometime soon.  Though maybe not, if she & Tab receive spec-review opinions that approve of the current design.)

	Mats Palmgren (inactive) Assignee
	Comment 30 • 9 years ago

I think it's fine from an implementation complexity POV.  It was quite easy to apply
the fallback value.  The problem I'm having is that the specs are so darn vague that
I have to guess every other sentence what they mean *EXACTLY*.

There is absolutely nothing in the spec that clearly defines under what set of
conditions the fallback value should be used.

	Mats Palmgren (inactive) Assignee
	Comment 31 • 9 years ago

It appears Chrome has full support for fallback values.
We have the exact same rendering for this test AFAICT.

	Mats Palmgren (inactive) Assignee
	Comment 32 • 9 years ago

Fixed the nits and added new code to apply the fallback value.

	Daniel Holbert [:dholbert]
	Comment 33 • 9 years ago

Comment on attachment 8667473 [details] [diff] [review]
part 3 - [css-grid] Implement layout for the 'justify-content' and 'align-content' CSS properties.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +1070,5 @@
>  }
>  
> +static uint16_t
> +GetAlignJustifyValue(uint16_t aAlignment, const WritingMode aWM,
> +                     const bool aIsAlign, bool* aOverflowSafe)

This function's name seems to imply that it's relevant to all [align|justify]-* properties. But really, it's only for  [align|justify]-content, right?  Could it be renamed (or at least documented) to make that clearer?

@@ +1072,5 @@
> +static uint16_t
> +GetAlignJustifyValue(uint16_t aAlignment, const WritingMode aWM,
> +                     const bool aIsAlign, bool* aOverflowSafe)
> +{
> +  *aOverflowSafe = aAlignment & NS_STYLE_ALIGN_SAFE;

When we're called via GetAlignJustifyFallbackIfAny codepath, it looks like we're checking this bit *after* we've right-shifted by NS_STYLE_ALIGN_ALL_SHIFT. Are you sure that's correct?

(If it is correct, this suggests to me that our uint16_t value must really have 2 bits for "safe" -- one in the fallback part, and one in the normal part.  Is that right? That seems a bit odd, because I think the syntax in https://drafts.csswg.org/css-align-3/#propdef-align-self only allows a single "safe" keyword (as part of <overflow-position>.  Maybe we simply set the bit in both parts when there's a fallback value, though, so the bit will always have the same value in each part? If that's the case, I think we should maybe drop aOverflowSafe from the "IfAny" method, per a note further down.)

@@ +1073,5 @@
> +GetAlignJustifyValue(uint16_t aAlignment, const WritingMode aWM,
> +                     const bool aIsAlign, bool* aOverflowSafe)
> +{
> +  *aOverflowSafe = aAlignment & NS_STYLE_ALIGN_SAFE;
> +  aAlignment &= (NS_STYLE_ALIGN_ALL_BITS & ~NS_STYLE_ALIGN_FLAG_BITS);

Can we remove NS_STYLE_ALIGN_ALL_BITS from the binary arithmetic here?

In the previous patch, we had just:
  alignment &= ~NS_STYLE_ALIGN_FLAG_BITS;
That seems simpler & clearer, because really, we're just trying to clear the flag bits here.

The new NS_STYLE_ALIGN_ALL_BITS seems like it's trying to also clear arbitrary high bits, but those arbitrary high bits shouldn't be set anyway.... If they are set, it seems better to handle them separately and/or catch them in assertions.

@@ +1098,5 @@
> +}
> +
> +static uint16_t
> +GetAlignJustifyFallbackIfAny(uint16_t aAlignment, const WritingMode aWM,
> +                             const bool aIsAlign, bool* aOverflowSafe)

As above, "AlignJustifyFallback" feels a bit generic in the function name here, since this function is really only applicable to 1/2 of the align/justify properties. (or 1/3 of them, if you consider the "*-items" properties)

Please add a bit of documentation mentioning that this is for "align-content" & "justify-content", and/or rename the function to make that a bit clearer.

@@ +1112,5 @@
> +      return NS_STYLE_ALIGN_START;
> +    case NS_STYLE_ALIGN_SPACE_AROUND:
> +    case NS_STYLE_ALIGN_SPACE_EVENLY:
> +      return NS_STYLE_ALIGN_CENTER;
> +  }

These return statements all leave |overflowSafe| uninitialized.  At first I thought this was accidental (and thought it could lead to uninitialized usage in the caller), but now I think it works out, so this is probably as-designed.

It'd be worth some documentation to make the behavior clearer, though, since it's different between this method vs. GetAlignJustifyValue (and since AlignJustifyContent calls both methods one after the other). In particular, I think these facts are worth documenting:
 - GetAlignJustifyValue always sets its aOverflowSafe outparam.
 - GetAlignJustifyFallbackIfAny *only* sets its aOverflowSafe outparam if it uses an *author-provided* fallback value. Otherwise it leaves aOverflowSafe untouched.

(As noted above, I'm unclear on what it means to detect 'safe' when we're looking at an author-provided fallback value, & if that's guaranteed to be the same as the non-fallback value. If they're guaranteed to be the same, maybe it'd be better to just remove the aOverflowSafe arg entirely from GetAlignJustifyFallbackIfAny, and have it pass a dummy-bool or null to its nested GetAlignJustifyValue call?)

@@ +2618,5 @@
> +    return;
> +  }
> +
> +  // Distribute free space to/between tracks and set their position.
> +  MOZ_ASSERT(space > 0, "should've handled that on the fallback path above");

This assertion-message doesn't quite explain why we expect space to be positive here, since the fallback path doesn't modify 'space' or directly bypass this code.

I think something like this might be clearer about why we expect the assertion to hold:
  "should've taken fallback path & !distribute early-return above"

@@ +2627,5 @@
> +      nscoord spacePerTrack;
> +      roundingError = NSCoordDivRem(space, numAutoTracks, &spacePerTrack);
> +      for (TrackSize& sz : mSizes) {
> +#ifdef DEBUG
> +        space += sz.mBase; // for the assert below: space + all mBase == pos

This seems like a possible footgun -- we shouldn't make a DEBUG-guarded tweak to a variable which is used in opt builds.  (In this case, opt builds are done with this variable from this point on - but it's conceivable that this might change in the future, and then this DEBUG section would cause us some trouble.)

So: for better hygiene, let's just use a DebugOnly variable here (initialized to be equal to 'space') instead of messing with 'space' directly.  Possible name: spacePlusAllUnstretchedBases, since I think that's what this basically ends up representing...

(As a bonus, I'm pretty sure you can do away with #ifdef DEBUG guards, too, since operator= and operator+= calls for DebugOnly<> are supposed be optimized away in opt builds.)

	Daniel Holbert [:dholbert]
	Comment 34 • 9 years ago

(This is basically r+ with comment 33 addressed, but I'm not quite comfortable granting preemptive r+, since I'm unsure about how the aOverflowSafe bit is represented & handled, as noted above.)

	Mats Palmgren (inactive) Assignee
	Comment 35 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #33)
> > +GetAlignJustifyValue(uint16_t aAlignment, const WritingMode aWM,
> > +                     const bool aIsAlign, bool* aOverflowSafe)
> 
> This function's name seems to imply that it's relevant to all
> [align|justify]-* properties. But really, it's only for 
> [align|justify]-content, right?  Could it be renamed (or at least
> documented) to make that clearer?

It could be used for any (future) property that has an alignment
value + fallback, but fair enough, I'll rename it / document that
it's only used for align/justify-content currently.

> When we're called via GetAlignJustifyFallbackIfAny codepath, it looks like
> we're checking this bit *after* we've right-shifted by
> NS_STYLE_ALIGN_ALL_SHIFT. Are you sure that's correct?

Yes.

> (If it is correct, this suggests to me that our uint16_t value must really
> have 2 bits for "safe" -- one in the fallback part, and one in the normal
> part.  Is that right?

Yes.  The mAlignContent member contains the regular value in the low
byte and the fallback value in the high byte (or zero, if none).
So a value without fallback such as "align-content:left safe" has
the SAFE bit in the lower byte and "align-content:stretch left safe"
has the SAFE bit in the high byte.  You can think of the two bytes
as two independent (alternative) values.

I chose this representation so that the two bytes can be treated as
a valid value on their own.  That is, by simply shifting down the
fallback you can treat the result as if it were the regular value
in the first place.  This way we can the same code to handle a value
regardless if it's originally a regular or fallback value.
(And the representation within a byte is exactly the same as for the
other 1-byte properties of course, so we can also share code with
those - which we do in the style system in several places.)

> > +  aAlignment &= (NS_STYLE_ALIGN_ALL_BITS & ~NS_STYLE_ALIGN_FLAG_BITS);
> 
> Can we remove NS_STYLE_ALIGN_ALL_BITS from the binary arithmetic here?

No, we need to zero out the fallback part before we can start comparing
it to the NS_STYLE_ALIGN_* constants.

> In the previous patch, we had just:
>   alignment &= ~NS_STYLE_ALIGN_FLAG_BITS;

Yes, I realized that was just a bug. :-)
(sorry, I should've mentioned that)

> The new NS_STYLE_ALIGN_ALL_BITS seems like it's trying to also clear
> arbitrary high bits, but those arbitrary high bits shouldn't be set
> anyway....

Yes.  Why shouldn't they be set?

> > +GetAlignJustifyFallbackIfAny(uint16_t aAlignment, const WritingMode aWM,
> Please add a bit of documentation mentioning that this is for
> "align-content" & "justify-content", and/or rename the function to make that
> a bit clearer.

OK, sure.

> > +    case NS_STYLE_ALIGN_SPACE_EVENLY:
> > +      return NS_STYLE_ALIGN_CENTER;
> 
> These return statements all leave |overflowSafe| uninitialized.  At first I
> thought this was accidental (and thought it could lead to uninitialized
> usage in the caller), but now I think it works out, so this is probably
> as-designed.

Yes, it's intentionally not modified when there is no fallback, so that
for example "left safe" goes through without clobbering |overflowSafe|.

> I think these facts are worth documenting:
>  - GetAlignJustifyValue always sets its aOverflowSafe outparam.
>  - GetAlignJustifyFallbackIfAny *only* sets its aOverflowSafe outparam if it
> uses an *author-provided* fallback value. Otherwise it leaves aOverflowSafe
> untouched.

OK.

> > +  // Distribute free space to/between tracks and set their position.
> > +  MOZ_ASSERT(space > 0, "should've handled that on the fallback path above");
> 
> I think something like this might be clearer about why we expect the
> assertion to hold:
>   "should've taken fallback path & !distribute early-return above"

OK, sure.

> > +#ifdef DEBUG
> > +        space += sz.mBase; // for the assert below: space + all mBase == pos
> 
> So: for better hygiene, let's just use a DebugOnly variable here
> (initialized to be equal to 'space') instead of messing with 'space'
> directly.  Possible name: spacePlusAllUnstretchedBases, since I think that's
> what this basically ends up representing...

Hmm, I'll consider it.

	Daniel Holbert [:dholbert]
	Comment 36 • 9 years ago

(In reply to Mats Palmgren (:mats) from comment #35)
> The mAlignContent member contains the regular value in the low
> byte and the fallback value in the high byte (or zero, if none).
> So a value without fallback such as "align-content:left safe" has
> the SAFE bit in the lower byte and "align-content:stretch left safe"
> has the SAFE bit in the high byte.  You can think of the two bytes
> as two independent (alternative) values.

OK.  So, is it true that, whenever there's an author-provided fallback value, the SAFE bit will *not* be set in the main value? (since SAFE is inherently tied to a fallback scenario, due to lack of space)

If that sounds right, then I think this makes sense & I'm understanding correctly. (and I agree that your design w/ the fallback being its own valid byte makes sense)

(My confusion before was around what it would mean if both bits were set, or if the "main" bit were set and the "fallback" bit were not set. But I think neither of those things can happen, in practice.)

> > Can we remove NS_STYLE_ALIGN_ALL_BITS from the binary arithmetic here?
> 
> No, we need to zero out the fallback part before we can start comparing
> it to the NS_STYLE_ALIGN_* constants.

Ah, right. I hadn't gotten to the fallback bits yet when I was looking at this.  And at the point where this happens, we're working with something called "aAlignment" -- not "alignmentAndFallback" -- so it's non-obvious that we've got a compound value.

I have a few ideas for how we could make this clearer:

 (1) (preferred) We could do this bit-clearing up one level, at the sole callsite where we actually expect to be passing in a compound value. Specifically, in AlignJustifyContent, I'd suggest changing this...
  auto alignment = ::GetAlignJustifyValue(valueAndFallback, aWM, isAlign,
                                          &overflowSafe);
...to this:
  auto alignment =  // (Mask away the fallback value, if present:)
    ::GetAlignJustifyValue(valueAndFallback & NS_STYLE_ALIGN_ALL_BITS,
                           aWM, isAlign, &overflowSafe);

Or:
 (2) We could name GetAlignJustifyValue's arg to aAlignmentAndFallback to make it clearer that it is (or can be) compound.

And/or:
 (3) We should add a comment next to the clearing to explain that the value might have a fallback-value encoded in the high bits and we're clearing that out.

	Daniel Holbert [:dholbert]
	Comment 37 • 9 years ago

Comment on attachment 8667473 [details] [diff] [review]
part 3 - [css-grid] Implement layout for the 'justify-content' and 'align-content' CSS properties.

r+ since the overflow-bit-handling makes sense to me now (I think), with the ALIGN_ALL_BITS masking clarified somehow, per my previous comment. (and with comment 33 addressed, but you're already working on that)

	Mats Palmgren (inactive) Assignee
	Comment 38 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #36)
> OK.  So, is it true that, whenever there's an author-provided fallback
> value, the SAFE bit will *not* be set in the main value?

Yes, the syntax allows one safe/true value at most:
Either as regular value, "left safe", or with a fallback,
"stretch left safe".

The syntax doesn't allow it with only a <content-distribution>,
"stretch safe", nor in both parts, "stretch safe left safe"
https://drafts.csswg.org/css-align-3/#propdef-align-content

	Mats Palmgren (inactive) Assignee
	Comment 39 • 9 years ago

Minor update to handle 'auto' values for justify/align-content.
Interdiff with the last patch:

> @@ -2425,16 +2430,24 @@ MainAxisPositionTracker::
73a74,78
> +  // 'auto' behaves as 'stretch' which behaves as 'flex-start' in the main axis
> +  if (mJustifyContent == NS_STYLE_JUSTIFY_AUTO) {
> +    mJustifyContent = NS_STYLE_JUSTIFY_FLEX_START;
> +  }
> +

> @@ -2578,16 +2604,24 @@ CrossAxisPositionTracker::
139a145,149
> +  // 'auto' behaves as 'stretch'
> +  if (mAlignContent == NS_STYLE_ALIGN_AUTO) {
> +    mAlignContent = NS_STYLE_ALIGN_STRETCH;
> +  }
> +

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1148a255e669

	Daniel Holbert [:dholbert]
	Comment 40 • 9 years ago

Comment on attachment 8678354 [details] [diff] [review]
part 2 - [css-flexbox] Shim implemention for the new align/justify property values in flexbox layout (just to avoid fatal assertions).

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

r=me; just 2 instances of a comment nit, noted below:

::: layout/generic/nsFlexContainerFrame.cpp
@@ +2440,5 @@
> +    mJustifyContent = NS_STYLE_JUSTIFY_FLEX_START;
> +  }
> +
> +  // XXX strip of the <overflow-position> bit until we implement that
> +  mJustifyContent &= ~NS_STYLE_JUSTIFY_FLAG_BITS;

s/of/off/

@@ +2614,5 @@
> +    mAlignContent = NS_STYLE_ALIGN_STRETCH;
> +  }
> +
> +  // XXX strip of the <overflow-position> bit until we implement that
> +  mAlignContent &= ~NS_STYLE_ALIGN_FLAG_BITS;

s/of/off/

	Mats Palmgren (inactive) Assignee
	Comment 41 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/56aebef6afdf

	Mats Palmgren (inactive) Assignee
	Comment 42 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb626ed4c37
https://hg.mozilla.org/integration/mozilla-inbound/rev/beea1a34f2a2

	Carsten Book [:Tomcat]
	Comment 43 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/56aebef6afdf
https://hg.mozilla.org/mozilla-central/rev/3bb626ed4c37
https://hg.mozilla.org/mozilla-central/rev/beea1a34f2a2

	Carsten Book [:Tomcat]
	Comment 44 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/56aebef6afdf
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3bb626ed4c37
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/beea1a34f2a2

	Carsten Book [:Tomcat]
	Comment 45 • 9 years ago

(In reply to Carsten Book [:Tomcat] from comment #44)
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/56aebef6afdf
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3bb626ed4c37
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/beea1a34f2a2

backed this out on request from mats from b2g v2.5 in http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0869ace8c965