[Masonry] Apply [min|max]-content style to masonry items [chromium/src : main]
Celeste Pan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Alison Maher
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 4 comments![]( "Open in Gerrit")
}Hmmm... this shouldn't be necessary. Basically we shouldn't condition the algorithm, based on if the width has a particular keyword.
Put another way - there should be no different in the layout if we have
`width: min-content` or `width: 100px` (if `100px` == `min-content`).
bool is_for_min_max_sizing) {Do we need both args? (e.g. if comment above, can we get away with just std::optional<SizingConstraint> ?)
}Pull into lambda so we do thing on demand. e.g.
```
std::optional<LayoutUnit> fixed_inline_size = ([&]() {
if (sizing_constraint == kLayout)
return std::nullopt;
const ConstraintSpace space = CreateConstraintSpaceForMeasure(masonry_item);
const MinMaxSizes sizes =
ComputeMinAndMaxContentContributionForSelf(masonry_item.node,
space)
.sizes;
if (sizing_constraitn == kMinContent) {
return sizes.min_size;
}
})();
```
}```
const ConstraintSpace space = is_for_min_max_sizing ? CreateCon... : CreateCon...;
```
Related details
- Alison Maher
- Celeste Pan
- Kurt Catti-Schmidt
Code-Coverage
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")
DCHECK_EQ(fixed_available_size.inline_size, kIndefiniteSize);These DCHECK's aren't validating any logic - you're setting `fixed_available_size` to `kIndefiniteLogicalSize` above, so this'll always be true.
Is this supposed to be:
`DCHECK_EQ(opt_fixed_inline_size->inline_size, kIndefiniteSize);`
DCHECK_EQ(fixed_available_size.block_size, kIndefiniteSize);Same here
Related details
- Alison Maher
- Celeste Pan
Code-Coverage
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 1 comment![]( "Open in Gerrit")
Hmmm... this shouldn't be necessary. Basically we shouldn't condition the algorithm, based on if the width has a particular keyword.
Put another way - there should be no different in the layout if we have
`width: min-content` or `width: 100px` (if `100px` == `min-content`).
+1, we likely want to pass down which one we want if this is coming from a compute min/max call instead of trying to calculate it here.
Related details
- Celeste Pan
Code-Coverage
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 1 comment![]( "Open in Gerrit")
Alison MaherHmmm... this shouldn't be necessary. Basically we shouldn't condition the algorithm, based on if the width has a particular keyword.
Put another way - there should be no different in the layout if we have
`width: min-content` or `width: 100px` (if `100px` == `min-content`).
+1, we likely want to pass down which one we want if this is coming from a compute min/max call instead of trying to calculate it here.
Chatted a bit with Celeste on this, and I realized I was thinking this was something else (i.e. I thought this was being set for when we called ComputeMinMaxSizes as opposed to Layout directly). So, feel free to ignore my suggestion in the comment, but in either case, agree with Ian that we shouldn't need this, so this is likely indicative of a bug elsewhere (potentially track sizing or the constraint space)
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(fixed_available_size.inline_size, kIndefiniteSize);These DCHECK's aren't validating any logic - you're setting `fixed_available_size` to `kIndefiniteLogicalSize` above, so this'll always be true.
Is this supposed to be:
`DCHECK_EQ(opt_fixed_inline_size->inline_size, kIndefiniteSize);`
Maybe `DCHECK_NE(opt_fixed_inline_size->inline_size, kIndefiniteSize);` makes more sense here. Or is this supposed to be checked after the assignment? If not, can this DCHECK be removed?
Related details
- Celeste Pan
Code-Coverage
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 6 comments![]( "Open in Gerrit")
Hmmm... this shouldn't be necessary. Basically we shouldn't condition the algorithm, based on if the width has a particular keyword.
Put another way - there should be no different in the layout if we have
`width: min-content` or `width: 100px` (if `100px` == `min-content`).
Changes were made to use kLayout instead; I've also updated the test expectations as it looks like the min-content/max-content constraints of the container should NOT apply to the children of the container during layout, and fit-content should apply to them instead (as a default). However, I've put the min-content and max-content tests under a "fit-content" section in the TestExpectations file as they will only exhibit the correct behavior once we have Alison's change to have fit-content as the default behavior.
bool is_for_min_max_sizing) {Do we need both args? (e.g. if comment above, can we get away with just std::optional<SizingConstraint> ?)
Hm, I'm not sure if we can because in the `ComputeMinMaxSizes` pass we'd want to make sure to call `LayoutMasonryItemForMeasure` instead of `item.Layout(...)`. And there isn't really a way to differentiate if we're in a layout pass vs computing pass just based on the value of `sizing_constraint`, since both layout and measuring could use any of the `SizingConstraint` options (nullopt, kLayout, kMinContent, kMaxContent).
Pull into lambda so we do thing on demand. e.g.
```
std::optional<LayoutUnit> fixed_inline_size = ([&]() {
if (sizing_constraint == kLayout)
return std::nullopt;
const ConstraintSpace space = CreateConstraintSpaceForMeasure(masonry_item);
const MinMaxSizes sizes =
ComputeMinAndMaxContentContributionForSelf(masonry_item.node,
space)
.sizes;
if (sizing_constraitn == kMinContent) {
return sizes.min_size;
}})();
```
Done
```
const ConstraintSpace space = is_for_min_max_sizing ? CreateCon... : CreateCon...;
```
Done
DCHECK_EQ(fixed_available_size.inline_size, kIndefiniteSize);These DCHECK's aren't validating any logic - you're setting `fixed_available_size` to `kIndefiniteLogicalSize` above, so this'll always be true.
Is this supposed to be:
`DCHECK_EQ(opt_fixed_inline_size->inline_size, kIndefiniteSize);`
Alison was able to help me better understand the logic here, and the thing here is that we don't want to set up a forced constraint if the parent already has a size set in that direction, because in that case we would use that sizing constraint instead.
This will automatically be true at the moment, but may not be later when we add in code for subgrid (as marked by the TODO in `CreateConstraintSpaceForMeasure`). I'll add in a TODO that's identical to the one in `CreateConstraintForMeasure`!
DCHECK_EQ(fixed_available_size.block_size, kIndefiniteSize);Celeste PanSame here
Addressed in the above comment :)
Related details
- Alison Maher
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
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 4 comments![]( "Open in Gerrit")
class MasonryRunningPositions;This looks like a repeat of line 18 (probably from a rebase)
Celeste PanHmmm... this shouldn't be necessary. Basically we shouldn't condition the algorithm, based on if the width has a particular keyword.
Put another way - there should be no different in the layout if we have
`width: min-content` or `width: 100px` (if `100px` == `min-content`).
Changes were made to use kLayout instead; I've also updated the test expectations as it looks like the min-content/max-content constraints of the container should NOT apply to the children of the container during layout, and fit-content should apply to them instead (as a default). However, I've put the min-content and max-content tests under a "fit-content" section in the TestExpectations file as they will only exhibit the correct behavior once we have Alison's change to have fit-content as the default behavior.
Acknowledged
/*is_for_min_max_sizing=*/false);This should already be false, right? I may just remove the false default for these to be on the safe side
bool is_for_min_max_sizing) {Celeste PanDo we need both args? (e.g. if comment above, can we get away with just std::optional<SizingConstraint> ?)
Hm, I'm not sure if we can because in the `ComputeMinMaxSizes` pass we'd want to make sure to call `LayoutMasonryItemForMeasure` instead of `item.Layout(...)`. And there isn't really a way to differentiate if we're in a layout pass vs computing pass just based on the value of `sizing_constraint`, since both layout and measuring could use any of the `SizingConstraint` options (nullopt, kLayout, kMinContent, kMaxContent).
Hmm I'm somewhat confused by this, as well. Are you saying when we run layout, we may pass in a non-kLayout sizing constraint (or was this residual from the code you had added (but now removed) to force items to min or max content at layout time)? If so, I think we should only need the sizing constraint
Related details
- Celeste Pan
- Ian Kilpatrick
- 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. |
Celeste Pan (Gerrit)
Celeste Pan added 3 comments![]( "Open in Gerrit")
This looks like a repeat of line 18 (probably from a rebase)
Done
This should already be false, right? I may just remove the false default for these to be on the safe side
Done
bool is_for_min_max_sizing) {Celeste PanDo we need both args? (e.g. if comment above, can we get away with just std::optional<SizingConstraint> ?)
Alison MaherHm, I'm not sure if we can because in the `ComputeMinMaxSizes` pass we'd want to make sure to call `LayoutMasonryItemForMeasure` instead of `item.Layout(...)`. And there isn't really a way to differentiate if we're in a layout pass vs computing pass just based on the value of `sizing_constraint`, since both layout and measuring could use any of the `SizingConstraint` options (nullopt, kLayout, kMinContent, kMaxContent).
Hmm I'm somewhat confused by this, as well. Are you saying when we run layout, we may pass in a non-kLayout sizing constraint (or was this residual from the code you had added (but now removed) to force items to min or max content at layout time)? If so, I think we should only need the sizing constraint
Looked over this again, and after the discussion that we should always be using `kLayout` when running `PlaceMasonryItems` during the layout pass, you're right in that we don't need the variable anymore, and we can get by with just using `SizingConstraint`. Removed the boolean!
Related details
- Alison Maher
- Ian Kilpatrick
- Kurt Catti-Schmidt
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 added 6 comments![]( "Open in Gerrit")
ResolveItemToleranceForMasonry(Style(), masonry_available_size_));This was something that came through in a rebase from one of my recent CLs. I think we can continue to use `masonry_available_size_` here, instead of calling `ChildAvailableSize()`.
ResolveItemToleranceForMasonry(Style(), ChildAvailableSize()));Same here, `masonry_available_size_` instead
const auto is_for_layout = sizing_constraint == SizingConstraint::kLayout;I would use bool here, instead of auto
// We need to compute the available space for the item, IF we are using itnit: "if"
/*is_for_min_max_sizing=*/true);nit: up to you, but we could also pass in `!is_for_layout` instead
sizing_constraint == SizingConstraint::kLayoutCan we use `is_for_layout` here, too?
Related details
- Celeste Pan
- Ian Kilpatrick
- 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. |
Celeste Pan (Gerrit)
Celeste Pan added 6 comments![]( "Open in Gerrit")
ResolveItemToleranceForMasonry(Style(), masonry_available_size_));This was something that came through in a rebase from one of my recent CLs. I think we can continue to use `masonry_available_size_` here, instead of calling `ChildAvailableSize()`.
Done
ResolveItemToleranceForMasonry(Style(), ChildAvailableSize()));Celeste PanSame here, `masonry_available_size_` instead
Done
const auto is_for_layout = sizing_constraint == SizingConstraint::kLayout;I would use bool here, instead of auto
Done
// We need to compute the available space for the item, IF we are using itCeleste Pannit: "if"
Done
nit: up to you, but we could also pass in `!is_for_layout` instead
I think it's slightly easier to read with the `true`, so will leave as is, but thank you for the suggestion!
Can we use `is_for_layout` here, too?
- Alison Maher
- Ian Kilpatrick
- Kurt Catti-Schmidt
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 Code-Review+1![]( "Open in Gerrit")
- Celeste Pan
- Ian Kilpatrick
- 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
// Create the space based on whether we're doing this for measure or for
// layout.Nit: this comment can go away, the code is nice and clear
Related details
- Celeste Pan
- Ian Kilpatrick
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 1 comment![]( "Open in Gerrit")
// Create the space based on whether we're doing this for measure or for
// layout.Nit: this comment can go away, the code is nice and clear
- Ian Kilpatrick
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
11 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_layout_algorithm.cc
Insertions: 0, Deletions: 2.
@@ -209,8 +209,6 @@
: sizes.max_size);
})();
- // Create the space based on whether we're doing this for measure or for
- // layout.
const ConstraintSpace space =
is_for_layout
? CreateConstraintSpaceForLayout(masonry_item, track_collection,
```
Change information
[Masonry] Apply [min|max]-content style to masonry items
This change applies min/max-content sizing to all of the items inside of
the container when we are computing the min/max-content size of the
masonry container.
The new tests which display visible items have been triaged in
TestExpectations under the fit-content section, as we are waiting on
this change:
https://chromium-review.googlesource.com/c/chromium/src/+/6649165 to
always apply fit-content as the default behavior for items.
- M third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm.cc
- M third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm.h
- M third_party/blink/web_tests/TestExpectations
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-auto-placement-max-content-ref.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-auto-placement-max-content.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-auto-placement-min-content-ref.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-auto-placement-min-content.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-min-max-content-container-ref.html
- A third_party/blink/web_tests/wpt_internal/css/css-masonry/row-min-max-content-container.html
Code-Review: +1 by Kurt Catti-Schmidt, +1 by Alison Maher
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |