[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. |
Alison Maher (Gerrit)
Alison Maher added 28 comments![]( "Open in Gerrit")
GridSizingTrackCollection& track_collection,
GridItemData& item,Are these both out params? If so, it'd be good to add this to the comment. Also if they are, they should be moved to the bottom of the variable list per the C++ style guide.
If not, then can these be const?
LayoutUnit TrackBaseline(const GridItemData& grid_item,nit: GetTrackBaseline to match the method naming below
void StoreItemBaseline(const LogicalBoxFragment& baseline_fragment,Would Set make sense here? If not, ok with Store
mutable bool has_baseline_aligned_items_ = false;Is this needed? Should be mutable without this I think
// indicating that a two-pass placement is needed to compute track baselines."and final item placement"
// Set to true if any item in the grid-lanes layout has baseline alignment,"When true, there is at least one item that is baseline aligned, indicating..."
LayoutUnit stacking_axis_gap,
PlacementPhase placement_phase);These and `sizing_constraint` look to be input params. According to the style guide, output params should always be placed last, so these should move up above `running_positions`
void LayoutEachGridLanesItem(Maybe something like `RunGridLanesPlacementPhase()`
// Iterates through and lays out each grid-lanes item in two passes to handleThis comment kind of implies that there are always two passes, and that this method running both. Instead, the caller handles running the passes, and this method does something different depending on the pass.
Given this, I wonder if we should update this to just note what happens if called with each type of placement phase.
i.e.
"This method iterates through and lays out each item in `grid_lanes_items`. If `placement_phase` is kCalculateBaselines, that means.... If `placement_phase` is kFinalPlacement, that means..."
I'd also suggest mentioning the out params in the comment (running_positions, baseline_accumulator)
// Iterates through and lays out each grid-lanes item in two passes to handlenit: "item in `grid_lanes_items`"
// Baseline alignment requires two passes if any items use baseline alignment."If an item is baseline aligned, placement is required in two passes because..."
// Most grid-lanes layouts don't use baseline alignment, so we can optimize
// by skipping the first pass when it's not needed.This comment can be removed since it is implied
// First pass: Compute track baselines by placing items to determine theirInstead of saying "First pass:" maybe something like "In the initial placemet pass, compute..."
// positions and baselines. This pass mutates running_positions throughnit: missing ``
// UpdateAutoPlacementCursor() calls. We use a copy to preserve the original"of `running_positoins`"
// UpdateAutoPlacementCursor() calls. We use a copy to preserve the original"their"
// positions and baselines. This pass mutates running_positions through
// FinalizeItemSpanAndGetMaxPosition(), UpdateRunningPositionsForSpan(), and
// UpdateAutoPlacementCursor() calls. We use a copy to preserve the originalInstead of noting where it is mutated, I think just noting what we need running positions for in the first pass would be sufficient
// Final placement pass (second pass if baseline alignment needed, otherwise
// the only pass).nit: "Run a final placement pass of all items and add their layout results to the container. This is the second placement pass if there are any items requiring baseline alignment, and the only placement pass otherwise."
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);
}Yanling WangThis 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.
Acknowledged
// Final placement pass: Position items and apply self-alignment. This mustnit: "In the final placement pass, position items..."
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.
Acknowledged
// after running position updates.nit: could you add more details why. i.e. "This check must happen after the running position updates because..."
// after running position updates.nit: "the"
if (placement_phase == PlacementPhase::kCalculateBaselines &&
!grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {
continue;
}
if (placement_phase == PlacementPhase::kCalculateBaselines) {nit: these can be combined
```
if (placement_phase == PlacementPhase::kCalculateBaselines
// <move your comment from above here>
if (!grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {
continue;
}
...
}
CHECK(grid_lanes_item.IsBaselineAligned(grid_axis_direction));If we adjust as mentioned above, this would be implied and can be removed
// PlacementPhase::kFinalPlacement.
//nit: This part of the comment can be removed
// Items are only added to the container in the final placement pass.
// During the baseline calculation pass, we only compute and store track
// baselines without adding items, since baseline information is needed
// before items can be properly aligned and placed.This is a great comment! I think some of these details around when item layout results are added to the container can be added to the comments at the call sites for this method, and perhaps in the comment for this method in the header, as well.
// Allocate baseline storage. The constructor creates the `baselines_`It's not clear what ctr this is referring to just by reading this. May be able to just simplify to "Allocate the major/minor baseline vectors now that we now the total size."
The name "Reset" also makes me think it could be used not just on constructing but resetting at some later point. Is that true, or is it just on set up?
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. |
Yanling Wang (Gerrit)
Yanling Wang added 26 comments![]( "Open in Gerrit")
GridSizingTrackCollection& track_collection,
GridItemData& item,Are these both out params? If so, it'd be good to add this to the comment. Also if they are, they should be moved to the bottom of the variable list per the C++ style guide.
If not, then can these be const?
Yes, they're both out params. Thanks for pointing out, I've updated it.
nit: GetTrackBaseline to match the method naming below
Done
void StoreItemBaseline(const LogicalBoxFragment& baseline_fragment,Would Set make sense here? If not, ok with Store
I feel like "store" better reflects that we're storing baseline contributions from individual items into the track collection, rather than just setting a value.
Is this needed? Should be mutable without this I think
The mutable is needed here because we modified it in the const method BuildVirtualGridLanesItems(), so without mutable the code won't compile.
// indicating that a two-pass placement is needed to compute track baselines.Yanling Wang"and final item placement"
Done
// Set to true if any item in the grid-lanes layout has baseline alignment,"When true, there is at least one item that is baseline aligned, indicating..."
Done
LayoutUnit stacking_axis_gap,
PlacementPhase placement_phase);These and `sizing_constraint` look to be input params. According to the style guide, output params should always be placed last, so these should move up above `running_positions`
thanks for pointing out, I've updated it.
void LayoutEachGridLanesItem(Yanling WangMaybe something like `RunGridLanesPlacementPhase()`
This makes more sense, I've updated it, thanks!
// Iterates through and lays out each grid-lanes item in two passes to handleThis comment kind of implies that there are always two passes, and that this method running both. Instead, the caller handles running the passes, and this method does something different depending on the pass.
Given this, I wonder if we should update this to just note what happens if called with each type of placement phase.
i.e.
"This method iterates through and lays out each item in `grid_lanes_items`. If `placement_phase` is kCalculateBaselines, that means.... If `placement_phase` is kFinalPlacement, that means..."
I'd also suggest mentioning the out params in the comment (running_positions, baseline_accumulator)
Done
// Iterates through and lays out each grid-lanes item in two passes to handleYanling Wangnit: "item in `grid_lanes_items`"
Done
// Baseline alignment requires two passes if any items use baseline alignment."If an item is baseline aligned, placement is required in two passes because..."
Done
// Most grid-lanes layouts don't use baseline alignment, so we can optimize
// by skipping the first pass when it's not needed.This comment can be removed since it is implied
Done
// First pass: Compute track baselines by placing items to determine theirInstead of saying "First pass:" maybe something like "In the initial placemet pass, compute..."
Done
// positions and baselines. This pass mutates running_positions throughYanling Wangnit: missing ``
Done
// UpdateAutoPlacementCursor() calls. We use a copy to preserve the originalYanling Wang"their"
Done
// UpdateAutoPlacementCursor() calls. We use a copy to preserve the originalYanling Wang"of `running_positoins`"
Done
// positions and baselines. This pass mutates running_positions through
// FinalizeItemSpanAndGetMaxPosition(), UpdateRunningPositionsForSpan(), and
// UpdateAutoPlacementCursor() calls. We use a copy to preserve the originalInstead of noting where it is mutated, I think just noting what we need running positions for in the first pass would be sufficient
Done
// Final placement pass (second pass if baseline alignment needed, otherwise
// the only pass).nit: "Run a final placement pass of all items and add their layout results to the container. This is the second placement pass if there are any items requiring baseline alignment, and the only placement pass otherwise."
Done
// Final placement pass: Position items and apply self-alignment. This mustnit: "In the final placement pass, position items..."
Done
// after running position updates.Yanling Wangnit: "the"
Done
nit: could you add more details why. i.e. "This check must happen after the running position updates because..."
Done
if (placement_phase == PlacementPhase::kCalculateBaselines &&
!grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {
continue;
}
if (placement_phase == PlacementPhase::kCalculateBaselines) {nit: these can be combined
```
if (placement_phase == PlacementPhase::kCalculateBaselines
// <move your comment from above here>
if (!grid_lanes_item.IsBaselineAligned(grid_axis_direction)) {
continue;
}
...
}
Done
CHECK(grid_lanes_item.IsBaselineAligned(grid_axis_direction));If we adjust as mentioned above, this would be implied and can be removed
Done
nit: This part of the comment can be removed
Done
// Items are only added to the container in the final placement pass.
// During the baseline calculation pass, we only compute and store track
// baselines without adding items, since baseline information is needed
// before items can be properly aligned and placed.This is a great comment! I think some of these details around when item layout results are added to the container can be added to the comments at the call sites for this method, and perhaps in the comment for this method in the header, as well.
Done
// Allocate baseline storage. The constructor creates the `baselines_`It's not clear what ctr this is referring to just by reading this. May be able to just simplify to "Allocate the major/minor baseline vectors now that we now the total size."
The name "Reset" also makes me think it could be used not just on constructing but resetting at some later point. Is that true, or is it just on set up?
Yes, that's true for "reset". Because the grid computes the item baseline during track sizing, it was called multiple times to clears and reallocates the baseline vectors when baselines need to be recomputed in the track sizing algorithm, not just on initial construction.
Related details
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 6 comments![]( "Open in Gerrit")
mutable bool has_baseline_aligned_items_ = false;Yanling WangIs this needed? Should be mutable without this I think
The mutable is needed here because we modified it in the const method BuildVirtualGridLanesItems(), so without mutable the code won't compile.
Is it possible just to make that method non-const?
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);So here we pass in kForRows in one and kForColumns in the other. Shouldn't only one apply in masonry? And as such, would only one direction of alignment apply depending on the direction of the container?
grid-auto-flow: dense;We were asked to keep both syntaxes for dense packing, so would you be able to add this back, alongside the grid-lanes-pack property? Same with the similar row test
<div class="grid-lanes lb">Haven't looked deaply at this, but why in the ref do we get overlapping of items in this case?
<html>We might want to split this test out since the wpt runner has limited size and this looks like it might cause overflow
<pre>flex max-function columns - items with relative width<br>baseline is not applied initially, but orthogonal items in an auto-sized row force repeating the track sizing and height is not indefinite in that phase.</pre>There should be no rows here since this is a column test
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. |
Yanling Wang (Gerrit)
Yanling Wang added 6 comments![]( "Open in Gerrit")
mutable bool has_baseline_aligned_items_ = false;Yanling WangIs this needed? Should be mutable without this I think
Alison MaherThe mutable is needed here because we modified it in the const method BuildVirtualGridLanesItems(), so without mutable the code won't compile.
Is it possible just to make that method non-const?
updated the method to be non-const.
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);So here we pass in kForRows in one and kForColumns in the other. Shouldn't only one apply in masonry? And as such, would only one direction of alignment apply depending on the direction of the container?
You're correct that only one direction applies in masonry. However, `ComputeBaselineOffset` already handles this internally - it has an early check that returns `LayoutUnit()` (zero offset) if the item is not baseline-aligned in the specified direction:
For instance, when `is_for_columns` is `true`:
`inline_alignment` uses the actual alignment, so `inline_baseline_offset` may be non-zero
`block_alignment` is forced to `kStart`, so `ComputeBaselineOffset` returns 0 for `block_baseline_offset`
And vice versa when `is_for_columns` is `false`.
While we could add a conditional to avoid the call entirely (e.g., inline_baseline_offset = is_for_columns ? ComputeBaselineOffset(...) : LayoutUnit()), it would add extra logic for it since the function already optimizes this internally.
grid-auto-flow: dense;We were asked to keep both syntaxes for dense packing, so would you be able to add this back, alongside the grid-lanes-pack property? Same with the similar row test
Done
<div class="grid-lanes lb">Haven't looked deaply at this, but why in the ref do we get overlapping of items in this case?
I think this is the expected behavior based on grid baseline alignment calculation. When using "justify-items: last baseline", items are aligned by their last baseline, which can cause them to extend beyond their grid area boundaries and overlap with other items.
For last baseline alignment (minor baseline group), the offset calculation is:
`return available_size - baseline_delta - item_size;`
The reference file demonstrates what grid produces in this scenario, and the test verifies that grid-lanes matches this behavior.
We might want to split this test out since the wpt runner has limited size and this looks like it might cause overflow
Done
<pre>flex max-function columns - items with relative width<br>baseline is not applied initially, but orthogonal items in an auto-sized row force repeating the track sizing and height is not indefinite in that phase.</pre>There should be no rows here since this is a column test
- Alison Maher
- Ian Kilpatrick
- 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. |
Alison Maher (Gerrit)
Alison Maher added 2 comments![]( "Open in Gerrit")
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);Yanling WangSo here we pass in kForRows in one and kForColumns in the other. Shouldn't only one apply in masonry? And as such, would only one direction of alignment apply depending on the direction of the container?
You're correct that only one direction applies in masonry. However, `ComputeBaselineOffset` already handles this internally - it has an early check that returns `LayoutUnit()` (zero offset) if the item is not baseline-aligned in the specified direction:
For instance, when `is_for_columns` is `true`:
`inline_alignment` uses the actual alignment, so `inline_baseline_offset` may be non-zero
`block_alignment` is forced to `kStart`, so `ComputeBaselineOffset` returns 0 for `block_baseline_offset`
And vice versa when `is_for_columns` is `false`.While we could add a conditional to avoid the call entirely (e.g., inline_baseline_offset = is_for_columns ? ComputeBaselineOffset(...) : LayoutUnit()), it would add extra logic for it since the function already optimizes this internally.
What happens though if the item does have justify-self: baseline set, but the container doesn't support justify-self because it is a row container. Will ComputeBaselineOffset return LayoutUnit() in that case? It seems to just check if the item alignment matches the track direction, and we will pass in both track directions.
If it still calculates a baseline in that case, we may want to scope to only one here
<div class="grid-lanes lb">Yanling WangHaven't looked deaply at this, but why in the ref do we get overlapping of items in this case?
I think this is the expected behavior based on grid baseline alignment calculation. When using "justify-items: last baseline", items are aligned by their last baseline, which can cause them to extend beyond their grid area boundaries and overlap with other items.
For last baseline alignment (minor baseline group), the offset calculation is:
`return available_size - baseline_delta - item_size;`The reference file demonstrates what grid produces in this scenario, and the test verifies that grid-lanes matches this behavior.
If it matches grid, that seems reasonable, so no need to block on this comment. But just so I understand, it sounds like internally, the major axis is the first baseline, and the minor axis is the last baseline.
So since these are using last baseline, we go down the minor baseline path. In this test, which items are aligned up with which? i.e. what are the alignment groups (I wouldn't have thought it's items in the same column). And why do some seem to align towards the left side of the container, and some on the right? Is it because of spanning items? Happy to chat offline to better understand this one
Related details
- Ian Kilpatrick
- 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 2 comments![]( "Open in Gerrit")
mutable bool has_baseline_aligned_items_ = false;Yanling WangIs this needed? Should be mutable without this I think
Alison MaherThe mutable is needed here because we modified it in the const method BuildVirtualGridLanesItems(), so without mutable the code won't compile.
Is it possible just to make that method non-const?
If we do want to make these methods const again, adding it to `GridItemData` instead will be easier, and it'll be optimal if you check when each item is appended.
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);So here we pass in kForRows in one and kForColumns in the other. Shouldn't only one apply in masonry? And as such, would only one direction of alignment apply depending on the direction of the container?
This should effectively be what happens, but it's not super clear looking at the code (`ComputeBaselineOffset` will return 0 if there's no alignment in that dimension). This could definitely use a comment though, because it looks like it's doing extra work.
@yanli...@microsoft.com, can you add a test where both align and justify are set with grid-lanes and make sure it's not applying both types of alignment?
Related details
- Ian Kilpatrick
- 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. |
Alison Maher (Gerrit)
Alison Maher added 1 comment![]( "Open in Gerrit")
<div class="grid-lanes lb">Yanling WangHaven't looked deaply at this, but why in the ref do we get overlapping of items in this case?
Alison MaherI think this is the expected behavior based on grid baseline alignment calculation. When using "justify-items: last baseline", items are aligned by their last baseline, which can cause them to extend beyond their grid area boundaries and overlap with other items.
For last baseline alignment (minor baseline group), the offset calculation is:
`return available_size - baseline_delta - item_size;`The reference file demonstrates what grid produces in this scenario, and the test verifies that grid-lanes matches this behavior.
If it matches grid, that seems reasonable, so no need to block on this comment. But just so I understand, it sounds like internally, the major axis is the first baseline, and the minor axis is the last baseline.
So since these are using last baseline, we go down the minor baseline path. In this test, which items are aligned up with which? i.e. what are the alignment groups (I wouldn't have thought it's items in the same column). And why do some seem to align towards the left side of the container, and some on the right? Is it because of spanning items? Happy to chat offline to better understand this one
Confirmed offline what is going on here, so closing this out
Yanling Wang (Gerrit)
Yanling Wang added 3 comments![]( "Open in Gerrit")
mutable bool has_baseline_aligned_items_ = false;Yanling WangIs this needed? Should be mutable without this I think
Alison MaherThe mutable is needed here because we modified it in the const method BuildVirtualGridLanesItems(), so without mutable the code won't compile.
Kurt Catti-SchmidtIs it possible just to make that method non-const?
If we do want to make these methods const again, adding it to `GridItemData` instead will be easier, and it'll be optimal if you check when each item is appended.
I considered adding it to `GridItemData`, but that would require iterating all items every time we need to check if the container has baseline-aligned items. The current approach gives us O(1) lookup since has_baseline_aligned_items_ is a container-level flag. We discover baseline alignment during virtual item creation in BuildVirtualGridLanesItems(), which naturally modifies the algorithm's state, so making these methods non-const is semantically reasonable and avoids the `mutable`
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);So here we pass in kForRows in one and kForColumns in the other. Shouldn't only one apply in masonry? And as such, would only one direction of alignment apply depending on the direction of the container?
Alison MaherYou're correct that only one direction applies in masonry. However, `ComputeBaselineOffset` already handles this internally - it has an early check that returns `LayoutUnit()` (zero offset) if the item is not baseline-aligned in the specified direction:
For instance, when `is_for_columns` is `true`:
`inline_alignment` uses the actual alignment, so `inline_baseline_offset` may be non-zero
`block_alignment` is forced to `kStart`, so `ComputeBaselineOffset` returns 0 for `block_baseline_offset`
And vice versa when `is_for_columns` is `false`.While we could add a conditional to avoid the call entirely (e.g., inline_baseline_offset = is_for_columns ? ComputeBaselineOffset(...) : LayoutUnit()), it would add extra logic for it since the function already optimizes this internally.
What happens though if the item does have justify-self: baseline set, but the container doesn't support justify-self because it is a row container. Will ComputeBaselineOffset return LayoutUnit() in that case? It seems to just check if the item alignment matches the track direction, and we will pass in both track directions.
If it still calculates a baseline in that case, we may want to scope to only one here
After debugging the specific edge case, I see the issue now, I've updated it to scope to only one here.
<div class="grid-lanes lb">Yanling WangHaven't looked deaply at this, but why in the ref do we get overlapping of items in this case?
Alison MaherI think this is the expected behavior based on grid baseline alignment calculation. When using "justify-items: last baseline", items are aligned by their last baseline, which can cause them to extend beyond their grid area boundaries and overlap with other items.
For last baseline alignment (minor baseline group), the offset calculation is:
`return available_size - baseline_delta - item_size;`The reference file demonstrates what grid produces in this scenario, and the test verifies that grid-lanes matches this behavior.
If it matches grid, that seems reasonable, so no need to block on this comment. But just so I understand, it sounds like internally, the major axis is the first baseline, and the minor axis is the last baseline.
So since these are using last baseline, we go down the minor baseline path. In this test, which items are aligned up with which? i.e. what are the alignment groups (I wouldn't have thought it's items in the same column). And why do some seem to align towards the left side of the container, and some on the right? Is it because of spanning items? Happy to chat offline to better understand this one
After our offline sync, this is the expected behavior. I've updated the test with larger track sizes and a different writing-mode to make the item alignment more visible.
Related details
- Alison Maher
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
2 comments
LayoutUnit inline_baseline_offset =
is_for_columns
? ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForColumns,
containing_rect.size.inline_size)
: LayoutUnit();
LayoutUnit block_baseline_offset =
is_for_columns
? LayoutUnit()
: ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForRows,
containing_rect.size.block_size);nit: I might reorganize slightly like so:
```
LayoutUnit inline_baseline_offset;
LayoutUnit block_baseline_offset;
if (is_for_columns) {
inline_baseline_offset = ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForColumns,
containing_rect.size.inline_size);
} else {
block_baseline_offset = ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForRows,
containing_rect.size.block_size);
```
And then might be worth a comment above to note that in grid-lanes, we only have tracks in one dimension, so baseline alignment is only supported in one dimension. Or something along those lines
<div class="grid-lanes lb">Yanling WangHaven't looked deaply at this, but why in the ref do we get overlapping of items in this case?
Alison MaherI think this is the expected behavior based on grid baseline alignment calculation. When using "justify-items: last baseline", items are aligned by their last baseline, which can cause them to extend beyond their grid area boundaries and overlap with other items.
For last baseline alignment (minor baseline group), the offset calculation is:
`return available_size - baseline_delta - item_size;`The reference file demonstrates what grid produces in this scenario, and the test verifies that grid-lanes matches this behavior.
Yanling WangIf it matches grid, that seems reasonable, so no need to block on this comment. But just so I understand, it sounds like internally, the major axis is the first baseline, and the minor axis is the last baseline.
So since these are using last baseline, we go down the minor baseline path. In this test, which items are aligned up with which? i.e. what are the alignment groups (I wouldn't have thought it's items in the same column). And why do some seem to align towards the left side of the container, and some on the right? Is it because of spanning items? Happy to chat offline to better understand this one
After our offline sync, this is the expected behavior. I've updated the test with larger track sizes and a different writing-mode to make the item alignment more visible.
Great thanks! Is it possible to split into two tests (one with your updates, and one with the old behavior). I think the original is still an interesting edge case (just tough to reason about), so would be good to test in masonry, as well
Related details
- Ian Kilpatrick
- 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. |
Yanling Wang (Gerrit)
Yanling Wang added 2 comments![]( "Open in Gerrit")
LayoutUnit inline_baseline_offset =
is_for_columns
? ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForColumns,
containing_rect.size.inline_size)
: LayoutUnit();
LayoutUnit block_baseline_offset =
is_for_columns
? LayoutUnit()
: ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForRows,
containing_rect.size.block_size);nit: I might reorganize slightly like so:
```
LayoutUnit inline_baseline_offset;
LayoutUnit block_baseline_offset;
if (is_for_columns) {
inline_baseline_offset = ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForColumns,
containing_rect.size.inline_size);
} else {
block_baseline_offset = ComputeBaselineOffset(grid_lanes_item, track_collection,
baseline_fragment, fragment,
style.GetFontBaseline(), kForRows,
containing_rect.size.block_size);
```And then might be worth a comment above to note that in grid-lanes, we only have tracks in one dimension, so baseline alignment is only supported in one dimension. Or something along those lines
Done
<div class="grid-lanes lb">Yanling WangHaven't looked deaply at this, but why in the ref do we get overlapping of items in this case?
Alison MaherI think this is the expected behavior based on grid baseline alignment calculation. When using "justify-items: last baseline", items are aligned by their last baseline, which can cause them to extend beyond their grid area boundaries and overlap with other items.
For last baseline alignment (minor baseline group), the offset calculation is:
`return available_size - baseline_delta - item_size;`The reference file demonstrates what grid produces in this scenario, and the test verifies that grid-lanes matches this behavior.
Yanling WangIf it matches grid, that seems reasonable, so no need to block on this comment. But just so I understand, it sounds like internally, the major axis is the first baseline, and the minor axis is the last baseline.
So since these are using last baseline, we go down the minor baseline path. In this test, which items are aligned up with which? i.e. what are the alignment groups (I wouldn't have thought it's items in the same column). And why do some seem to align towards the left side of the container, and some on the right? Is it because of spanning items? Happy to chat offline to better understand this one
Alison MaherAfter our offline sync, this is the expected behavior. I've updated the test with larger track sizes and a different writing-mode to make the item alignment more visible.
Great thanks! Is it possible to split into two tests (one with your updates, and one with the old behavior). I think the original is still an interesting edge case (just tough to reason about), so would be good to test in masonry, as well
- Alison Maher
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted Code-Review+1![]( "Open in Gerrit")
- Ian Kilpatrick
- Kurt Catti-Schmidt
- Yanling Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |