Mats Palmgren (inactive) Assignee
	Description • 8 years ago

https://drafts.csswg.org/css-align-3/#baseline-align-content
"Baseline Content-Alignment
Grid Items:
    A grid item participates in first (last) baseline content-alignment in either its row or column (whichever matches its inline axis) if its computed align-content is baseline (last-baseline), and its computed align-self or justify-self (whichever affects its block axis) is stretch or self-start (self-end). For this purpose, the start, end, flex-start, and flex-end values of align-self are treated as either self-start or self-end, whichever they end up equivalent to. "

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

Here we use the mBaselineOffset established in bug 1221525 to
implement *-content baseline alignment.  In this case I'm
propagating the value in a frame property since it needs to
affect the padding we set up in the reflow state.
https://drafts.csswg.org/css-align-3/#baseline-align-content
"a box participating in baseline content-alignment acts like its padding was increased"

The nsHTMLReflowState changes mostly just moves things around since
we now need the reflow flags in nsCSSOffsetState::InitOffsets (which
is a base class to nsHTMLReflowState where the mFlags lives).

The padding contribution is always positive (padding can't be negative)
so I'm exploiting that fact by negating the property value for
'last-baseline' to indicate that it's for the end padding.

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

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

	Daniel Holbert [:dholbert]
	Comment 3 • 8 years ago

(In reply to Mats Palmgren (:mats) from comment #1)
> The nsHTMLReflowState changes mostly just moves things around since
> we now need the reflow flags in nsCSSOffsetState::InitOffsets (which
> is a base class to nsHTMLReflowState where the mFlags lives).

Could most of these nsHTMLReflowState changes be split into a separate non-functional patch?  ("Part 0" if you like, or "part 1" with the main changes renamed to "part 2".)

Always best to perform non-functional code-moving/refactoring independently from functional changes, where possible.

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

Comment on attachment 8755944 [details] [diff] [review]
part 1 - [css-grid] Implement Grid layout for align|justify-content:baseline|last-baseline

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

r=me with the following fixed, though as noted above, I think most* of the nsHTMLReflowState changes should be split into their own no-functional-changes patch.  (Only the ApplyBaselinePadding lambda chunk should remain in this main patch, and perhaps the nsCSSOffsetState assertion-text-tweak)

::: layout/generic/nsGridContainerFrame.cpp
@@ +4580,5 @@
>      }
>      cb += aContentArea.Origin(wm);
>      aState.mRows.AlignBaselineSubtree(*aGridItemInfo);
>      aState.mCols.AlignBaselineSubtree(*aGridItemInfo);
> +    // Setup [align|justify]-content:[last-]baseline related properties.

This probably wants...
  s/properties/frame properties/
...to clarify which properties you're talking about here. (since [align|justify]-content are themselves CSS properties.)

(This tweak doesn't push this over 80 characters, so no rewrapping needed.)

@@ +4585,5 @@
> +    // These are added to the padding in nsCSSOffsetState::InitOffsets.
> +    // (a negative value signals the value is for 'last-baseline' and should be
> +    //  added to the (logical) end padding)
> +    typedef const FramePropertyDescriptor<SmallValueHolder<nscoord>>* Prop;
> +    auto SetProp = [aGridItemInfo, aChild] (LogicalAxis aAxis,

Consider s/aAxis/aGridAxis/; otherwise, it's ambiguous whether this is about the grid's block-axis/inline-axis vs. the child's grid/inline axis.)

::: layout/generic/nsHTMLReflowState.cpp
@@ +161,5 @@
>    MOZ_ASSERT(!aFrame->IsFlexOrGridItem(),
>               "We're about to resolve percent margin & padding "
>               "values against CB inline size, which is incorrect for "
> +             "flex/grid items"
> +             "Additionally for grid items, this path doesn't handle baseline "

Right now, this would print out as "...itemsAdditionally..."

Please add a period and a space between the word "items" and its close-quote, to fix this.

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

This does the nsHTMLReflowState refactoring separately as you suggested.

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

Try run together with bug 1221525:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba3fa36e865b

	Pulsebot
	Comment 8 • 8 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7294fad22949
part 1 - Move the ReflowStateFlags type to nsCSSOffsetState and add a 'ReflowStateFlags aFlags' param to nsCSSOffsetState::InitOffsets for later use (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea32b5e7ecf
part 2 - [css-grid] Implement Grid layout for align|justify-content:baseline|last-baseline (aka "Baseline Content-Alignment").  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4dfcc5160c
part 3 - [css-grid] Reftests for align|justify-content:baseline|last-baseline and mixed *-content/self tests.

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

https://hg.mozilla.org/mozilla-central/rev/7294fad22949
https://hg.mozilla.org/mozilla-central/rev/7ea32b5e7ecf
https://hg.mozilla.org/mozilla-central/rev/2b4dfcc5160c