[Masonry] Apply stacking-axis alignment [chromium/src : main]
Celeste Pan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alison Maher
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 41 comments![]( "Open in Gerrit")
Some initial comments/questions. Haven't looked at tests yet
GridLayoutData& layout_data,
const Vector<StackingAxisStretchItem>& stacking_axis_stretch_items);Can these be const? If not, would be good to call them out in the method comment on how they are updated
LayoutUnit effective_stacking_axis_size,
LayoutUnit stacking_axis_gap,reading this, it isn't immediately obvious to me the difference between these two, so would be worth calling out in the comment for the method. Can these be combined at all into one LayoutUnit or do they need to remain separate?
GridTrackSizingDirection grid_axis_direction,Same comment here about not needing to pass this in
GridItems& grid_items,
GridLanesRunningPositions& running_positions,Can these be const?
GridTrackSizingDirection grid_axis_direction);nit: since these are part of the algo, no need to pass them in to these methods since it is pretty cheap to recalc in the methods with `Style().GridLanesTrackSizingDirection();`
void RunGridLanesPlacementPhase(nit: can you add a note about the the new optional out param in the method comment?
struct StackingAxisStretchItem {I'm wondering if this can live on running positions, same with the vector for them, that way we don't need to pass around a vector of these, and can access via running positions, which we are already passing around
// placement via `MoveChildrenInDirection`.Since this is just used internally, could this be a Pair of <LayoutObject, LayoutUnit> below instead of a new struct?
HasStackingAxisAlignment(style, *grid_items, grid_axis_direction);I think all of these can be calculated inside of running_positions based on what is already passed in there. Can we move the calculation for that there instead?
if (running_positions.IsStackingAxisAlignmentPresent()) {nit: instead of checking this here, we can move it to ApplyStackingAxisAlignment and have it return early if this is false (this way if another call site uses this, it will already be in place for both)
for (const auto& item : grid_items) {Instead of looping here, can we store maybe a member var for has_alignment_in_stacking_axis and compute this at another spot in the code we loop over items? IIRC we may do something similar for baselines as a reference
Vector<AlignmentEntry> alignment_entries;Maybe alignment_subjects
for (auto& grid_lanes_item : grid_items) {Do we need to do this for every grid item, or are we already keeping track of alignment candidates as we go, and can just loop through those?
alignment_space, /*size=*/LayoutUnit(),
/*margin_start=*/LayoutUnit(), /*margin_end=*/LayoutUnit(),
/*baseline_offset=*/LayoutUnit(), stacking_axis_alignment,Why are all of these 0?
is_for_columns ? grid_lanes_item.IsOverflowSafe(kForRows)Are there any tests for safe alignment? Does that apply in the stacking axis?
if (!alignment_entries.empty()) {If we inverted this check, could we early return?
```
if (alignment_entries.empty()) {
return;
}
...
```
container_builder_.MoveChildrenInDirection(Do we need to do this for stretch candidates?
// TODO(celestepan): Adding stretch items to the container after laying out
// the rest of the items may affect paint order. Verify and fix if needed.Yeah I think if we have to relayout, this would impact ordering of items. To avoid this, I think we need to add a way to run an extra layout pass that somehow ensures on the second pass, stretched items are aware that they need to stretch as they are being laid out.
Something we may want to run by Ian for ideas.
Technically if dense packing weren't a thing, we would be able to do this as we go (and not have to relayout) since we'd know alignment candidates as we go
Unless, like offsets, we can adjust the block size of the layout result after it has completed layout previously without rerunning layout (this would be the most ideal approach)
if (!stacking_axis_stretch_items.empty()) {If the comments I mentioned about having a way to keep track of alignment candidates and only loop through those above make sense, I wonder if there is a way to avoid this separate vector, and as we loop through those, have a way to tell which ones are stretched, and add those to a local vector to this method instead
stretch_item.opt_fixed_inline_size);Wondering if we can use this to handle fixed_available_size.inline_size = stretched_size;
And then have an opt_fixed_block_size as well for the other case
This way we don't need a base_space
const StyleSelfAlignmentData normal_value(ItemPosition::kNormal,
OverflowAlignment::kDefault);What does this do?
Vector<Vector<ItemAboveTrackOpening>> items_above_track_openings_;Can this live on TrackOpening?
// `[track_index][opening_index]`. Each entry stores the `GridItemData`nit: similar to the comment for track_collection_openings_, I'd maybe spell this out more explicitly. If we do need to keep this separate from track_collection_openings_, I'd maybe place these next to each other instead
bool stacking_axis_alignment_{false};nit: is_stacking_axis_alignment_set_
// When true, track openings are populated in `UpdateRunningPositionsForSpan`
// even without dense-packing, for stacking-axis alignment purposes.nit: "When true, the last item in a track or the last item above a spanner can be aligned within empty space available in the track as a result. This means, similar to dense packing, we'll need to keep track of track openings as we place items."
bool IsStackingAxisAlignmentPresent() const {nit IsStackingAxisAlignmentSet
GridItemData* placed_item = nullptr);Should we just always pass in the grid item, and then we can maybe get the span from the item itself?
GridItemData* placed_item = nullptr);nit: `item`
GridItemData* placed_item = nullptr);Can this be const?
// When `placed_item` is provided and stacking axis alignment is needed, thenit: The stacking axis alignment stated for `placed_item` will be updated for each track it spans if applicable. This can happen if the item is above a newly formed opening from a spanning item or if the item is above the last unbounded opening in the track.
CalculateAndCacheTrackSizes(track_collection);Should we do this for stacking_axis_alignment_present as well?
is_dense_packing_(style.IsGridLanesPackDense()),I think stacking_axis_alignment_ should be set here based on stacking_axis_alignment_present
bool stacking_axis_alignment_present = false)nit: maybe is_stacking_axis_alignment_set
bool stacking_axis_alignment_present = false)Will we handle self alignment as well, or just content alignment in the stacking axis?
UntracedMember<GridItemData> item = nullptr;Since this only holds an item, I think we can just store the item directly rather than a new struct type. Also, as mentioned later, this could maybe just live on TrackOpening instead
if (stacking_axis_alignment_ && placed_item) {If `stacking_axis_alignment_` should `placed_item` always be passed in? If so, I'd move this to CHECK(placed_item) instead (same with other similar if-statements)
if (stacking_axis_alignment_ && placed_item) {nit: can you add a comment for this one?
last_opening.start_position -= stacking_axis_gap;Is this not needed for openings except the end?
return min_opening_size == LayoutUnit::Max() ? LayoutUnit()
: min_opening_size;Since we check for found above, do we need to check if it equals Max here? Or do we need the check for found above, and just rely on the check here for Max?
Related details
- Celeste Pan
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. |
Celeste Pan (Gerrit)
Celeste Pan added 40 comments![]( "Open in Gerrit")
GridLayoutData& layout_data,
const Vector<StackingAxisStretchItem>& stacking_axis_stretch_items);Can these be const? If not, would be good to call them out in the method comment on how they are updated
Done
LayoutUnit effective_stacking_axis_size,
LayoutUnit stacking_axis_gap,reading this, it isn't immediately obvious to me the difference between these two, so would be worth calling out in the comment for the method. Can these be combined at all into one LayoutUnit or do they need to remain separate?
They represent 2 different things -- `effective_stacking_axis_size` is the size of the tacking axis, and `stacking_axis_gap` is the gap between items specified by the `gap` property. Will add a comment for this!
Same comment here about not needing to pass this in
Done
GridItems& grid_items,
GridLanesRunningPositions& running_positions,Celeste PanCan these be const?
`grid_items` can be const, but `running_positions` can't be const because in `FinalizeTrackOpeningsForStackingAxisAlignment` we alter the track openings stored in the class.
nit: since these are part of the algo, no need to pass them in to these methods since it is pretty cheap to recalc in the methods with `Style().GridLanesTrackSizingDirection();`
Done
nit: can you add a note about the the new optional out param in the method comment?
Done
struct StackingAxisStretchItem {I'm wondering if this can live on running positions, same with the vector for them, that way we don't need to pass around a vector of these, and can access via running positions, which we are already passing around
I feel like since we're really only touching it during the placement algorithm and relayout, which all happens in the GridLanesLayoutAlgorithm methods, it might make more sense for it to live in this file, as opposed to running positions. Happy to discuss more offline!
Since this is just used internally, could this be a Pair of <LayoutObject, LayoutUnit> below instead of a new struct?
I'd tried this before, there are GC restrictions, so we can't just use a plan pair.
HasStackingAxisAlignment(style, *grid_items, grid_axis_direction);I think all of these can be calculated inside of running_positions based on what is already passed in there. Can we move the calculation for that there instead?
I think it makes sense for style and grid_axis_direction, but I'm not sure it would make sense to store `grid_items`, or even a reference to `grid_items` just for this occasion. If you think it makes sense to have a `grid_items` pointer we could do that, I can add a TODO for it for a later change!
if (running_positions.IsStackingAxisAlignmentPresent()) {nit: instead of checking this here, we can move it to ApplyStackingAxisAlignment and have it return early if this is false (this way if another call site uses this, it will already be in place for both)
Done
for (const auto& item : grid_items) {Instead of looping here, can we store maybe a member var for has_alignment_in_stacking_axis and compute this at another spot in the code we loop over items? IIRC we may do something similar for baselines as a reference
If you're thinking of `BuildSizingCollection`, since it's a const method, it would be a bit awkward to set a member variable, since it's invoked by a shared util method. The baselines variables that's set is a local variable.
I do feel like keeping things contained in this O(n) loop is cleaner since I can't think of another alternative at the moment, but let me know if you feel otherwise!
Vector<AlignmentEntry> alignment_entries;Celeste PanMaybe alignment_subjects
Done
for (auto& grid_lanes_item : grid_items) {Do we need to do this for every grid item, or are we already keeping track of alignment candidates as we go, and can just loop through those?
At the moment we're not keeping track of alignment candidates as we go, since alignment candidates are constantly changing as placement happens -- it's more efficient to do this O(n) loop here than to loop through an existing alignment candidates vector during placement and remove/replace as we go through placement.
alignment_space, /*size=*/LayoutUnit(),
/*margin_start=*/LayoutUnit(), /*margin_end=*/LayoutUnit(),
/*baseline_offset=*/LayoutUnit(), stacking_axis_alignment,Why are all of these 0?
The "size" represents the size the item takes up, but since `alignment_space` already represents the exact amoutn of free space that we have, there is not size of the item that we need to subtract in `AlignmentOffset`.
For the margins, we've already accounted for the margin sizes when creating the track opening, so we shouldn't be passing in values for those either.
We're not doing baseline alignment in the stacking axis, so we shouldn't be passing in a `baseline_offset` here.
is_for_columns ? grid_lanes_item.IsOverflowSafe(kForRows)Are there any tests for safe alignment? Does that apply in the stacking axis?
I'm not sure this is applicable in the stacking axis, since we only perform the alignment if there is free space -- if there's no free space, then we wouldn't be performing alignment anyways.
I'll change the code to just pass in false!
If we inverted this check, could we early return?
```
if (alignment_entries.empty()) {
return;
}
...
```
We shouldn't do an early return here because `stacking_axis_stretch_items` isn't a subset of `alignment_entries`, so we do have to handle those separately.
Do we need to do this for stretch candidates?
No, since we're not moving them in a certain direction like we are for items that need start/end alignment, we're actually re-sizing them and re-laying them out.
// TODO(celestepan): Adding stretch items to the container after laying out
// the rest of the items may affect paint order. Verify and fix if needed.Yeah I think if we have to relayout, this would impact ordering of items. To avoid this, I think we need to add a way to run an extra layout pass that somehow ensures on the second pass, stretched items are aware that they need to stretch as they are being laid out.
Something we may want to run by Ian for ideas.
Technically if dense packing weren't a thing, we would be able to do this as we go (and not have to relayout) since we'd know alignment candidates as we go
Unless, like offsets, we can adjust the block size of the layout result after it has completed layout previously without rerunning layout (this would be the most ideal approach)
Okay, so I tried out a version where we don't defer the addition into the container builder, and we end up recording the child index of items that need stretch. Later on, we re-layout the items which need stretch, and then call `container_builder_.ReplaceChild` with the new fragment.
if (!stacking_axis_stretch_items.empty()) {If the comments I mentioned about having a way to keep track of alignment candidates and only loop through those above make sense, I wonder if there is a way to avoid this separate vector, and as we loop through those, have a way to tell which ones are stretched, and add those to a local vector to this method instead
Not sure if I'm fully understanding this, but I actually do prefer a separate vector for stretch items since we do separate work for this -- it feels clean to have that ``MoveChildrenInDirection on `alignment_entries` separately as well.
Also, we need `child_layout_subtree` for stretching subgrid items, so that's more information we keep separately from the other alignment entries.
stretch_item.opt_fixed_inline_size);Wondering if we can use this to handle fixed_available_size.inline_size = stretched_size;
And then have an opt_fixed_block_size as well for the other case
This way we don't need a base_space
Could you clarify what you mean here? Right now `CreateConstraintSpaceForLayout` only supports `opt_fixed_inline_size`. For column grid-lanes, the stretched size is in the block direction, so we'd need to either add an `opt_fixed_block_size` parameter to `CreateConstraintSpaceForLayout`, or keep the current two-step approach (base_space + CreateConstraintSpace).
I tried implementing an approach where we just use the track size for the grid-axis available size and the stretch size for the stacking axis available size, let me know if you think this approach is okay!
const StyleSelfAlignmentData normal_value(ItemPosition::kNormal,
OverflowAlignment::kDefault);Celeste PanWhat does this do?
It's creating the value that ResolvedAlignSelf/ResolvedJustifySelf returns when the final resolved alignment is `normal` (i.e., neither the item nor the parent has set an explicit alignment).
Vector<Vector<ItemAboveTrackOpening>> items_above_track_openings_;Can this live on TrackOpening?
Addressed with the other comments to have this live on TrackOpening.
// `[track_index][opening_index]`. Each entry stores the `GridItemData`nit: similar to the comment for track_collection_openings_, I'd maybe spell this out more explicitly. If we do need to keep this separate from track_collection_openings_, I'd maybe place these next to each other instead
No longer relevant since we're having these live on TrackOpening!
bool stacking_axis_alignment_{false};Celeste Pannit: is_stacking_axis_alignment_set_
Done
// When true, track openings are populated in `UpdateRunningPositionsForSpan`
// even without dense-packing, for stacking-axis alignment purposes.nit: "When true, the last item in a track or the last item above a spanner can be aligned within empty space available in the track as a result. This means, similar to dense packing, we'll need to keep track of track openings as we place items."
Done
bool IsStackingAxisAlignmentPresent() const {Celeste Pannit IsStackingAxisAlignmentSet
Done
Should we just always pass in the grid item, and then we can maybe get the span from the item itself?
Done
GridItemData* placed_item = nullptr);Celeste PanCan this be const?
No, because the `item_above` we store can't be const since we have to change it as the item above the track opening changes, and we need to assign that to `placed_item` sometimes.
GridItemData* placed_item = nullptr);Celeste Pannit: `item`
Done
// When `placed_item` is provided and stacking axis alignment is needed, thenit: The stacking axis alignment stated for `placed_item` will be updated for each track it spans if applicable. This can happen if the item is above a newly formed opening from a spanning item or if the item is above the last unbounded opening in the track.
I'm not sure this comment is fully accurate since we're not tracking stacking axis alignment per track.
Also, since we changed the parameters, this would change the comment as well. Let me know what you think about what I add for `grid_lanes_item`!
Should we do this for stacking_axis_alignment_present as well?
We don't actually need the track sizes for stacking axis alignment, since we're not deciding the placement of any items!
I think stacking_axis_alignment_ should be set here based on stacking_axis_alignment_present
Done
Will we handle self alignment as well, or just content alignment in the stacking axis?
It's accounting for self-alignment as well!
bool stacking_axis_alignment_present = false)Celeste Pannit: maybe is_stacking_axis_alignment_set
Done
Since this only holds an item, I think we can just store the item directly rather than a new struct type. Also, as mentioned later, this could maybe just live on TrackOpening instead
Done
If `stacking_axis_alignment_` should `placed_item` always be passed in? If so, I'd move this to CHECK(placed_item) instead (same with other similar if-statements)
since it's not optional anymore going to resolve this comment!
nit: can you add a comment for this one?
Done
Is this not needed for openings except the end?
yup, since for the other openings we do still want to account for gap between items!
return min_opening_size == LayoutUnit::Max() ? LayoutUnit()
: min_opening_size;Since we check for found above, do we need to check if it equals Max here? Or do we need the check for found above, and just rely on the check here for Max?
Replaced with just returning min_opening_size -- we still want `found_opening` above since otherwise we would return a min opening that is the max size.
Related details
- Alison Maher
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 21 comments![]( "Open in Gerrit")
Parent: e863db91 (Printing: Check for inactive RFHs when handling more printing requests)I can't remember - do we already have tests for alignment + fill-reverse/track-reverse?
// offsets, and relayouts stretch items with their stretched size.nit: "items that are stretch aligned"
Vector<StackingAxisStretchItem>* stacking_axis_stretch_items = nullptr);nit: maybe instead items_to_stretch_in_stacking_axis (it's a bit more verbose, but slightly easier to read
// items that have stretch alignment in the stacking axis."will need to be stretched in the stacking axis"
struct StackingAxisStretchItem {Celeste PanI'm wondering if this can live on running positions, same with the vector for them, that way we don't need to pass around a vector of these, and can access via running positions, which we are already passing around
I feel like since we're really only touching it during the placement algorithm and relayout, which all happens in the GridLanesLayoutAlgorithm methods, it might make more sense for it to live in this file, as opposed to running positions. Happy to discuss more offline!
Was thinking that the grid layout algo could call into running positions to get those items, though. But more than happy to discuss offline. I might be missing something
UntracedMember<GridItemData> item;Chatted with Kurt, and he said we can probably just make this a raw ptr instead.
https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#STACK_ALLOCATED
If the struct is on the stack, it can use STACK_ALLOCATED and refer to GC'd objects as raw pointers
UntracedMember<GridLayoutSubtree> child_layout_subtree;Same here
// placement via `MoveChildrenInDirection`.Celeste PanSince this is just used internally, could this be a Pair of <LayoutObject, LayoutUnit> below instead of a new struct?
I'd tried this before, there are GC restrictions, so we can't just use a plan pair.
If you used raw ptr for the LayoutObject in the pair does that work?
HasStackingAxisAlignment(style, *grid_items, grid_axis_direction);Celeste PanI think all of these can be calculated inside of running_positions based on what is already passed in there. Can we move the calculation for that there instead?
I think it makes sense for style and grid_axis_direction, but I'm not sure it would make sense to store `grid_items`, or even a reference to `grid_items` just for this occasion. If you think it makes sense to have a `grid_items` pointer we could do that, I can add a TODO for it for a later change!
Ah for some reason I was thinking we already were passing in the items. Makes sense, thanks!
for (const auto& item : grid_items) {Celeste PanInstead of looping here, can we store maybe a member var for has_alignment_in_stacking_axis and compute this at another spot in the code we loop over items? IIRC we may do something similar for baselines as a reference
If you're thinking of `BuildSizingCollection`, since it's a const method, it would be a bit awkward to set a member variable, since it's invoked by a shared util method. The baselines variables that's set is a local variable.
I do feel like keeping things contained in this O(n) loop is cleaner since I can't think of another alternative at the moment, but let me know if you feel otherwise!
Another alternative would be to have an out param for this information
for (auto& grid_lanes_item : grid_items) {Celeste PanDo we need to do this for every grid item, or are we already keeping track of alignment candidates as we go, and can just loop through those?
At the moment we're not keeping track of alignment candidates as we go, since alignment candidates are constantly changing as placement happens -- it's more efficient to do this O(n) loop here than to loop through an existing alignment candidates vector during placement and remove/replace as we go through placement.
Hmm let's maybe chat on this offline since I think we could improve this a bit based on how we end up storing this information inside of running positions
alignment_space, /*size=*/LayoutUnit(),
/*margin_start=*/LayoutUnit(), /*margin_end=*/LayoutUnit(),
/*baseline_offset=*/LayoutUnit(), stacking_axis_alignment,Celeste PanWhy are all of these 0?
The "size" represents the size the item takes up, but since `alignment_space` already represents the exact amoutn of free space that we have, there is not size of the item that we need to subtract in `AlignmentOffset`.
For the margins, we've already accounted for the margin sizes when creating the track opening, so we shouldn't be passing in values for those either.
We're not doing baseline alignment in the stacking axis, so we shouldn't be passing in a `baseline_offset` here.
Ok thanks - I think a comment here with the context could be useful
is_for_columns ? grid_lanes_item.IsOverflowSafe(kForRows)Celeste PanAre there any tests for safe alignment? Does that apply in the stacking axis?
I'm not sure this is applicable in the stacking axis, since we only perform the alignment if there is free space -- if there's no free space, then we wouldn't be performing alignment anyways.
I'll change the code to just pass in false!
So safe alignment is about what happens when you overflow the container (IIRC). We can technically overflow the container in the stacking axis, if for example, it is explicitly sized too small. I wonder if we do need support for that?
if (!alignment_entries.empty()) {Celeste PanIf we inverted this check, could we early return?
```
if (alignment_entries.empty()) {
return;
}
...
```
We shouldn't do an early return here because `stacking_axis_stretch_items` isn't a subset of `alignment_entries`, so we do have to handle those separately.
Why aren't those a subset? I would have thought if it stretches, it is also an alignment subject. Perhaps a comment or a change in the name of the var would help clarify that
container_builder_.MoveChildrenInDirection(Celeste PanDo we need to do this for stretch candidates?
No, since we're not moving them in a certain direction like we are for items that need start/end alignment, we're actually re-sizing them and re-laying them out.
Yeah this question came out of the fact I assumed alignment_subjects included stretched items
if (!stacking_axis_stretch_items.empty()) {Celeste PanIf the comments I mentioned about having a way to keep track of alignment candidates and only loop through those above make sense, I wonder if there is a way to avoid this separate vector, and as we loop through those, have a way to tell which ones are stretched, and add those to a local vector to this method instead
Not sure if I'm fully understanding this, but I actually do prefer a separate vector for stretch items since we do separate work for this -- it feels clean to have that ``MoveChildrenInDirection on `alignment_entries` separately as well.
Also, we need `child_layout_subtree` for stretching subgrid items, so that's more information we keep separately from the other alignment entries.
I find tracking these separately to be somewhat difficult to follow. Even if we need to track extra inforamation for stretch, that information can be optional, and if set, we know they are stretch candidates
Also the mention of subgrids gets me wondering - if a subgrid stretches, do we need to re-run layout in case that resize in the standalone axis could impact track sizing of subgridded items in the grid axis. May be worth a TODO for that (and something I may want to run by Ian to double check)
const auto& fragment = To<PhysicalBoxFragment>(
*container_builder_.Children()[stretch_item.child_index].fragment);
const LogicalBoxFragment original_fragment(
GetConstraintSpace().GetWritingDirection(), fragment);
const LayoutUnit original_stacking_size =
is_for_columns ? original_fragment.BlockSize()
: original_fragment.InlineSize();
const LayoutUnit stretched_size = original_stacking_size + alignment_space;
// Build the containing size using the item's original grid-axis size
// and the stretched stacking-axis size.
const LogicalSize containing_size =
is_for_columns
? LogicalSize(original_fragment.InlineSize(), stretched_size)
: LogicalSize(stretched_size, original_fragment.BlockSize());
const ConstraintSpace stretched_space = CreateConstraintSpace(
*stretch_item.item, containing_size,
/*fixed_available_size=*/containing_size,
LayoutResultCacheSlot::kLayout, stretch_item.child_layout_subtree);Can we create a new CreateConstraintSpaceForStretch or something along the lines that handles this logic for us?
stretch_item.opt_fixed_inline_size);Celeste PanWondering if we can use this to handle fixed_available_size.inline_size = stretched_size;
And then have an opt_fixed_block_size as well for the other case
This way we don't need a base_space
Could you clarify what you mean here? Right now `CreateConstraintSpaceForLayout` only supports `opt_fixed_inline_size`. For column grid-lanes, the stretched size is in the block direction, so we'd need to either add an `opt_fixed_block_size` parameter to `CreateConstraintSpaceForLayout`, or keep the current two-step approach (base_space + CreateConstraintSpace).
I tried implementing an approach where we just use the track size for the grid-axis available size and the stretch size for the stacking axis available size, let me know if you think this approach is okay!
Yeah I meant to add opt_fixed_block_size, and then use each of those in their respective cases to handle this instead
But updates make sense, thanks!
// When true, the last item in a track or the last item above a spanner can be
// aligned within empty space available in the track as a result. This means,
// similar to dense packing, we'll need to keep track of track openings as we
// place items.Should we note that this is true if any item is self aligned to be stretched or if the container has this set for all items?
UntracedMember<GridItemData> item_above = nullptr;Can this be a raw ptr instead (similar to other comments)
last_opening.start_position -= stacking_axis_gap;Celeste PanIs this not needed for openings except the end?
yup, since for the other openings we do still want to account for gap between items!
- Celeste Pan
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. |
Celeste Pan (Gerrit)
Celeste Pan added 19 comments![]( "Open in Gerrit")
Parent: e863db91 (Printing: Check for inactive RFHs when handling more printing requests)I can't remember - do we already have tests for alignment + fill-reverse/track-reverse?
Not for align-items -- I just tested it and it only works correctly for track reverse (we need to account for fill-reverse in our math for moving the items).
I do think that a spec issue needs to be raised for this -- in the case that we have `align-items: start` for columns, is the "start" where the fill-reverse is, or the start of the container?
I think this should be in a separate CL so we don't add too much to this current one. I'll add a TODO and file an issue for what I mentioned above!
// offsets, and relayouts stretch items with their stretched size.nit: "items that are stretch aligned"
Done
Vector<StackingAxisStretchItem>* stacking_axis_stretch_items = nullptr);nit: maybe instead items_to_stretch_in_stacking_axis (it's a bit more verbose, but slightly easier to read
Done
// items that have stretch alignment in the stacking axis."will need to be stretched in the stacking axis"
Done
struct StackingAxisStretchItem {Celeste PanI'm wondering if this can live on running positions, same with the vector for them, that way we don't need to pass around a vector of these, and can access via running positions, which we are already passing around
Alison MaherI feel like since we're really only touching it during the placement algorithm and relayout, which all happens in the GridLanesLayoutAlgorithm methods, it might make more sense for it to live in this file, as opposed to running positions. Happy to discuss more offline!
Was thinking that the grid layout algo could call into running positions to get those items, though. But more than happy to discuss offline. I might be missing something
Spoke offline -- we end up storing the alignment information in `TrackOpening`, and then we return the items above the openings in the `GetStackingAxisAlignmentCandidates` method, and then work with those in the `GridLanesLayoutAlgorithm`.
Chatted with Kurt, and he said we can probably just make this a raw ptr instead.
https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/heap/BlinkGCAPIReference.md#STACK_ALLOCATED
If the struct is on the stack, it can use STACK_ALLOCATED and refer to GC'd objects as raw pointers
Spoke offline with Ian, we should use the pattern of `HeapVector` and `Member`. This does require a `Trace` method and `WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS`.
UntracedMember<GridLayoutSubtree> child_layout_subtree;Celeste PanSame here
Used HeapVector with Member.
// placement via `MoveChildrenInDirection`.Celeste PanSince this is just used internally, could this be a Pair of <LayoutObject, LayoutUnit> below instead of a new struct?
Alison MaherI'd tried this before, there are GC restrictions, so we can't just use a plan pair.
If you used raw ptr for the LayoutObject in the pair does that work?
Used HeapVector with Member.
for (const auto& item : grid_items) {Celeste PanInstead of looping here, can we store maybe a member var for has_alignment_in_stacking_axis and compute this at another spot in the code we loop over items? IIRC we may do something similar for baselines as a reference
Alison MaherIf you're thinking of `BuildSizingCollection`, since it's a const method, it would be a bit awkward to set a member variable, since it's invoked by a shared util method. The baselines variables that's set is a local variable.
I do feel like keeping things contained in this O(n) loop is cleaner since I can't think of another alternative at the moment, but let me know if you feel otherwise!
Another alternative would be to have an out param for this information
That had been one of my thoughts, but since `BuildSizingCollection` is called by `BuildGridSizingTree`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_utils.cc;l=644;drc=d635c024a266e5abb2e1b8a68fe6a4fa7f7c5755;bpv=1;bpt=1,
which is a method used by both grid and grid lanes in grid_layout_utils.cc, it felt a bit awkward to add a grid-lanes specific parameter to all the methods leading up to `BuildSizingCollection`. I might be missing something here, so happy to talk about this offline as well.
I moved the calculation to `ConstructGridItems` instead and performed the check there when we're iterating through the items, so it would avoid an extra iteration! Let me know if you have feedback on this.
for (auto& grid_lanes_item : grid_items) {Do we need to do this for every grid item, or are we already keeping track of alignment candidates as we go, and can just loop through those?
Alison MaherAt the moment we're not keeping track of alignment candidates as we go, since alignment candidates are constantly changing as placement happens -- it's more efficient to do this O(n) loop here than to loop through an existing alignment candidates vector during placement and remove/replace as we go through placement.
Hmm let's maybe chat on this offline since I think we could improve this a bit based on how we end up storing this information inside of running positions
Okay, actually I realized that we could just iterate through the items above track openings held in GridLanesRunningPositions, and populate `alignment_subjects` with those. Since we have reference to the actual items, then it's not a problem for to check their alignment with that.
I added that as the implementation, but let's still talk more about how we might be able to improve it!
Edit: spoke offline, this is fine. Done!
alignment_space, /*size=*/LayoutUnit(),
/*margin_start=*/LayoutUnit(), /*margin_end=*/LayoutUnit(),
/*baseline_offset=*/LayoutUnit(), stacking_axis_alignment,Celeste PanWhy are all of these 0?
Alison MaherThe "size" represents the size the item takes up, but since `alignment_space` already represents the exact amoutn of free space that we have, there is not size of the item that we need to subtract in `AlignmentOffset`.
For the margins, we've already accounted for the margin sizes when creating the track opening, so we shouldn't be passing in values for those either.
We're not doing baseline alignment in the stacking axis, so we shouldn't be passing in a `baseline_offset` here.
Ok thanks - I think a comment here with the context could be useful
Done
is_for_columns ? grid_lanes_item.IsOverflowSafe(kForRows)Celeste PanAre there any tests for safe alignment? Does that apply in the stacking axis?
Alison MaherI'm not sure this is applicable in the stacking axis, since we only perform the alignment if there is free space -- if there's no free space, then we wouldn't be performing alignment anyways.
I'll change the code to just pass in false!
So safe alignment is about what happens when you overflow the container (IIRC). We can technically overflow the container in the stacking axis, if for example, it is explicitly sized too small. I wonder if we do need support for that?
Spoke offline, but will clarify here: if there's no space for alignment of the last item in each track, then we wouldn't move the item at all.
And then for items in track openings, it's fine to align them because there are already items below it, so it's not as though aligning the item within the opening is causing the overflow.
Added tests for safe alignment though, to make sure that the behavior IS the same as regular alignment!
if (!alignment_entries.empty()) {Celeste PanIf we inverted this check, could we early return?
```
if (alignment_entries.empty()) {
return;
}
...
```
Alison MaherWe shouldn't do an early return here because `stacking_axis_stretch_items` isn't a subset of `alignment_entries`, so we do have to handle those separately.
Why aren't those a subset? I would have thought if it stretches, it is also an alignment subject. Perhaps a comment or a change in the name of the var would help clarify that
Technically `alignment_entries` are only center and end alignment (since start doesn't need anything). We handle this separately from stretch items.
Just renamed to `items_to_offset_in_stacking_axis`!
// TODO(celestepan): Adding stretch items to the container after laying out
// the rest of the items may affect paint order. Verify and fix if needed.Celeste PanYeah I think if we have to relayout, this would impact ordering of items. To avoid this, I think we need to add a way to run an extra layout pass that somehow ensures on the second pass, stretched items are aware that they need to stretch as they are being laid out.
Something we may want to run by Ian for ideas.
Technically if dense packing weren't a thing, we would be able to do this as we go (and not have to relayout) since we'd know alignment candidates as we go
Unless, like offsets, we can adjust the block size of the layout result after it has completed layout previously without rerunning layout (this would be the most ideal approach)
Okay, so I tried out a version where we don't defer the addition into the container builder, and we end up recording the child index of items that need stretch. Later on, we re-layout the items which need stretch, and then call `container_builder_.ReplaceChild` with the new fragment.
Done
if (!stacking_axis_stretch_items.empty()) {Celeste PanIf the comments I mentioned about having a way to keep track of alignment candidates and only loop through those above make sense, I wonder if there is a way to avoid this separate vector, and as we loop through those, have a way to tell which ones are stretched, and add those to a local vector to this method instead
Alison MaherNot sure if I'm fully understanding this, but I actually do prefer a separate vector for stretch items since we do separate work for this -- it feels clean to have that ``MoveChildrenInDirection on `alignment_entries` separately as well.
Also, we need `child_layout_subtree` for stretching subgrid items, so that's more information we keep separately from the other alignment entries.
I find tracking these separately to be somewhat difficult to follow. Even if we need to track extra inforamation for stretch, that information can be optional, and if set, we know they are stretch candidates
Also the mention of subgrids gets me wondering - if a subgrid stretches, do we need to re-run layout in case that resize in the standalone axis could impact track sizing of subgridded items in the grid axis. May be worth a TODO for that (and something I may want to run by Ian to double check)
Spoke offline -- added a comment for why a simple re-layout is fine for now, as the case where the subgrid might end up resizing the stacking axis would cause a circularity (ie, we change the stacking axis size, the subgrid might be placed in a different location then, which means it might not even be a stretch item anymore, etc0.
const auto& fragment = To<PhysicalBoxFragment>(
*container_builder_.Children()[stretch_item.child_index].fragment);
const LogicalBoxFragment original_fragment(
GetConstraintSpace().GetWritingDirection(), fragment);
const LayoutUnit original_stacking_size =
is_for_columns ? original_fragment.BlockSize()
: original_fragment.InlineSize();
const LayoutUnit stretched_size = original_stacking_size + alignment_space;
// Build the containing size using the item's original grid-axis size
// and the stretched stacking-axis size.
const LogicalSize containing_size =
is_for_columns
? LogicalSize(original_fragment.InlineSize(), stretched_size)
: LogicalSize(stretched_size, original_fragment.BlockSize());
const ConstraintSpace stretched_space = CreateConstraintSpace(
*stretch_item.item, containing_size,
/*fixed_available_size=*/containing_size,
LayoutResultCacheSlot::kLayout, stretch_item.child_layout_subtree);Can we create a new CreateConstraintSpaceForStretch or something along the lines that handles this logic for us?
Done
// When true, the last item in a track or the last item above a spanner can be
// aligned within empty space available in the track as a result. This means,
// similar to dense packing, we'll need to keep track of track openings as we
// place items.Should we note that this is true if any item is self aligned to be stretched or if the container has this set for all items?
Done
Can this be a raw ptr instead (similar to other comments)
We ended up using HeapVector + Member pattern!
last_opening.start_position -= stacking_axis_gap;Celeste PanIs this not needed for openings except the end?
Alison Maheryup, since for the other openings we do still want to account for gap between items!
Sounds good, might be worth a comment for extra context
This is in the header file comment -- let me know if you'd prefer for me to move it into this file!
Related details
- Alison Maher
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 19 comments![]( "Open in Gerrit")
Parent: e863db91 (Printing: Check for inactive RFHs when handling more printing requests)Celeste PanI can't remember - do we already have tests for alignment + fill-reverse/track-reverse?
Not for align-items -- I just tested it and it only works correctly for track reverse (we need to account for fill-reverse in our math for moving the items).
I do think that a spec issue needs to be raised for this -- in the case that we have `align-items: start` for columns, is the "start" where the fill-reverse is, or the start of the container?
I think this should be in a separate CL so we don't add too much to this current one. I'll add a TODO and file an issue for what I mentioned above!
Acknowledged
// Computed during item construction in grid-lanes.nit: no need to say when this computes, but it could say something like "this only applies to layout modes that has a stacking axis, like grid lanes."
// We currently don't account for the case where the stretched item is anit: Can you add TODO(layout-dev): at the begininning? I won't add a tracking item for it since this is something we aren't planning to fix (at least not now), but this would just help it stand out as something that is technically missing or incorrect
for (const auto& item : grid_items) {Celeste PanInstead of looping here, can we store maybe a member var for has_alignment_in_stacking_axis and compute this at another spot in the code we loop over items? IIRC we may do something similar for baselines as a reference
Alison MaherIf you're thinking of `BuildSizingCollection`, since it's a const method, it would be a bit awkward to set a member variable, since it's invoked by a shared util method. The baselines variables that's set is a local variable.
I do feel like keeping things contained in this O(n) loop is cleaner since I can't think of another alternative at the moment, but let me know if you feel otherwise!
Celeste PanAnother alternative would be to have an out param for this information
That had been one of my thoughts, but since `BuildSizingCollection` is called by `BuildGridSizingTree`: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/grid/grid_layout_utils.cc;l=644;drc=d635c024a266e5abb2e1b8a68fe6a4fa7f7c5755;bpv=1;bpt=1,
which is a method used by both grid and grid lanes in grid_layout_utils.cc, it felt a bit awkward to add a grid-lanes specific parameter to all the methods leading up to `BuildSizingCollection`. I might be missing something here, so happy to talk about this offline as well.
I moved the calculation to `ConstructGridItems` instead and performed the check there when we're iterating through the items, so it would avoid an extra iteration! Let me know if you have feedback on this.
This solution makes sense to me, thanks!
const LayoutUnit offset = AlignmentOffset(nit: alignment_offset_adjustment
static_cast<wtf_size_t>(container_builder_.Children().size()),Why is this case needed? Doesn't size() return wtf_size_t? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/vector.h;drc=3ca5cb59557399c66447326d1f9682d135db816d;l=1516
LayoutUnit GetAlignmentSpaceForItem(const GridItemData* item,nit: GetAvailableAlignmentSpaceForItem
// Returns the unique above track openings that have positive alignment space
// below them, along with each item's alignment space.all of the last items that are above a track opening that should be considered for stacking axis alignment. (A link to the spec here could be useful, as well)
// non-null for subgrids). Both are used for adjustments for alignment in the
// stacking axis after placement.nit: no need to say how they are used
// `child_index` is the index of the item's fragment in the containernit: item_index
AlignmentCandidate item_above;nit: alignment_candidate
LayoutUnit alignment_space;nit: Maybe "available_alignment_space"
Member<GridLayoutSubtree> child_layout_subtree;Same comment about this one being prefixed with "child".
Also might be worth a note that this is needed for stretch aligned items only as they will need to be relaid out once we know the final alignment candidate for a give track opening
wtf_size_t child_index{kNotFound};This naming makes me think it is the child index in item. Maybe item_index
// Stored on the track opening above which the item sits.nit: no need to say where it is stored
// Holds per-item data needed for stacking-axis alignment adjustment.nit: A link to the spec in this comment might be useful for future readers
https://drafts.csswg.org/css-grid-3/#stacking-self-alignment
bool found_opening = false;nit: found_consecutive_opening
HeapVector<GridLanesRunningPositions::AlignmentCandidate>
GridLanesRunningPositions::GetStackingAxisAlignmentCandidates() const {
DCHECK(is_stacking_axis_alignment_set_);
// References to multi-span items may appear multiple times across various
// track openings, so we use `seen_items` to ensure that we only return one
// alignment candidate per item.
HashSet<const void*> seen_items;
HeapVector<AlignmentCandidate> candidates;
for (const auto& track_openings : track_collection_openings_) {
for (const auto& opening : track_openings) {
if (!opening.item_above.IsValid() ||
!seen_items.insert(opening.item_above.item.Get()).is_new_entry) {
continue;
}
const LayoutUnit alignment_space = GetAlignmentSpaceForItem(
opening.item_above.item,
opening.item_above.item->resolved_position.Span(
grid_axis_direction_));
if (alignment_space > LayoutUnit()) {
AlignmentCandidate candidate = opening.item_above;
candidate.alignment_space = alignment_space;
candidates.push_back(candidate);
}
}
}
return candidates;
}Can we create an iterator of alignment candidates instead? This way, we don't have to loop to find them, and then loop to apply alignment to them (and can just do that all in the same loop)?
Also, how do we handle spanners that may have different alignment space in different tracks, or even may span a track that it doesn't have any alignment space at all?
Related details
- Celeste Pan
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. |
Celeste Pan (Gerrit)
Celeste Pan added 17 comments![]( "Open in Gerrit")
nit: no need to say when this computes, but it could say something like "this only applies to layout modes that has a stacking axis, like grid lanes."
Done
// We currently don't account for the case where the stretched item is anit: Can you add TODO(layout-dev): at the begininning? I won't add a tracking item for it since this is something we aren't planning to fix (at least not now), but this would just help it stand out as something that is technically missing or incorrect
Done
const LayoutUnit offset = AlignmentOffset(Celeste Pannit: alignment_offset_adjustment
Done
static_cast<wtf_size_t>(container_builder_.Children().size()),Why is this case needed? Doesn't size() return wtf_size_t? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/vector.h;drc=3ca5cb59557399c66447326d1f9682d135db816d;l=1516
My bad, I had thought it returned a size_t! Just removed.
LayoutUnit GetAlignmentSpaceForItem(const GridItemData* item,Celeste Pannit: GetAvailableAlignmentSpaceForItem
Done
// Returns the unique above track openings that have positive alignment space
// below them, along with each item's alignment space.all of the last items that are above a track opening that should be considered for stacking axis alignment. (A link to the spec here could be useful, as well)
Done!
// non-null for subgrids). Both are used for adjustments for alignment in the
// stacking axis after placement.nit: no need to say how they are used
Done
// `child_index` is the index of the item's fragment in the containerCeleste Pannit: item_index
Done
AlignmentCandidate item_above;Celeste Pannit: alignment_candidate
Done
LayoutUnit alignment_space;Celeste Pannit: Maybe "available_alignment_space"
Done
Same comment about this one being prefixed with "child".
Also might be worth a note that this is needed for stretch aligned items only as they will need to be relaid out once we know the final alignment candidate for a give track opening
Done
This naming makes me think it is the child index in item. Maybe item_index
Done
// Stored on the track opening above which the item sits.nit: no need to say where it is stored
Done
// Holds per-item data needed for stacking-axis alignment adjustment.nit: A link to the spec in this comment might be useful for future readers
https://drafts.csswg.org/css-grid-3/#stacking-self-alignment
Done
bool found_opening = false;Celeste Pannit: found_consecutive_opening
Done
HeapVector<GridLanesRunningPositions::AlignmentCandidate>
GridLanesRunningPositions::GetStackingAxisAlignmentCandidates() const {
DCHECK(is_stacking_axis_alignment_set_);
// References to multi-span items may appear multiple times across various
// track openings, so we use `seen_items` to ensure that we only return one
// alignment candidate per item.
HashSet<const void*> seen_items;
HeapVector<AlignmentCandidate> candidates;
for (const auto& track_openings : track_collection_openings_) {
for (const auto& opening : track_openings) {
if (!opening.item_above.IsValid() ||
!seen_items.insert(opening.item_above.item.Get()).is_new_entry) {
continue;
}
const LayoutUnit alignment_space = GetAlignmentSpaceForItem(
opening.item_above.item,
opening.item_above.item->resolved_position.Span(
grid_axis_direction_));
if (alignment_space > LayoutUnit()) {
AlignmentCandidate candidate = opening.item_above;
candidate.alignment_space = alignment_space;
candidates.push_back(candidate);
}
}
}
return candidates;
}Can we create an iterator of alignment candidates instead? This way, we don't have to loop to find them, and then loop to apply alignment to them (and can just do that all in the same loop)?
Also, how do we handle spanners that may have different alignment space in different tracks, or even may span a track that it doesn't have any alignment space at all?
Done for the iterator portion!
We handle the spanner issue in `GetAlignmentSpaceForItem`, where we end up choosing the minimum space for all the openings that the item spans.
Related details
- Alison Maher
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 1 comment![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
1 comment
HashSet<const void*> seen_items_;nit: processed_alignment_candidates_
Related details
- Celeste Pan
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. |
Celeste Pan (Gerrit)
Celeste Pan added 1 comment![]( "Open in Gerrit")
HashSet<const void*> seen_items_;Celeste Pannit: processed_alignment_candidates_
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/60694.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_running_positions.h
Insertions: 1, Deletions: 1.
@@ -266,7 +266,7 @@
wtf_size_t opening_index_ = 0;
// Multi-span items may appear in multiple track openings; track which
// items have already been yielded to avoid returning duplicate candidates.
- HashSet<const void*> seen_items_;
+ HashSet<const void*> processed_alignment_candidates_;
};
AlignmentCandidateIterator GetAlignmentCandidateIterator() const {
```
```
The name of the file: third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_running_positions.cc
Insertions: 2, Deletions: 1.
@@ -280,7 +280,8 @@
// Skip invalid openings and already-seen multi-span items.
if (!opening.alignment_candidate.IsValid() ||
- !seen_items_.insert(opening.alignment_candidate.item.Get())
+ !processed_alignment_candidates_
+ .insert(opening.alignment_candidate.item.Get())
.is_new_entry) {
continue;
}
```
Change information
[Masonry] Apply stacking-axis alignment
Follow-ups will be needed to implement stacking-axis alignment together
with dense-packing, and to add tests to ensure that content alignment
works with stacking-axis alignment.
To account for items which need stacking-axis alignment, we keep track
of the item placed above each track opening. After placement is complete
and track openings are finalized, we perform stacking axis alignment. We
perform stretch alignment at the very end as we need to re-layout the
item and actually add it to the container builder.
Although we are still waiting on the CSSWG on whether or not explicit
sizing should override stretch alignment [1], this change assumes that
we do want to let explicit size override stretch alignment.
Lastly, tests were fixed to have widths/heights that matched with the
track with the largest running position, which is what the ref files had
-- previously the containers were falling short so it looked like the
stretch items weren't properly stretching.
Stacking axis alignment for OOF items are already tested in:
1.third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/abspos/column-grid-lanes-alignment.html
2.third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/abspos/column-grid-lanes-alignment-ref.html
3.third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/abspos/row-grid-lanes-alignment.html
4.third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/abspos/row-grid-lanes-alignment-ref.html
[1] https://github.com/w3c/csswg-drafts/issues/13950
- M third_party/blink/renderer/core/layout/fragment_builder.h
- M third_party/blink/renderer/core/layout/grid/grid_item.h
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_layout_algorithm.cc
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_layout_algorithm.h
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_node.cc
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_running_positions.cc
- M third_party/blink/renderer/core/layout/grid_lanes/grid_lanes_running_positions.h
- M third_party/blink/web_tests/TestExpectations
- M third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-items-003.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-items-004-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-items-004.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-items-005-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-items-005.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-items-006.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-items-007.html
- M third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/column-align-self-003.html
- M third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-001.html
- M third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-002.html
- M third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-003-ref.html
- M third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-003.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-004-ref.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-004.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-005.html
- A third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-items-006.html
- M third_party/blink/web_tests/external/wpt/css/css-grid/grid-lanes/alignment/row-justify-self-003.html
Code-Review: +1 by Alison Maher
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/60694
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |