[Masonry] Account for specified-tracks when in dense-packing [chromium/src : main]
Celeste Pan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 3 comments![]( "Open in Gerrit")
bool has_user_specified_position = false;This variable is not used in this method. I think for encapsulation purposes, this makes more sense to be a private member variable in `MasonryRunningPositions`.
if (has_user_specified_position && item_span != initial_span) {
++item_span;
continue;
}
if (CalculateUsedTrackSize(item_span) != used_track_size) {
++item_span;
continue;
}These two can be combined into one condition:
```
if ((CalculateUsedTrackSize(item_span) != used_track_size) || (has_user_specified_position && (item_span != initial_span))) {
++item_span;
continue;
}
```
That'll read more literally like your comment too.
<!DOCTYPE html>
<html>
<link rel="help" href="https://drafts.csswg.org/css-grid-3">
<link rel="match" href="column-dense-packing-006-ref.html">
<link rel="author" title="Celeste Pan" href="mailto:celes...@microsoft.com">
<style>
.masonry {
display: masonry;
item-tolerance: 0;
grid-template-columns: repeat(4, 50px);
grid-auto-flow: dense;
}
</style>Do you need row tests?
Related details
- Alison Maher
- Celeste Pan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 3 comments![]( "Open in Gerrit")
// `has_user_specified_position` is an output parameter which will be set toThis should be has_author_specified_position, although a simpler name could be is_auto_placed and then you'd just need to reverse the logic
if (has_user_specified_position && item_span != initial_span) {
++item_span;
continue;
}I'm wondering if another way to do this is by changing the max_iterations in this case before the while loop. Shouldn't the number of iterations in the auto placement case just be 1, in which case, then we'd only check starting in the current track
Related details
- Celeste Pan
Celeste Pan (Gerrit)
Celeste Pan added 6 comments![]( "Open in Gerrit")
bool has_user_specified_position = false;This variable is not used in this method. I think for encapsulation purposes, this makes more sense to be a private member variable in `MasonryRunningPositions`.
Hm, it wouldn't really make sense as a private member variable since this is specific to each individual masonry item. This could also be a return value, but it seems a bit weird to have it as a return value because it's not really the whole purpose of the method.
Personally I thought this was the best way, but if there are suggestions outside of using a private member variable I would love to hear them!
// `has_user_specified_position` is an output parameter which will be set toThis should be has_author_specified_position, although a simpler name could be is_auto_placed and then you'd just need to reverse the logic
I like the `is_auto_placed` suggestion a lot, will use that!
if (has_user_specified_position && item_span != initial_span) {
++item_span;
continue;
}I'm wondering if another way to do this is by changing the max_iterations in this case before the while loop. Shouldn't the number of iterations in the auto placement case just be 1, in which case, then we'd only check starting in the current track
I actually don't think we need to do the whole `max_iterations` business once we stop iterating from after the auto-placement cursor; we can just ensure the endline of `item_span` is within range.
I think it makes sense to address this in the change where we always start our search for openings from the start of the track; even though the code changes there would be small, there would be a decent amount of test changes, so I think that should be separate from this.
I'll make a TODO for that!
if (has_user_specified_position && item_span != initial_span) {
++item_span;
continue;
}
if (CalculateUsedTrackSize(item_span) != used_track_size) {
++item_span;
continue;
}These two can be combined into one condition:
```
if ((CalculateUsedTrackSize(item_span) != used_track_size) || (has_user_specified_position && (item_span != initial_span))) {
++item_span;
continue;
}
```That'll read more literally like your comment too.
Done
<!DOCTYPE html>
<html>
<link rel="help" href="https://drafts.csswg.org/css-grid-3">
<link rel="match" href="column-dense-packing-006-ref.html">
<link rel="author" title="Celeste Pan" href="mailto:celes...@microsoft.com">
<style>
.masonry {
display: masonry;
item-tolerance: 0;
grid-template-columns: repeat(4, 50px);
grid-auto-flow: dense;
}
</style>Do you need row tests?
Yup, translated these tests into row tests!
missing extra line at the end
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
bool has_user_specified_position = false;Celeste PanThis variable is not used in this method. I think for encapsulation purposes, this makes more sense to be a private member variable in `MasonryRunningPositions`.
Hm, it wouldn't really make sense as a private member variable since this is specific to each individual masonry item. This could also be a return value, but it seems a bit weird to have it as a return value because it's not really the whole purpose of the method.
Personally I thought this was the best way, but if there are suggestions outside of using a private member variable I would love to hear them!
Since this is a property of an individual masonry item, it belongs in `GridItemData`. That way you won't need these out bools that hold state on a specific masonry item.
It looks like you can't just look at the `resolved_position`, so you'll need to add another bool to `GridItemData`. There are already a bunch of these for similar purposes (e.g. `has_subgridded_columns`), so that seems like the best place to put it.
// true if the item it auto-placed.Typo: "...is auto placed"
Related details
- Alison Maher
- Celeste Pan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 2 comments![]( "Open in Gerrit")
bool has_user_specified_position = false;Celeste PanThis variable is not used in this method. I think for encapsulation purposes, this makes more sense to be a private member variable in `MasonryRunningPositions`.
Kurt Catti-SchmidtHm, it wouldn't really make sense as a private member variable since this is specific to each individual masonry item. This could also be a return value, but it seems a bit weird to have it as a return value because it's not really the whole purpose of the method.
Personally I thought this was the best way, but if there are suggestions outside of using a private member variable I would love to hear them!
Since this is a property of an individual masonry item, it belongs in `GridItemData`. That way you won't need these out bools that hold state on a specific masonry item.
It looks like you can't just look at the `resolved_position`, so you'll need to add another bool to `GridItemData`. There are already a bunch of these for similar purposes (e.g. `has_subgridded_columns`), so that seems like the best place to put it.
That's a good idea! I'll also just set the `is_auto_placed` value in the method where we actually resolve the lines for the masonry item.
// true if the item it auto-placed.Celeste PanTypo: "...is auto placed"
Done
Related details
- Alison Maher
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Code-Review | +1 |
1 comment
LGTM but don't land until Alison gives a +1
Related details
- Alison Maher
- Celeste Pan
Code-Coverage
Code-Owners
Code-Review
| 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
if ((CalculateUsedTrackSize(item_span) != used_track_size) ||nit: I don't think this paren is needed
(!masonry_item.is_auto_placed && (item_span != initial_span))) {nit: I don't think this paren is needed
Related details
- Celeste Pan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Celeste Pan (Gerrit)
Celeste Pan added 2 comments![]( "Open in Gerrit")
if ((CalculateUsedTrackSize(item_span) != used_track_size) ||nit: I don't think this paren is needed
Done
(!masonry_item.is_auto_placed && (item_span != initial_span))) {nit: I don't think this paren is needed
Code-Coverage
Code-Owners
Code-Review
| 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 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 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/masonry/masonry_running_positions.cc
Insertions: 2, Deletions: 2.
@@ -188,8 +188,8 @@
// If the used track size of the item doesn't match the total track size of
// the span OR if the item we are attempting to place has a user-specified
// position that doesn't match the current span, move onto the next span.
- if ((CalculateUsedTrackSize(item_span) != used_track_size) ||
- (!masonry_item.is_auto_placed && (item_span != initial_span))) {
+ if (CalculateUsedTrackSize(item_span) != used_track_size ||
+ (!masonry_item.is_auto_placed && item_span != initial_span)) {
++item_span;
continue;
}
```
Change information
[Masonry] Account for specified-tracks when in dense-packing
If an item has a specified track and dense-packing is enabled, only
place the item in openings in the specified track.
- M third_party/blink/renderer/core/layout/grid/grid_item.h
- M third_party/blink/renderer/core/layout/masonry/masonry_node.cc
- M third_party/blink/renderer/core/layout/masonry/masonry_running_positions.cc
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-006-ref.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-006.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-008-ref.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/column-dense-packing-multi-span-008.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-004-ref.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-004.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-004-ref.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-dense-packing-multi-span-004.html
Code-Review: +1 by Alison Maher, +1 by Kurt Catti-Schmidt
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |