-
Notifications
You must be signed in to change notification settings - Fork 757
[css-grid-3][masonry] Define dense packing for masonry #9326 #12807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3666d49 to
af6f90f
Compare
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
af6f90f to
e647c1a
Compare
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}
No description provided.