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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 2 bugs)

unspecified
mozilla49
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 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. "
(Assignee)

Comment 1

3 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.
Attachment #8755944 - Flags: review?(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.
Flags: needinfo?(mats)
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.
Attachment #8755944 - Flags: review?(dholbert) → review+
(Assignee)

Comment 5

3 years ago
This does the nsHTMLReflowState refactoring separately as you suggested.
Attachment #8755944 - Attachment is obsolete: true
Flags: needinfo?(mats)
Attachment #8758831 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8755945 - Attachment description: part 2 - [css-grid] Reftests for align|justify-content:baseline|last-baseline and mixed *-content/self tests → part 3 - [css-grid] Reftests for align|justify-content:baseline|last-baseline and mixed *-content/self tests

Comment 8

3 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.
(Assignee)

Updated

3 years ago
Flags: in-testsuite+

Comment 9

3 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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.