1221525 - [css-grid] Implement Grid layout for align|justify-self:baseline|last-baseline (aka "Baseline Self-Alignment")

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

https://drafts.csswg.org/css-align-3/#propdef-justify-self
https://drafts.csswg.org/css-align-3/#propdef-align-self

(Feel free to dupe it if you plan to handle this in bug 1151204)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Break out the grid item measuring reflow code ...

This separates the measuring reflow code out from ContentContribution to
its own function.  I'm also changing the child frame param to take
a GridItemInfo in a few places, in preparation for later patches
that needs it.  No functional changes in this patch.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Add GridItemInfo::mState member ...

Change the mIsFlexing bools into a mState bit flag instead,
and add a few new bits for keeping baseline state per item.
I'm also adding a mBaselineOffset member for the baseline
offset that will be used in later patches, and a DEBUG-only
Dump method.  (No functional changes in terms of grid layout
in this patch.)

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Caclulate the baseline offset values and baseline subtree alignment

This patch determines if an item should participate in baseline
alignment and initializes its baseline state flags accordingly.
If so, it also calculates the mBaselineOffset for the item so that it
aligns with other items in its baseline group (baseline/last-baseline).
It also calculates the size subtree for each group in each track.
(the same offset will be used for both *-self and *-content alignment)

There is one caveat in this patch that I'm intentionally punting on.
I'm just using nsLayoutUtils::Get[First|Last]LineBaseline so far.
I realize I should probably call GetLogicalBaseline if it returns
false, but the problem is that method assumes you want the block-
axis baseline IIUC.  I'd like to investigate this a bit more and
possibly do some refactoring of the existing baseline methods
before fixing this issue, and before that I want to add Grid
container baseline support in bug 1151204 first (where we need
to return a meaningful baseline in both axis).

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Implement Grid layout for align|justify-self:baseline|last-baseline

This uses the mBaselineOffset to do two things - it adds it to the item's
size contribution (per spec).  This is to make intrinsically sized tracks
large enough to contain the baseline-aligned subtree as a whole.
It also adds code to AlignJustifySelf to do the alignment (for *-self
baseline alignment, bug 1256429 will address *-content alignment).

AlignBaselineSubtree adjusts the mBaselineOffset values so that
the subtree as a whole aligns in the track (after we know the track's
size).  I settled on doing something simple for now 'baseline'
aligns to the track start, 'last-baseline' to the end.
This is what the CSS Align spec currently says (vaguely), but I think
we could improve on that in various ways.
https://lists.w3.org/Archives/Public/www-style/2016May/0141.html

So, the NS_STYLE_ALIGN_CENTER used here isn't actually used
at the moment.  (I'll remove it in the future depending on what
we decide after a bit more discussion on www-style.)

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Reftests for align|justify-self:baseline|last-baseline

Comment 6

[Mats Palmgren (inactive)]

s/the NS_STYLE_ALIGN_CENTER used here/the NS_STYLE_ALIGN_CENTER *code* here/ in comment 4.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8755924 [details] [diff] [review]
part 1 - [css-grid] Break out the grid item measuring reflow code ...

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

r=me, with nits addressed:

::: layout/generic/nsGridContainerFrame.cpp
@@ +3182,5 @@
>   * Return the [min|max]-content contribution of aChild to its parent (i.e.
>   * the child's margin-box) in aAxis.
>   */
>  static nscoord
> +MeasuringReflow(nsIFrame*                aChild,

Nit: this patch ends up producing two functions which have *identical* documentation[1].  I think this might be a mistake? Or even if it's not a mistake (i.e. even if the identical documentation is strictly correct in each case), might be good to add a sentence to each piece of documentation to clarify each function's purpose (& distinguish them from each other).

[1] The new function, MeasuringReflow(), ends up with an identical documentation comment to its caller, ContentContribution().

@@ +3187,5 @@
> +                const nsHTMLReflowState* aReflowState,
> +                nsRenderingContext*      aRC,
> +                const LogicalSize&       aAvailableSize)
> +{
> +  WritingMode wm = aChild->GetWritingMode();

Nit: in this new refactored-out function, this "wm" variable isn't actually used until after we've created a reflow state for the child (childRS).

And at that point, it's better to just use the cached WritingMode on childRS. (since nsIFrame::GetWritingMode is a virtual function call, and involves querying data from its StyleContext).

So: either in this patch or in a followup, please move this |WritingMode wm| declaration further down, and assign it to childRS.GetWritingMode() (which is cheap).

@@ +3226,5 @@
> + * Return the [min|max]-content contribution of aChild to its parent (i.e.
> + * the child's margin-box) in aAxis.
> + */
> +static nscoord
> +ContentContribution(const nsGridContainerFrame::GridItemInfo& aGridItem,

Could you shorten the fully-qualified type in this line (and other similar lines in this patch) by adding a typedef at the top of this .cpp file?

This should do...
typedef nsGridContainerFrame::GridItemInfo GridItemInfo;
...inserted just above the existing line for "nsGridContainerFrame::TrackSize":
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp?rev=5e11a2a81e37#32

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8755928 [details] [diff] [review]
part 2 - [css-grid] Add GridItemInfo::mState member ...

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +616,5 @@
> +    eAnyBaseline = eFirstBaseline | eLastBaseline,
> +    eSelfBaseline =         0x8, // is it align-self:[last-]baseline alignment?
> +    // Ditto align-content. Mutually exclusive w. eSelfBaseline.
> +    eContentBaseline =     0x10,
> +    eAllBaselineBits = eAnyBaseline | eSelfBaseline | eContentBaseline,

A number of documentation/naming concerns about these bits:

(1) Are eFirstBaseline & eLastBaseline mutually exclusive? If so, probably worth stating that. 

(2) If they *are* mutually exclusive, it might (optionally) be cleaner to structure your flags to collapse away the mutually-exclusive stuff, like so:
 - One master "eIsBaselineAligned" bit, which determines whether the other baseline-alignment bits are meaningful at all.
 - One bit for "first-baseline vs. last baseline" (e.g. "eIsFirstBaseline")
 - One bit for "is this baseline alignment for align-self vs. align-content" (e.g. "eIsBaselineForAlignSelf")

This representation would have the benefit of being *unable to express* a lot of nonsensical scenarios that this patch's current representation *can* express.  The "mutually exlusive" requirement that you're documenting here would just naturally fall out of the representation, instead of needing to be documented/enforced/sanity-checked.

(Note: if you act on this suggestion #2, then my next two comments #3 and #4 might no longer apply, depending on how the enum values change.)

(3) The names "eAnyBaseline" and "eAllBaselineBits" are too easy to confuse, IMO...  One of them might want a rename, or at least we should add a line of documentation above "eAllBaselineBits" clarifying its purpose & how it semantically differs from the similarly-named "eAnyBaseline".

(4) It's not immediately clear how eFirstBaseline/eLastBaseline relate to eSelfBaseline/eContentBaseline.  I'm guessing maybe the latter bits are *only* expected to be set when one of the earlier bits is also set?  (i.e. the latter ones tell you which property is doing baseline alignment, IFF one of the earlier bits is set?)  Please add a comment to clarify this.

(5) Here, you only mention "align-self" and "align-content", but further down (in documentation for GetSelfBaseline) you mention "[align|justify]-self".  So: does this enum track information about "justify-self" as well, potentially? If so, the enum values' documentation probably needs to mention the "justify" properties, instead of being align-[self|content] specific.  Or alternately, maybe this is just a typo in GetSelfBaseline?

@@ +627,5 @@
>    {
> +    mState[0] = StateBits(0);
> +    mState[1] = StateBits(0);
> +    mBaselineOffset[0] = nscoord(0);
> +    mBaselineOffset[1] = nscoord(0);

Elsewhere in this patch, you index into these arrays using type "LogicalAxis", rather than a raw integer value.

Let's do that here as well, for consistency, as well as for clarity about what these entries represent. So:
 s/[0]/[eLogicalAxisBlock]/
 s/[1]/[eLogicalAxisInline]/

@@ +632,5 @@
>    }
>  
> +  /**
> +   * Return the [align|justify]-self baseline alignment in the given axis, or
> +   * a fallback alignment if the item doesn't have a baseline in that axis.

This sentence is a bit confusing, in that it changes what it's talking about midstream ("return...baseline alignment" is actually about the outparam; "or a fallback alignment" is about the *actual* return value).

Please reword to make the outparam usage clearer.

@@ +659,5 @@
>  
>    nsIFrame* const mFrame;
>    GridArea mArea;
> +  // Offset from the margin edge to the baseline (LogicalAxis index), it's from
> +  // the start edge when eFirstBaseline is set, end edge otherwise.

Nit: make "it's from" a new sentence (i.e. replace the comma before it with a period -- or at least a semicolon)

@@ +660,5 @@
>    nsIFrame* const mFrame;
>    GridArea mArea;
> +  // Offset from the margin edge to the baseline (LogicalAxis index), it's from
> +  // the start edge when eFirstBaseline is set, end edge otherwise.
> +  mutable nscoord mBaselineOffset[2];

Why 'mutable'?  (consider mentioning in comment above this variable)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8755933 [details] [diff] [review]
part 3 - [css-grid] Caclulate the baseline offset values and baseline subtree alignment

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +144,5 @@
>    nscoord mLimit;
>    nscoord mPosition;  // zero until we apply 'align/justify-content'
> +  // mBaselineSubtreeSize is the size of a baseline-aligned subtree within
> +  // this track.  One subtree per baseline-sharing group (per track).
> +  nscoord mBaselineSubtreeSize[2];

You have this magic value "2" used for sizing two different arrays in this patch.  Magic numbers (even 2) seem worth avoiding... Might be better to use an explicit (self-documenting) enum value, "BaselineSharingGroup::eGroup_COUNT"?  (in the style of "eCSSProperty_COUNT")

Not a huge deal though.

@@ +210,5 @@
>        }
>    }
> +
> +  mBaselineSubtreeSize[0] = nscoord(0);
> +  mBaselineSubtreeSize[1] = nscoord(0);

As in comment 8, I'd prefer that we consistently use the enum as an index, for arrays that use enum-valued indexes.  Mixing enum indices with numeric indices seems bug-prone and magical.

So, this should probably be changed to:
  mBaselineSubtreeSize[BaselineSharingGroup::eFirst] = nscoord(0);
  mBaselineSubtreeSize[BaselineSharingGroup::eLast] = nscoord(0);

(A bit more verbose, but also clearer about what's going on.)

@@ +1073,5 @@
> +  explicit Tracks(LogicalAxis aAxis)
> +    : mAxis(aAxis)
> +  {
> +    mBaselineSubtreeAlign[0] = NS_STYLE_ALIGN_AUTO;
> +    mBaselineSubtreeAlign[1] = NS_STYLE_ALIGN_AUTO;

Same here:
  mBaselineSubtreeAlign[BaselineSharingGroup::eFirst] = NS_STYLE_ALIGN_AUTO;
  mBaselineSubtreeAlign[BaselineSharingGroup::eLast] = NS_STYLE_ALIGN_AUTO;

(Still fits within the 80-character threshold, I believe!) :)

@@ +1554,5 @@
>    nscoord mContentBoxSize;
>    nscoord mGridGap;
>    LogicalAxis mAxis;
> +  // Used for aligning a baseline-aligned subtree of items.  The only possible
> +  // values are 'start', 'end', 'center', or 'auto'.  'auto' means there are

"The only possible values are 'start', [etc]" here is a bit confusing, type-wise, since 'start' etc. are keywords/strings, and this is documentation for a uint8_t array.

Maybe express this as NS_STYLE_ALIGN_{START,END,CENTER,AUTO}, to make it a bit clearer what value-space you're talking about here?

Or, maybe add a parenthetical saying e.g. "(represented as NS_STYLE_ALIGN_* values)" after your list of keywords here.

@@ +3615,5 @@
> +    auto state = ItemState(0);
> +    auto childWM = child->GetWritingMode();
> +    const bool isOrthogonal = wm.IsOrthogonalTo(childWM);
> +    const bool isInlineAxis = mAxis == eLogicalAxisInline;
> +    const bool parallellInline = isInlineAxis == isOrthogonal;

Several nits about these bools:

(1) typo: "parallel" only has a single "l" at the end.

  "parallellInline" -->
  "parallelInline"
  ...but, it might want a more substantial rename anyway, because:

(2) "parallellInline" is ambiguously named. I initially interpreted it as "the child & parent's inline axes are parallel", but that's clearly wrong, since it's assigned to something more complex than that; I had to stare at it a while to figure out what it actually means. (See (3).)  A comment and/or a rename might help. Maybe "itemInlineAxisParallelToTrack"?  (A bit verbose, but you only actually use this variable once, so verbosity isn't too costly.)

(3) Consider adding a comment to explain the logic behind these bools here -- they're hard to follow (for me at least), in part due to parallellInline being hard to understand (noted above), and in part because it's easy to forget what "mAxis == eLogicalAxisInline" (aka "isInlineAxis") actually means here.  (I initially read it as meaning "Does this Tracks object lay out its grid items in the inline axis", but in fact that's exactly the opposite of what it means.)  Might be better to just name this "isGridColumns"...

Sample explanatory comment:
  // Note: isInlineAxis indicates whether we're working with grid columns (rather
  // than rows); columns are stacked in the inline axis. So: if we're dealing
  // with grid columns, and the child's writing mode is orthogonal to its
  // parent's, then the child's inline axis is parallel to the flow of a
  // grid-track. (Similarly, if we're dealing with rows and the child's writing
  // mode is *not* orthogonal to the parent, then again the child's inline axis
  // is parallel to the flow of the track.)

@@ +3620,5 @@
> +    if (parallellInline) {
> +      // [align|justify]-self:[last-]baseline.
> +      auto alignSelf = isOrthogonal ?
> +        child->StylePosition()->ComputedJustifySelf(containerSC) :
> +        child->StylePosition()->ComputedAlignSelf(containerSC);

"alignSelf" is confusingly-named here, since it is *not necessarily align-self* -- it could just as easily be justify-self.

Maybe s/alignSelf/selfAlignment/ would help clarify that? (and reduce the immediate assumption that "this is align-self")

@@ +3651,5 @@
> +                          alignSelf == selfAlignEdge;
> +        if (!validCombo) {
> +          auto alignSelfForAxis = isInlineAxis ?
> +            child->StylePosition()->ComputedJustifySelf(containerSC) :
> +            child->StylePosition()->ComputedAlignSelf(containerSC);

I don't understand how this variable differs from "alignSelf" up above.

It seems to me like they must strictly be the same, because:
 (1) They only differ based on the bool that they check. (For alignSelf, you check "isOrthogonal"; here, you check "isInlineAxis")
 (2) We only reach this code if those bools are equal (via a "parallellInline" check).
 (3) Therefore, the alignSelf expression and alignSelfForAxis expression seems like they *must* evaluate to the same thing...

So I don't see why we've got two separate variables.

@@ +3657,5 @@
> +          // the same as the child's start side, in the child's parallel axis.
> +          // XXX SameSide is way to complicated to use! - add a WritingMode
> +          // XXX method instead that does this in a better way.
> +          auto side = isInlineAxis ? eLogicalSideBStart : eLogicalSideIStart;
> +          auto childSide = isInlineAxis == isOrthogonal ? eLogicalSideIStart

This "isInlineAxis == isOrthogonal" check seems redundant -- this code is nested inside of this check from up above:
   const bool parallellInline = isInlineAxis == isOrthogonal;
   if (parallellInline) {

So, there shouldn't be any need to re-check "isInlineAxis == isOrthogonal" here -- you should be able to assume that that comparison still passes.

@@ +3662,5 @@
> +                                                        : eLogicalSideBStart;
> +          bool sameSide = SameSide(wm, side, childWM, childSide);
> +          switch (alignSelfForAxis) {
> +            case NS_STYLE_ALIGN_START:
> +            case NS_STYLE_ALIGN_FLEX_START:

I'm not 100% sure, but I think we might need to conditionally check for 'left' and 'right' somewhere in here, too, since the spec says each of these values "behaves as start" in some conditions....  (So they might have to transitively behave as self-{start|end} here, in the same way that 'start' does.)
https://drafts.csswg.org/css-align-3/#valdef-self-position-left

@@ +3664,5 @@
> +          switch (alignSelfForAxis) {
> +            case NS_STYLE_ALIGN_START:
> +            case NS_STYLE_ALIGN_FLEX_START:
> +              validCombo = sameSide ==
> +                          (alignContent == NS_STYLE_ALIGN_BASELINE);

You need one more space before the open-paren here. (open-paren should be directly below the "s")

@@ +3676,5 @@
> +        }
> +        if (validCombo) {
> +          const GridArea& area = gridItem.mArea;
> +          if (alignContent == NS_STYLE_ALIGN_BASELINE) {
> +            state |= ItemState::eFirstBaseline | ItemState::eContentBaseline;

So, you have documentation saying that ItemState::eSelfBaseline and eContentBaseline are mutually exclusive, but I don't immediately see how that's enforced here -- there's no strict "if/else" relationship between the blocks of code that sets them.

If you're sure they're mutually exclusive (and hence |state| must be blank at this point), please assert about it here,  maybe up a few lines, inside the validCombo check:
  if (validCombo) {
    MOZ_ASSERT(!state, "[hint at why we're so sure |state| can't be set]");

@@ +3690,5 @@
> +    if (state & ItemState::eAnyBaseline) {
> +      // XXX available size issue
> +      LogicalSize avail(childWM, INFINITE_ISIZE_COORD, NS_UNCONSTRAINEDSIZE);
> +      auto* rc = &aState.mRenderingContext;
> +      ::MeasuringReflow(child, aState.mReflowState, rc, avail);

Do we really need a dedicated measuring reflow, just to establish the baseline? (Can we share another measuring reflow somehow? or the main reflow?)

If it's really not possible to share another reflow, consider adding a comment here to briefly explain why.

@@ +3724,5 @@
> +    gridItem.mState[mAxis] = state;
> +  }
> +
> +  if (firstBaselineItems.IsEmpty() && lastBaselineItems.IsEmpty()) {
> +    return;

Nit: this "early return" at the very bottom of this method seems a bit out of place, given that it only skips 4 lines of code.  Might be cleaner to just flip the logic of the "if" statement, and get rid of the explicit "return".

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8755938 [details] [diff] [review]
part 4 - [css-grid] Implement Grid layout for align|justify-self:baseline|last-baseline

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

r=me on part 4, with concerns addressed:

::: layout/generic/nsGridContainerFrame.cpp
@@ +1691,5 @@
> +          auto item = mGridItems.AppendElement(GridItemInfo(child, itemInfo.mArea));
> +          item->mState[0] |= itemInfo.mState[0] & ItemState::eAllBaselineBits;
> +          item->mState[1] |= itemInfo.mState[1] & ItemState::eAllBaselineBits;
> +          item->mBaselineOffset[0] = itemInfo.mBaselineOffset[0];
> +          item->mBaselineOffset[1] = itemInfo.mBaselineOffset[1];

Two concerns on this section:

(1) Why are we copying these specific pieces of state from the first-in-flow? A comment would be helpful here.  (In particular, it's non-obvious why the first-in-flow's mBaselineOffset is something that we'd want to copy to its continuations, since the continuations could have differently-sized content, and will have no border/padding-top, and hence would have different baseline offsets. But maybe we're treating the whole continuation-chain as having a single first/last-baseline shared among all of them? If so, this assignment makes some sense, but that needs documenting as well.)

(2) Similar to in earlier comments: the [0]/[1] indexes seem magical/hand-wavy, in the four lines above... Technically this should really be using enum constants as the index for this array, to make it clearer what these represent.  (That would make this a bit more verbose; but if that verbosity ends up being prohibitive, maybe that means this wants a refactoring/helper-method or a restructure of some sort?)  Alternately, maybe you could add a comment here, saying that you're using 0 and 1 as shorthand for eLogicalAxisBlock/eLogicalAxisInline, which are the enum values that this array uses as its indices.

@@ +2322,5 @@
>    nscoord offset = 0; // NOTE: this is the resulting frame offset (border box).
>    switch (aAlignment) {
>      case NS_STYLE_ALIGN_BASELINE:
> +    case NS_STYLE_ALIGN_LAST_BASELINE: {
> +      nscoord baselineAdjust = aBaselineAdjust;

This local variable doesn't add any value here; you don't modify it after this declaration, so it's effectively a 1-character-shorter alias for aBaselineAdjust.

So, consider dropping it & just using aBaselineAdjust directly.

(I think means you can drop the curly-braces that you're adding around this "case" body, too -- it looks like they only exist to provide a scope for this new variable.)

@@ +3412,5 @@
>      size += offsets.hMargin;
>      size = nsLayoutUtils::AddPercents(aConstraint, size, offsets.hPctMargin);
>    }
> +  MOZ_ASSERT(aGridItem.mBaselineOffset[aAxis] >= 0,
> +             "baseline offset should be positive at this point");

s/positive/non-negative/, to accurately describe the assertion condition.  (Or alternately, if you really mean "positive", update the assertion condition to disallow 0.  I suspect "non-negative" is what you mean to say, though.)

@@ +3461,5 @@
>    // transferred size" for min-width:auto if overflow == visible (as min-width:0
>    // otherwise), or NS_UNCONSTRAINEDSIZE for other min-width intrinsic values
>    // (which results in always taking the "content size" part below).
> +  MOZ_ASSERT(aGridItem.mBaselineOffset[aAxis] >= 0,
> +             "baseline offset should be positive at this point");

As above, the message doesn't match the condition here. Please update either the message (to "non-negative") or the condition (to "> 0") as appropriate.

@@ +3851,5 @@
> +  } else {
> +    baselineTrack = (mAxis == eLogicalAxisBlock ? area.mRows.mEnd
> +                                                : area.mCols.mEnd) - 1;
> +  }
> +  const auto& sz = mSizes[baselineTrack];

No compelling need to use "auto" here.  Just use the actual type, for clarity, please:
  s/auto&/TrackSize&/

(It's not too much more verbose, and it makes it easier to figure out what e.g. "sz.mBase" etc. represents, further down.)

@@ +3859,5 @@
> +  const auto subtreeAlign = mBaselineSubtreeAlign[baselineGroup];
> +  switch (subtreeAlign) {
> +    case NS_STYLE_ALIGN_START:
> +      if (state & ItemState::eLastBaseline) {
> +        aGridItem.mBaselineOffset[mAxis] += delta;

This is the only place in this function where you explicitly check the eLastBaseline bit.  I wonder if it'd be better to remove this and check !isFirstBaseline instead, since that's the switch you use elsewhere in this function. (e.g. for the "if (isFirstBaseline)...else" at the top, "else" is implicitly "!isFirstBaseline" and that's where you handle "last-baseline").

It shouldn't matter from a functional perspective; just from a consistency & checking-fewer-potentially-independent-pieces-of-data perspective.

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Add GridItemInfo::mState member ...

part 1: nits fixed

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Comment on attachment 8755928 [details] [diff] [review]
> part 2 - [css-grid] Add GridItemInfo::mState member ...
> (1) Are eFirstBaseline & eLastBaseline mutually exclusive? If so, probably
> worth stating that. 

Yes, fixed.

> (2) If they *are* mutually exclusive, it might (optionally) be cleaner to
> structure your flags to collapse away the mutually-exclusive stuff, like so:
>  - One master "eIsBaselineAligned" bit, which determines whether the other
> baseline-alignment bits are meaningful at all.

I think I prefer the current design since it makes it easier to understand
the code using these bits.  We could add accessors to hide the bit tests
you're proposing but that seems overkill.

> (3) The names "eAnyBaseline" and "eAllBaselineBits" are too easy to confuse,
> IMO...  One of them might want a rename

OK, I renamed the former eIsBaselineAligned.

> (4) It's not immediately clear how eFirstBaseline/eLastBaseline relate to
> eSelfBaseline/eContentBaseline.  I'm guessing maybe the latter bits are
> *only* expected to be set when one of the earlier bits is also set?

Yes, I added a comment.

> (5) Here, you only mention "align-self" and "align-content", but further
> down (in documentation for GetSelfBaseline) you mention
> "[align|justify]-self". 

Yes, the item has one mState per axis.  I updated the comment to say
"*-self" and "*-content" ...

>  s/[0]/[eLogicalAxisBlock]/
>  s/[1]/[eLogicalAxisInline]/

Fixed.

> Please reword to make the outparam usage clearer.

Fixed.

> Nit: make "it's from" a new sentence

Fixed.

> > +  mutable nscoord mBaselineOffset[2];
> 
> Why 'mutable'?  (consider mentioning in comment above this variable)

There's a bunch of methods and data structures that currently use
"const GridItemInfo&".  Since the baseline offsets needs to be updated
fairly late, the choice was to make all of those "GridItemInfo&" or
make just the mBaselineOffset field mutable.  I chose the latter since
at least then it's clear that no other fields are updated in those
methods.

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Caclulate the baseline offset values and baseline subtree alignment

(In reply to Daniel Holbert [:dholbert] from comment #9)
> > +  mBaselineSubtreeSize[0] = nscoord(0);
> > +  mBaselineSubtreeSize[1] = nscoord(0);
> 
> As in comment 8, I'd prefer that we consistently use the enum as an index,

Fixed, although the code already depends on the indicies
being exactly 0 and 1 so it's not a big deal IMO.

> Same here:
>   mBaselineSubtreeAlign[BaselineSharingGroup::eFirst] = NS_STYLE_ALIGN_AUTO;
>   mBaselineSubtreeAlign[BaselineSharingGroup::eLast] = NS_STYLE_ALIGN_AUTO;

Fixed.

> > +  // Used for aligning a baseline-aligned subtree of items.  The only possible
> > +  // values are 'start', 'end', 'center', or 'auto'.  'auto' means there are
> Maybe express this as NS_STYLE_ALIGN_{START,END,CENTER,AUTO}, to make it a
> bit clearer what value-space you're talking about here?

Fixed.

> (2) "parallellInline" is ambiguously named. I initially interpreted it as
> "the child & parent's inline axes are parallel", but that's clearly wrong,
> since it's assigned to something more complex than that; I had to stare at
> it a while to figure out what it actually means. (See (3).)  A comment
> and/or a rename might help. Maybe "itemInlineAxisParallelToTrack"?  (A bit
> verbose, but you only actually use this variable once, so verbosity isn't
> too costly.)

Yeah, this test is actually a little bit bogus since it makes
the assumption that an item can only have a baseline in its inline
axis.  I'm punting on that for now though (since it's true until
we fix bug 1151204).  So, itemInlineAxisParallelToTrack would also
be wrong eventually.  I renamed it itemHasBaselineParallelToTrack
which I think is what we're looking for here.

> (3) Consider adding a comment to explain the logic behind these bools here
> -- they're hard to follow (for me at least), in part due to parallellInline
> being hard to understand (noted above), and in part because it's easy to
> forget what "mAxis == eLogicalAxisInline" (aka "isInlineAxis") actually
> means here.

I think I prefer isInlineAxis here, since it really is the grid container's
inline axis (i.e. columns) we're dealing with.  It makes it's easier to see
that the code that follows later is correct.  E.g. 
"isInlineAxis ? child->ISize(wm) : child->BSize(wm)" where "wm" is our
writing mode.

> > +      // [align|justify]-self:[last-]baseline.
> > +      auto alignSelf = isOrthogonal ?
> > +        child->StylePosition()->ComputedJustifySelf(containerSC) :
> > +        child->StylePosition()->ComputedAlignSelf(containerSC);
> 
> "alignSelf" is confusingly-named here, since it is *not necessarily
> align-self* -- it could just as easily be justify-self.
> 
> Maybe s/alignSelf/selfAlignment/ would help clarify that? (and reduce the
> immediate assumption that "this is align-self")

Well, the comment mentions both, but fair enough, "selfAlignment" is
better.

> > +          auto alignSelfForAxis = isInlineAxis ?
> > +            child->StylePosition()->ComputedJustifySelf(containerSC) :
> > +            child->StylePosition()->ComputedAlignSelf(containerSC);
> 
> I don't understand how this variable differs from "alignSelf" up above.

Yeah, I think the -self and -content alignment code were originally
in two separate scopes and I didn't realize this was actually the same
value when I merged them.  I removed the one above.

> This "isInlineAxis == isOrthogonal" check seems redundant -- this code is
> nested inside of this check from up above:
>    const bool parallellInline = isInlineAxis == isOrthogonal;
>    if (parallellInline) {

True.  Perhaps it will become a relevant check though if we change
the itemHasBaselineParallelToTrack condition to include grid/table?
I kept it as is and added a XXX comment there for now.

> I'm not 100% sure, but I think we might need to conditionally check for
> 'left' and 'right' somewhere in here, too

Good catch, fixed.

> So, you have documentation saying that ItemState::eSelfBaseline and
> eContentBaseline are mutually exclusive, but I don't immediately see how
> that's enforced here -- there's no strict "if/else" relationship between the
> blocks of code that sets them.

If 'validCombo' is true and 'alignContent' is NS_STYLE_ALIGN_[LAST_]BASELINE
then *-self can't be a baseline value.  I added assertions.


> > +    if (state & ItemState::eAnyBaseline) {
> > +      // XXX available size issue
> > +      LogicalSize avail(childWM, INFINITE_ISIZE_COORD, NS_UNCONSTRAINEDSIZE);
> > +      auto* rc = &aState.mRenderingContext;
> > +      ::MeasuringReflow(child, aState.mReflowState, rc, avail);
> 
> Do we really need a dedicated measuring reflow, just to establish the
> baseline? (Can we share another measuring reflow somehow? or the main
> reflow?)

That's a good question.  I think I'll punt on this until we fix
bug 1174569.  It's possible we can make this the real reflow,
at least in some cases.  I've added a XXX comment for now.

Comment 13

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8755938 [details] [diff] [review]
> part 4 - [css-grid] Implement Grid layout for
> (1) Why are we copying these specific pieces of state from the
> first-in-flow? A comment would be helpful here.  (In particular, it's
> non-obvious why the first-in-flow's mBaselineOffset is something that we'd
> want to copy to its continuations, since the continuations could have
> differently-sized content, and will have no border/padding-top, and hence
> would have different baseline offsets. But maybe we're treating the whole
> continuation-chain as having a single first/last-baseline shared among all
> of them? If so, this assignment makes some sense, but that needs documenting
> as well.)

The spec says:
"If a box spans multiple shared alignment contexts, it participates
in first (last) baseline self-alignment within its start-most (end-most)
shared alignment context along that axis."
https://drafts.csswg.org/css-align-3/#baseline-align-self

So, translated to grid items, that means the item is first-baseline
aligned in its first track, or last-baseline alignment in its last.

Note that the last-baseline offset is stored as a distance from
the border-box end edge so it's independent on any fragmentation
that might occur.

Fwiw, the relevant specs (Grid/Align/Writing-mode) doesn't really say
anything explicitly about what happens when the boxes are fragmented,
so I think we can do whatever we think makes sense there.

I added a comment:
// Copy the item's baseline data so that the item's last fragment can do
// last-baseline alignment if necessary.

Hmm, now that I tested this more carefully I found a bug - I'll file
a follow-up on that.

> > +      nscoord baselineAdjust = aBaselineAdjust;
> So, consider dropping it & just using aBaselineAdjust directly.

Did so.

> > +  MOZ_ASSERT(aGridItem.mBaselineOffset[aAxis] >= 0,
> > +             "baseline offset should be positive at this point");
> 
> s/positive/non-negative/, to accurately describe the assertion condition.

Fixed (non-negative is indeed what I meant).

> No compelling need to use "auto" here.  Just use the actual type, for
> clarity, please:
>   s/auto&/TrackSize&/

Fixed.

> > +      if (state & ItemState::eLastBaseline) {
> > +        aGridItem.mBaselineOffset[mAxis] += delta;
> 
> I wonder if it'd be better to remove this and check
> !isFirstBaseline instead

I think it's slightly clearer as is.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8758826 [details] [diff] [review]
part 3 - [css-grid] Caclulate the baseline offset values and baseline subtree alignment

> I'm not 100% sure, but I think we might need to conditionally check for
> 'left' and 'right' somewhere in here, too

So the addition here to fix that is:

> +          if (isInlineAxis) {
> +            switch (selfAlignment) {
> +              case NS_STYLE_ALIGN_LEFT:
> +                selfAlignment = wm.IsBidiLTR() ? NS_STYLE_ALIGN_START
> +                                               : NS_STYLE_ALIGN_END;
> +                break;
> +              case NS_STYLE_ALIGN_RIGHT:
> +                selfAlignment = wm.IsBidiLTR() ? NS_STYLE_ALIGN_END
> +                                               : NS_STYLE_ALIGN_START;
> +                break;
> +            }
> +          }

Other than that I'm just addressing your nits.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8758824 [details] [diff] [review]
part 2 - [css-grid] Add GridItemInfo::mState member ...

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

r=me on part 2

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8758826 [details] [diff] [review]
part 3 - [css-grid] Caclulate the baseline offset values and baseline subtree alignment

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +3676,5 @@
> +          if (isInlineAxis) {
> +            switch (selfAlignment) {
> +              case NS_STYLE_ALIGN_LEFT:
> +                selfAlignment = wm.IsBidiLTR() ? NS_STYLE_ALIGN_START
> +                                               : NS_STYLE_ALIGN_END;

So, this handles selfAlignment=LEFT/RIGHT in the inline-axis scenario. (This is inside of a new "if isInlineAxis" check).

BUT, I expect we *also* need to handle them in the not-inlineAxis scenario, too, right? (In that case, they unconditionally behave as "start", per last sentence in their definitions at https://drafts.csswg.org/css-align-3/#valdef-self-position-left )

It might be simplest to remove the "if (isInlineAxis)" check that wraps this code, and drop it down into the selfAlignment ternary condition, like so:
       case NS_STYLE_ALIGN_LEFT:
         selfAlignment = (!isInlineAxis || wm.IsBidiLTR()
                          ? NS_STYLE_ALIGN_START
                          : NS_STYLE_ALIGN_END);
       case NS_STYLE_ALIGN_RIGHT:
         selfAlignment = (!isInlineAxis || wm.IsBidiLTR()
                          ? NS_STYLE_ALIGN_END
                          : NS_STYLE_ALIGN_START);

@@ +3756,5 @@
> +               "*-self and *-content baseline bits are mutually exclusive");
> +    MOZ_ASSERT(!(state &
> +                 (ItemState::eSelfBaseline | ItemState::eContentBaseline)) ==
> +               !(state &
> +                 (ItemState::eSelfBaseline | ItemState::eContentBaseline)),

The condition in this last assertion here seems to be a tautology.  It's effectively asserting (!foo == !foo).

I think this is just a copypaste typo -- I think you meant for the first expression to be about e{First,Last}Baseline instead of e{Self,Content}Baseline...?

Comment 17

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> BUT, I expect we *also* need to handle them in the not-inlineAxis scenario,
> too, right?

Yes, good point.  I mapped block-axis left/right to start.
(I'll file a follow-up bug on writing more "negative" tests for all
these value combinations.)

> I think this is just a copypaste typo

Indeed, thanks.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad5e5ff1669
part 1 - [css-grid] Break out the grid item measuring reflow code to a separate function.  Also, make the size contribution functions take a GridItemInfo instead of a frame, for use in later parts (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a36a1082b8f
part 2 - [css-grid] Add GridItemInfo::mState member (for each axis) and make the mIsFlexing bool into a state flag, and add baseline state flags.  Also add a baseline offset member.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd828a28532
part 3 - [css-grid] Caclulate the baseline offset values and baseline subtree alignment.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/017d5f639183
part 4 - [css-grid] Implement Grid layout for align|justify-self:baseline|last-baseline (aka "Baseline Self-Alignment").  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9274544598
part 5 - [css-grid] Reftests for align|justify-self:baseline|last-baseline.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ad5e5ff1669
https://hg.mozilla.org/mozilla-central/rev/0a36a1082b8f
https://hg.mozilla.org/mozilla-central/rev/1dd828a28532
https://hg.mozilla.org/mozilla-central/rev/017d5f639183
https://hg.mozilla.org/mozilla-central/rev/be9274544598