Skip to content

Conversation

@fantasai
Copy link
Collaborator

No description provided.

@fantasai fantasai force-pushed the dense-masonry branch 2 times, most recently from 3666d49 to af6f90f Compare September 16, 2025 02:46
Copy link
Collaborator

@alisonmaher alisonmaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just one open clarification that I think may be worth adding around how the auto placement cursor is used when looking for track openings

have the same total used size as the tracks into which it is currently placed,
move the item into that position
and rewind the [=auto-placement cursor=] and the [=running position=]
to their values before this item’s placement.
Copy link
Collaborator

@alisonmaher alisonmaher Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One open question I have is the order in which we break ties with openings of the same start offset. I presume it is using the auto-placement cursor, but do we start from the earlier openings in the current track, or the next track from the one we are currently starting in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It follows the same rules as normal placement... in other words, we move the item into the earliest placement order that would not change the placement of items after it. Maybe I should clarify this text with that concept...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, tweaked the wording a bit. I'll merge this, and if you have suggestions on how to clarify more we can try for improvements. :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell what text changed since I think commits got squashed maybe, but interesting. I'm curious what "earliest in placement order" means - the order in which items get placed, or the earliest in terms of offset? Given that items can be explicitly placed, the former would be tough to track.

I would have guessed we follow the placement cursor to some degree (likely starting from the track the item is originally placed in), and then loop back around to find the opening with the earliest start offset. If there is a tie, you would choose the one closest to the original auto placement cursor.

Or are you saying we should always start looking for track openings at track 0 (i.e. prefer track openings in earlier tracks if there is a tie?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we don't have an in-built ordering such that we can just say "earliest". Individual items are ordered, but their gaps aren't, unlike Grid.

Also, I think this shouldn't change the ordering of things if there isn't a hole somewhere; that is, if your only valid choices are the ends of columns, we should continue to pack as normal, following the placement cursor.

So I recommend saying that, if there are holes that don't correspond to the existing non-dense placements, we choose the "highest" one, tie-breaking with simple layout direction order. No attention paid to or affect on the cursor when this happens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a placement order, so what this is saying is if there's a gap earlier than the current location, then you place it as if it were in the earliest possible position where it would satisfy these constraints.

If you have a clearer way to express that, sure, but I don't want to repeat the placement rules here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I recommend saying that, if there are holes that don't correspond to the existing non-dense placements, we choose the "highest" one, tie-breaking with simple layout direction order. No attention paid to or affect on the cursor when this happens.

This makes sense to me and I think is worth updating to clarify since in Chromium we just implemented it based on the placement cursor position for tie breaking, and the current text doesn't really make it clearer to me that the auto placement cursor is ignored when choosing the potential openings

Copy link
Member

@tabatkins tabatkins Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have a well-defined "placement order" that includes gaps, no. We have a set of possible placements within the threshold distance of the highest one, and cursor+placement direction breaks ties when that set is more than one. This isn't using that set, tho, or even modifying it - it first finds a standard placement as normal, then looks for gaps with the same track size and decides if it wants to move to one of those instead.

(@fantasai and I chatted offline) Ah, you're talking about "what hypothetical rearranging of this item in the list of children would have put it in that hole; choose the position with the earliest index". This implies replaying the placement algorithm to some extent, and at minimum requires recording the state of the auto-placement cursor after each placement; this hypothetical rearrangement could, I think, have had knock-on effects as well on the placement of other items.

Instead, I just want to choose the highest valid hole, and if there are multiple valid holes within the tie threshold of the highest, choose the start-most one. In almost all cases this'll be the same as your intended position, but it's stateless and requires no memory of how placement was performed originally. All you need to maintain is the size and position of the holes you form as placement occurs. (I've given you some proposed text in our shared doc.)

@fantasai fantasai merged commit 1672b71 into w3c:main Sep 17, 2025
1 check passed
@fantasai fantasai deleted the dense-masonry branch September 17, 2025 18:08
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 24, 2025
Per the resolution here:
w3c/csswg-drafts#12807 (comment) ignore
the auto-placement cursor when searching for openings in dense-packing.

This change also updates tests to no longer prioritize track openings
closest after the auto-placement cursor.

Bug: 343257585
Change-Id: I7e6fe5f52298b532af5feeaf2fa70f362cca6945
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6975019
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Celeste Pan <celestepan@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1519720}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants