[Masonry] Implement item-level baseline alignment [chromium/src : main]
Alison Maher (Gerrit)
Alison Maher added 23 comments![]( "Open in Gerrit")
Initial pass, haven't take a look at tests yet
bool& must_create_baselines) const;nit: maybe `needs_baseline` or `needs_baseline_calculation`
bool& must_create_baselines) const;Since this is an out pram, I' suggest updating the comment to note what it is and what it gets set to
LayoutUnit TrackBaseline(const GridItemData& grid_lanes_item,This one looks pretty much the same as https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc;l=790-807?q=grid_layout_algorithm.cc&ss=chromium
Could we move that one to grid_layout_utils and reuse that here instead? That way if one gets updated, it will work in both.
LayoutUnit GetItemBaseline(const LogicalBoxFragment& baseline_fragment, // avoid mutating the state needed for final placement.Where are things getting mutated? Can we just not mutate it if we are in the first pass?
// avoid mutating the state needed for final placement.nit: I'd suggest adding more details here on why 2 passes are needed
GridLanesRunningPositions running_positions_for_baseline(running_positions);
PlaceAndAlignGridLanesItems(track_collection, grid_lanes_items, start_offset,
running_positions_for_baseline, sizing_constraint,
baseline_accumulator, stacking_axis_gap,
/*is_first_pass=*/true);Is there any way we can make `must_create_baselines` a member var that is something like `item_has_baseline_alignment_` and that way we don't need to run the first pass in that case?
// Second pass: Perform final placement and alignment using computednit: "the"
const LayoutUnit align_content_offset = AlignContentOffset(
is_for_columns ? intrinsic_block_size_ : intrinsic_inline_size,
is_for_columns ? ChildAvailableSize().block_size
: ChildAvailableSize().inline_size,
baseline_accumulator->FirstBaseline().value_or(LayoutUnit()),
content_alignment);
if (is_for_columns) {
if (ChildAvailableSize().block_size != kIndefiniteSize) {
container_builder_.MoveChildrenInDirection(align_content_offset,
/*is_block_direction=*/true);
}
} else {
if (ChildAvailableSize().inline_size != kIndefiniteSize) {
container_builder_.MoveChildrenInDirection(
align_content_offset, /*is_block_direction=*/false);
}This method handles alignment as well, so it is unclear PlaceGridLanesItems and PlaceAndAlignGridLanesItems should be different based on names alone. Trying to think of another names
void GridLanesLayoutAlgorithm::PlaceAndAlignGridLanesItems(Would it make sense for this to be an inner method of PlaceGridLanesItems so that we don't need to recalc all the vars below here? Might make reading this kind of confusing though, so not a strong pref
bool is_first_pass) {Instead of a bool, I wonder if it would make sense for this to be an enum:
```
enum PlacementPhase {
kCalculateBaselines,
kFinalPlacement,
}
```
That way it is more clear what the first and seconf pass are for
const LogicalBoxFragment baseline_fragment(
grid_lanes_item.BaselineWritingDirection(grid_axis_direction),
To<PhysicalBoxFragment>(result->GetPhysicalFragment()));Do we only need this if the item has baseline alignment set on itself?
// Second pass: Position items and apply baseline alignment.nit: It might be worth expanding here why this can only be done in the second pass. This also applies other kinds of alignment, so we may want to say "self-alignment" more generically
LayoutUnit inline_baseline_offset = ComputeBaselineOffset(nit: I'd add a space before this one to split things up slightly. We may even want this to go after the inline and block alignment vars below so they are paired with the comment that says "// Adjust item's position in the track based on style."
LayoutUnit inline_baseline_offset = ComputeBaselineOffset(
grid_lanes_item, track_collection, baseline_fragment, fragment,
style.GetFontBaseline(), kForColumns,
containing_rect.size.inline_size);
LayoutUnit block_baseline_offset = ComputeBaselineOffset(
grid_lanes_item, track_collection, baseline_fragment, fragment,
style.GetFontBaseline(), kForRows, containing_rect.size.block_size);Do we only need to do this if the item has baseline alignment set on itself?
// Calculate track baseline.nit: I'd suggest adding more details. Something like "In the first placement pass, we need to calculate the track baseline in order to..."
if (is_first_pass &&
grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {If the main point of the first pass is the logic below, could we instead earlier at some point say
```
if (is_first_pass &&
!grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {
continue;
```
That way at this point, we could do
```
if (is_first_pass) {
CHECK(grid_lanes_item.IsBaselineAligned(grid_axis_direction));
...
else {
container_builder_.AddResult(*result, containing_rect.offset, margins);
...
}
```
const bool has_synthesized_baseline =
!baseline_fragment.FirstBaseline().has_value();
grid_lanes_item.SetAlignmentFallback(grid_axis_direction,
has_synthesized_baseline);
const LayoutUnit extra_margin =
grid_lanes_item.IsLastBaselineSpecified(grid_axis_direction)
? (is_for_columns ? margins.inline_end : margins.block_end)
: (is_for_columns ? margins.inline_start : margins.block_start);
const LayoutUnit item_baseline = GetItemBaseline(
baseline_fragment, style.GetFontBaseline(),
grid_lanes_item.IsLastBaselineSpecified(grid_axis_direction));
const LayoutUnit total_baseline = extra_margin + item_baseline;
// Store baseline in appropriate track.
const auto& [begin_set_index, end_set_index] =
grid_lanes_item.SetIndices(grid_axis_direction);
if (grid_lanes_item.BaselineGroup(grid_axis_direction) ==
BaselineGroup::kMajor) {
track_collection.SetMajorBaseline(begin_set_index, total_baseline);
} else {
track_collection.SetMinorBaseline(end_set_index - 1, total_baseline);
}Is any of this something we can consolidate with logic in grid, or is this fairly different?
if (!is_first_pass) {nit: I'd suggest adding a comment here why we don't add items in the first pass
if (must_create_baselines) {nit: this block could use a comment on why when this is true, we reset the baselines
must_create_baselines)) {nit: since we only use this var once, we could remove it and update this to `/*must_create_baselines=*/false`
Related details
- Kurt Catti-Schmidt
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 5 comments![]( "Open in Gerrit")
GridLanesRunningPositions running_positions_for_baseline(running_positions);
PlaceAndAlignGridLanesItems(track_collection, grid_lanes_items, start_offset,
running_positions_for_baseline, sizing_constraint,
baseline_accumulator, stacking_axis_gap,
/*is_first_pass=*/true);Is there any way we can make `must_create_baselines` a member var that is something like `item_has_baseline_alignment_` and that way we don't need to run the first pass in that case?
Agreed, and most grid-lanes layouts will probably not use baseline alignment, so it can be optimized quite a bit for the general case.
You could make it a member variable or store it on `GridItems` as items are appended.
const LogicalBoxFragment baseline_fragment(
grid_lanes_item.BaselineWritingDirection(grid_axis_direction),
To<PhysicalBoxFragment>(result->GetPhysicalFragment()));Is this ever going to be different than `physical_fragment`? Can we use that instead?
must_create_baselines |=
item_data.IsBaselineSpecified(grid_axis_direction);Based on the other comment, this can likely be moved to a member variable or on `GridItems` - that way you won't need the out variable here either.
track_collection.ResetBaselines();Is this just to allocate the vectors for baselines? If so, I think this should just happen automatically when `must_create_baselines` is true in the constructor above. You can probably remove a call to `ResetBaselines` in grid if you do this too.
<title>CSS Grid Lanes Test: justify-items:baseline/last baseline</title>
<link rel="author" title="Yanling Wang" href="mailto:yanli...@microsoft.com">
<link rel="help" href="https://drafts.csswg.org/css-grid-3/#masonry-baseline-alignment">
<link rel="match" href="column-grid-lanes-item-baseline-001-ref.html">Tests overall are great, but baselines are so complex that there are a few categories not covered.
First, synthesized baselines. For these, you should use the Ahem font and an orthogonal item, see `third_party/blink/web_tests/external/wpt/css/css-grid/alignment/grid-row-axis-self-baseline-synthesized-001.html` and related.
Next, the cyclic dependency cases, which may not apply the same way for masonry, but should be tested regardless, see `third_party/blink/web_tests/external/wpt/css/css-grid/alignment/grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001.html` and related.
So a few more test cases and you should be all set re: testing. If they have failures or don't behave as expected, you can add them here and fix in a follow up, as this CL gets most of the functionality.
Related details
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yanling Wang (Gerrit)
Yanling Wang added 26 comments![]( "Open in Gerrit")
Since this is an out pram, I' suggest updating the comment to note what it is and what it gets set to
Done
nit: maybe `needs_baseline` or `needs_baseline_calculation`
Thanks for the suggestion! I've updated it to directly use member variable.
LayoutUnit TrackBaseline(const GridItemData& grid_lanes_item,This one looks pretty much the same as https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_algorithm.cc;l=790-807?q=grid_layout_algorithm.cc&ss=chromium
Could we move that one to grid_layout_utils and reuse that here instead? That way if one gets updated, it will work in both.
I've moved the core baseline calculation logic to `TrackBaseline()` in grid_layout_utils. I kept the `Baseline()` wrapper in grid_layout_algorithm since it works with GridLayoutData (which bundles row/column collections), while grid-lanes works directly with individual track collections. This way both implementations get the API that fits their data structures while sharing the actual calculation logic.
LayoutUnit GetItemBaseline(const LogicalBoxFragment& baseline_fragment,Done
Done
// avoid mutating the state needed for final placement.nit: I'd suggest adding more details here on why 2 passes are needed
Done
// avoid mutating the state needed for final placement.Where are things getting mutated? Can we just not mutate it if we are in the first pass?
We need to mutate `running_positions` during the first pass in order to place items and determine their baselines. The placement logic requires updating the running positions as items are placed (via FinalizeItemSpanAndGetMaxPosition, UpdateRunningPositionsForSpan, and UpdateAutoPlacementCursor)
GridLanesRunningPositions running_positions_for_baseline(running_positions);
PlaceAndAlignGridLanesItems(track_collection, grid_lanes_items, start_offset,
running_positions_for_baseline, sizing_constraint,
baseline_accumulator, stacking_axis_gap,
/*is_first_pass=*/true);Kurt Catti-SchmidtIs there any way we can make `must_create_baselines` a member var that is something like `item_has_baseline_alignment_` and that way we don't need to run the first pass in that case?
Agreed, and most grid-lanes layouts will probably not use baseline alignment, so it can be optimized quite a bit for the general case.
You could make it a member variable or store it on `GridItems` as items are appended.
Thanks for the suggestions! I've made it a member variable and optimized this for the general case.
// Second pass: Perform final placement and alignment using computedYanling Wangnit: "the"
Done
const LayoutUnit align_content_offset = AlignContentOffset(
is_for_columns ? intrinsic_block_size_ : intrinsic_inline_size,
is_for_columns ? ChildAvailableSize().block_size
: ChildAvailableSize().inline_size,
baseline_accumulator->FirstBaseline().value_or(LayoutUnit()),
content_alignment);
if (is_for_columns) {
if (ChildAvailableSize().block_size != kIndefiniteSize) {
container_builder_.MoveChildrenInDirection(align_content_offset,
/*is_block_direction=*/true);
}
} else {
if (ChildAvailableSize().inline_size != kIndefiniteSize) {
container_builder_.MoveChildrenInDirection(
align_content_offset, /*is_block_direction=*/false);
}This method handles alignment as well, so it is unclear PlaceGridLanesItems and PlaceAndAlignGridLanesItems should be different based on names alone. Trying to think of another names
Thanks for the feedback! I've renamed the `PlaceAndAlignGridLanesItems` to `LayoutEachGridLanesItem` since it iterates through and processes each individual item - performs layout, placement, and item alignment. Please let me know if you feel there's a more appropriate name for it.
void GridLanesLayoutAlgorithm::PlaceAndAlignGridLanesItems(Would it make sense for this to be an inner method of PlaceGridLanesItems so that we don't need to recalc all the vars below here? Might make reading this kind of confusing though, so not a strong pref
Thanks for the suggestion! I feel like it's better to keep it as is for readability, since the method is already quite long.
Instead of a bool, I wonder if it would make sense for this to be an enum:
```
enum PlacementPhase {
kCalculateBaselines,
kFinalPlacement,
}
```That way it is more clear what the first and seconf pass are for
Done
const LogicalBoxFragment baseline_fragment(
grid_lanes_item.BaselineWritingDirection(grid_axis_direction),
To<PhysicalBoxFragment>(result->GetPhysicalFragment()));Is this ever going to be different than `physical_fragment`? Can we use that instead?
The `baseline_fragment` uses BaselineWritingDirection(grid_axis_direction) rather than the container's writing direction. This ensures baselines are calculated relative to the correct axis.
const LogicalBoxFragment baseline_fragment(
grid_lanes_item.BaselineWritingDirection(grid_axis_direction),
To<PhysicalBoxFragment>(result->GetPhysicalFragment()));Do we only need this if the item has baseline alignment set on itself?
Thanks for the feedback. I've optimized the code to only create baseline_fragment when it's actually needed.
// Second pass: Position items and apply baseline alignment.nit: It might be worth expanding here why this can only be done in the second pass. This also applies other kinds of alignment, so we may want to say "self-alignment" more generically
Done
LayoutUnit inline_baseline_offset = ComputeBaselineOffset(nit: I'd add a space before this one to split things up slightly. We may even want this to go after the inline and block alignment vars below so they are paired with the comment that says "// Adjust item's position in the track based on style."
Done
LayoutUnit inline_baseline_offset = ComputeBaselineOffset(
grid_lanes_item, track_collection, baseline_fragment, fragment,
style.GetFontBaseline(), kForColumns,
containing_rect.size.inline_size);
LayoutUnit block_baseline_offset = ComputeBaselineOffset(
grid_lanes_item, track_collection, baseline_fragment, fragment,
style.GetFontBaseline(), kForRows, containing_rect.size.block_size);Do we only need to do this if the item has baseline alignment set on itself?
You're right that the baseline_offset is only meaningful to those items have baseline alignment set on itself. The `ComputeBaselineOffset` already has an early return for non-baseline items, and LogicalBoxFragment creation is a lightweight wrapper. I don't feel like we need to add an additional check here.
nit: I'd suggest adding more details. Something like "In the first placement pass, we need to calculate the track baseline in order to..."
Done
if (is_first_pass &&
grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {If the main point of the first pass is the logic below, could we instead earlier at some point say
```
if (is_first_pass &&
!grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {
continue;
```That way at this point, we could do
```
if (is_first_pass) {
CHECK(grid_lanes_item.IsBaselineAligned(grid_axis_direction));
...
else {
container_builder_.AddResult(*result, containing_rect.offset, margins);
...
}```
Done
const bool has_synthesized_baseline =
!baseline_fragment.FirstBaseline().has_value();
grid_lanes_item.SetAlignmentFallback(grid_axis_direction,
has_synthesized_baseline);
const LayoutUnit extra_margin =
grid_lanes_item.IsLastBaselineSpecified(grid_axis_direction)
? (is_for_columns ? margins.inline_end : margins.block_end)
: (is_for_columns ? margins.inline_start : margins.block_start);
const LayoutUnit item_baseline = GetItemBaseline(
baseline_fragment, style.GetFontBaseline(),
grid_lanes_item.IsLastBaselineSpecified(grid_axis_direction));
const LayoutUnit total_baseline = extra_margin + item_baseline;
// Store baseline in appropriate track.
const auto& [begin_set_index, end_set_index] =
grid_lanes_item.SetIndices(grid_axis_direction);
if (grid_lanes_item.BaselineGroup(grid_axis_direction) ==
BaselineGroup::kMajor) {
track_collection.SetMajorBaseline(begin_set_index, total_baseline);
} else {
track_collection.SetMinorBaseline(end_set_index - 1, total_baseline);
}Is any of this something we can consolidate with logic in grid, or is this fairly different?
Good suggestion! I've consolidated the shared baseline calculation logic into a helper function `StoreItemBaseline()`
nit: I'd suggest adding a comment here why we don't add items in the first pass
Done
must_create_baselines |=
item_data.IsBaselineSpecified(grid_axis_direction);Based on the other comment, this can likely be moved to a member variable or on `GridItems` - that way you won't need the out variable here either.
Done
nit: this block could use a comment on why when this is true, we reset the baselines
Done
Is this just to allocate the vectors for baselines? If so, I think this should just happen automatically when `must_create_baselines` is true in the constructor above. You can probably remove a call to `ResetBaselines` in grid if you do this too.
The constructor creates the baselines_ container (`baselines_.emplace()`) when `must_create_baselines` is true, but can't allocate the major/minor baseline vectors yet since the set count is unknown until `BuildSets()` completes. `ResetBaselines()` allocates those vectors once we know the count. I've added a comment to clarify this.
nit: since we only use this var once, we could remove it and update this to `/*must_create_baselines=*/false`
I've removed this since we decided to use member variable instead. Thanks!
<title>CSS Grid Lanes Test: justify-items:baseline/last baseline</title>
<link rel="author" title="Yanling Wang" href="mailto:yanli...@microsoft.com">
<link rel="help" href="https://drafts.csswg.org/css-grid-3/#masonry-baseline-alignment">
<link rel="match" href="column-grid-lanes-item-baseline-001-ref.html">Tests overall are great, but baselines are so complex that there are a few categories not covered.
First, synthesized baselines. For these, you should use the Ahem font and an orthogonal item, see `third_party/blink/web_tests/external/wpt/css/css-grid/alignment/grid-row-axis-self-baseline-synthesized-001.html` and related.
Next, the cyclic dependency cases, which may not apply the same way for masonry, but should be tested regardless, see `third_party/blink/web_tests/external/wpt/css/css-grid/alignment/grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001.html` and related.
So a few more test cases and you should be all set re: testing. If they have failures or don't behave as expected, you can add them here and fix in a follow up, as this CL gets most of the functionality.
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |