Kurt Catti-Schmidt (Gerrit)

unread,

Sep 22, 2025, 8:53:35 PM (2 days ago) Sep 22

to Alison Maher, Ian Kilpatrick, Celeste Pan, Yanling Wang, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Attention needed from Alison Maher and Ian Kilpatrick

Kurt Catti-Schmidt added 3 comments

File third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm.cc

Line 310, Patchset 2 (Latest): std::optional<LayoutUnit> opt_fixed_inline_size = std::nullopt;

Kurt Catti-Schmidt . unresolved

Nit - the assignment is unnecessary, this'll be initialized to `std::nullopt` automatically.

Line 1042, Patchset 2 (Latest): bool used_block_constraint_at_track_sizing =

Kurt Catti-Schmidt . unresolved

`const bool`

Line 1044, Patchset 2 (Latest):

    if (used_block_constraint_at_track_sizing) {
      if (containing_size.block_size == kIndefiniteSize && !is_parallel) {
        CHECK_NE(containing_size.inline_size, kIndefiniteSize);
        fixed_available_size.block_size = *opt_fixed_inline_size;
      } else if (containing_size.inline_size == kIndefiniteSize &&
                 is_parallel) {
        fixed_available_size.inline_size = *opt_fixed_inline_size;
      }
    }

Kurt Catti-Schmidt . unresolved

Is it simpler to lift the `is_parallel` condition and swap cases so you get a straight if/else?

```
if (used_block_constraint_at_track_sizing) {
  if (is_parallel) {
    if(containing_size.inline_size == kIndefiniteSize) {
      fixed_available_size.inline_size = *opt_fixed_inline_size;
    }
  } else {
   // Set the block size to the fixed inline size if the item is orthogonal.
   if (containing_size.block_size == kIndefiniteSize) {
     CHECK_NE(containing_size.inline_size, kIndefiniteSize);
     fixed_available_size.block_size = *opt_fixed_inline_size;
    }
  }
}
```

Open in Gerrit

Related details

Attention is currently required from:

Alison Maher
Ian Kilpatrick

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Celeste Pan (Gerrit)

unread,

Sep 23, 2025, 1:54:58 PM (22 hours ago) Sep 23

to Alison Maher, Kurt Catti-Schmidt, Ian Kilpatrick, Yanling Wang, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Attention needed from Alison Maher and Ian Kilpatrick

Celeste Pan added 4 comments

Commit Message

Line 12, Patchset 2 (Latest):to the items max content [1].

Celeste Pan . unresolved

nit: "item's"

Line 20, Patchset 2 (Latest):when needed, which results in column-explicit-placement-002.html

Celeste Pan . unresolved

Could you elaborate on which cases "when needed" occurs? Is it just whenever the inline constraint is forced to max-content during track sizing, we will always enforce the same during layout?

File third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm.cc

Line 1016, Patchset 2 (Latest): const auto writing_mode = GetConstraintSpace().GetWritingMode();

Celeste Pan . unresolved

Would it make sense to move this and `opt_fixed_inline_size` closer down to where they're being used?

Line 1039, Patchset 2 (Latest): const auto item_writing_mode = masonry_item.node.Style().GetWritingMode();

Celeste Pan . unresolved

nit: could just call this directly in `IsParallelWritingMode` instead of caching. Same goes for `writing_mode`.

satisfied_requirement

unsatisfied_requirement

open

diffy

Alison Maher (Gerrit)

unread,

Sep 23, 2025, 3:00:07 PM (21 hours ago) Sep 23

to Kurt Catti-Schmidt, Ian Kilpatrick, Celeste Pan, Yanling Wang, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Attention needed from Celeste Pan, Ian Kilpatrick and Kurt Catti-Schmidt

Alison Maher voted and added 7 comments

Votes added by Alison Maher

Commit-Queue

+1

7 comments

Commit Message

Line 12, Patchset 2:to the items max content [1].

Celeste Pan . resolved

nit: "item's"

Alison Maher

Done

Line 20, Patchset 2:when needed, which results in column-explicit-placement-002.html

Celeste Pan . resolved

Could you elaborate on which cases "when needed" occurs? Is it just whenever the inline constraint is forced to max-content during track sizing, we will always enforce the same during layout?

Alison Maher

Yeah essentially in the same cases as [1]. I've added to the description to call that out

File third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm.cc

Line 310, Patchset 2: std::optional<LayoutUnit> opt_fixed_inline_size = std::nullopt;

Kurt Catti-Schmidt . resolved

Nit - the assignment is unnecessary, this'll be initialized to `std::nullopt` automatically.

Alison Maher

Done

Line 1016, Patchset 2: const auto writing_mode = GetConstraintSpace().GetWritingMode();

Celeste Pan . resolved

Would it make sense to move this and `opt_fixed_inline_size` closer down to where they're being used?

Alison Maher

Done

Line 1039, Patchset 2: const auto item_writing_mode = masonry_item.node.Style().GetWritingMode();

Celeste Pan . resolved

nit: could just call this directly in `IsParallelWritingMode` instead of caching. Same goes for `writing_mode`.

Alison Maher

Yeah we could, but this is one where I think it is somewhat helpful to name these so it is clear which is the container and which is the item

Line 1042, Patchset 2: bool used_block_constraint_at_track_sizing =

Kurt Catti-Schmidt . resolved

`const bool`

Alison Maher

Done

Line 1044, Patchset 2: if (used_block_constraint_at_track_sizing) {


      if (containing_size.block_size == kIndefiniteSize && !is_parallel) {
        CHECK_NE(containing_size.inline_size, kIndefiniteSize);
        fixed_available_size.block_size = *opt_fixed_inline_size;
      } else if (containing_size.inline_size == kIndefiniteSize &&
                 is_parallel) {
        fixed_available_size.inline_size = *opt_fixed_inline_size;
      }
    }

Kurt Catti-Schmidt . resolved

Is it simpler to lift the `is_parallel` condition and swap cases so you get a straight if/else?

```
if (used_block_constraint_at_track_sizing) {
  if (is_parallel) {
    if(containing_size.inline_size == kIndefiniteSize) {
      fixed_available_size.inline_size = *opt_fixed_inline_size;
    }
  } else {
   // Set the block size to the fixed inline size if the item is orthogonal.
   if (containing_size.block_size == kIndefiniteSize) {
     CHECK_NE(containing_size.inline_size, kIndefiniteSize);
     fixed_available_size.block_size = *opt_fixed_inline_size;
    }
  }
}
```

Alison Maher

Good point, updated

Open in Gerrit

Related details

Attention is currently required from:

Celeste Pan
Ian Kilpatrick
Kurt Catti-Schmidt

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Kurt Catti-Schmidt (Gerrit)

unread,

Sep 23, 2025, 6:02:35 PM (18 hours ago) Sep 23

to Alison Maher, Ian Kilpatrick, Celeste Pan, Yanling Wang, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Attention needed from Alison Maher, Celeste Pan and Ian Kilpatrick

Kurt Catti-Schmidt voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Alison Maher
Celeste Pan
Ian Kilpatrick

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Alison Maher (Gerrit)

unread,

Sep 23, 2025, 7:09:34 PM (17 hours ago) Sep 23

to Kurt Catti-Schmidt, Ian Kilpatrick, Celeste Pan, Yanling Wang, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Attention needed from Celeste Pan and Ian Kilpatrick

Alison Maher voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Celeste Pan
Ian Kilpatrick

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Sep 23, 2025, 7:22:14 PM (17 hours ago) Sep 23

to Alison Maher, Kurt Catti-Schmidt, Ian Kilpatrick, Celeste Pan, Yanling Wang, chromium...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, zol...@webkit.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[Masonry] Fix column-explicit-placement-002.html

column-explicit-placement-002.html was added in CL:6641991. This test
was failing because during track sizing, when we calculated the block
contribution for the orthogonal item, its inline constraint was forced
to the item's max content [1].

However, at layout time, we don't enforce this same constraint. This
works in Grid because we are constrained by the final track sizes in
both directions, but in masonry, we only constrain by tracks in one
direction.

This change ensures that we enforce the same constraint at layout time
when needed (see [1] for the cases where this is needed), which results
in column-explicit-placement-002.html rendering similarly to grid.

The reference file was also updated because the container has a width of
200px in the test file, which incorrectly didn't match the reference.

[1]
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/masonry/masonry_layout_algorithm.cc;l=745-761?q=ComputeMasonryItemBlockContribution&ss=chromium

Bug: 343257585

Change-Id: I6afea04b89d484eac0dd11a59a504db92a6c6e25

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6974401

Reviewed-by: Kurt Catti-Schmidt <ksc...@microsoft.com>

Commit-Queue: Alison Maher <alm...@microsoft.com>

Cr-Commit-Position: refs/heads/main@{#1519661}

Files:

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
M third_party/blink/web_tests/wpt_internal/css/css-masonry/column-explicit-placement-002-ref.html

Change size: M

Delta: 4 files changed, 52 insertions(+), 4 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Kurt Catti-Schmidt

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[Masonry] Fix column-explicit-placement-002.html [chromium/src : main]

Kurt Catti-Schmidt (Gerrit)

Kurt Catti-Schmidt added 3 comments

Related details

Celeste Pan (Gerrit)

Celeste Pan added 4 comments

Alison Maher (Gerrit)

Alison Maher voted and added 7 comments

Votes added by Alison Maher

7 comments

Related details

Kurt Catti-Schmidt (Gerrit)

Kurt Catti-Schmidt voted Code-Review+1

Related details

Alison Maher (Gerrit)

Alison Maher voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information