1256429 - [css-grid] Implement Grid Item Baseline Content-Alignment

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Mats Palmgren (inactive)]

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

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.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Reftests for align|justify-content:baseline|last-baseline and mixed *-content/self tests

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

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 1 - Move the ReflowStateFlags type to nsCSSOffsetState and add a 'ReflowStateFlags aFlags' param to nsCSSOffsetState::InitOffsets for later use (idempotent patch)

This does the nsHTMLReflowState refactoring separately as you suggested.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Implement Grid layout for align|justify-content:baseline|last-baseline ...

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 9

[Carsten Book [:Tomcat]]

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