Skip to content
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

[css-grid-3] Masonry - Intrinsic sizing of tracks & masonry-grid doesn't produce to good/desirable results. #8206

Closed
bfgeek opened this issue Dec 8, 2022 · 11 comments

Comments

@bfgeek
Copy link

bfgeek commented Dec 8, 2022

This is a somewhat meta issue surrounding the current definition of how intrinsic track sizing works, and the intrinsic sizing of the masonry grid.

As currently defined (https://drafts.csswg.org/css-grid-3/#track-sizing) the tracks that have min-content, max-content, fit-content, and auto don't work as they do in regular grid. For most of the tracks the content based tracks will resolve to zero, and the auto will result to 1fr?

On top of this it means that an intrinsically sized masonry grid won't match developer expectations, e.g. width: max-content or similar (being a flex-item, grid-item, floating, etc, etc).
We currently have a similar issue for multi-line flexbox which we are trying to fix, but this is somewhat worse due to being the default.

There are multiple avenues to solving the above, however one would be make masonry its own display type with a reduced subset of allowable definitions for tracks.
Most masonry examples I've seen choose to size all the tracks the same size. An potential path forward would to allow repeats of "intrinsic" tracks where everything gets sizing as-if it was in one track, then decide how many tracks to insert.
On top of this you could additionally allow non-intrinsic tracks in combination with the repeater. E.g.
masonry-tracks: 100px repeat(auto-fill, minmax(min-content, 100px));

Edit - a simple example of intrinsic sizing:
https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11094

@fantasai
Copy link
Collaborator

fantasai commented Jul 20, 2023

Thinking about it, I think you're right, that these keywords should be defined over all the masonry items (including those ending up in other tracks). Using just the first row gets you reasonable results if all your items are the same size, but is prone to broken layouts if not. Better to have a little too much space in some columns than not enough.

@fantasai
Copy link
Collaborator

fantasai commented Sep 7, 2023

First cut of trying to spec this...

  1. Delete Grid Item Placement section.
  2. Replace Grid Axis Track Sizing section as below.
  3. Adjust Masonry Layout Algorithm as below.

Grid Axis Track Sizing

Track sizing works the same as in CSS Grid, except that when considering which items contribute to intrinsic sizes:

  • All items explicitly placed in that track contribute, and
  • All items without an explicit placement contribute

In the case of spanning items with no explicit placement, they are assumed to be placed at every possible start position, and contribute accordingly.

Example 1: Suppose there are two columns in the grid axis and that

  • Items A, B, and C have no explicit placement.
  • Item D is explicitly placed into the first column.

In this case, items A, B, C, and D all contribute to sizing the first column, and A, B, and C (but not D) contribute to the second column.

Example 2: Suppose there are 5 columns in the grid axis, with the middle having a fixed size of 100px and the other two being auto-sized. An item that spans 2 tracks and has an intrinsic contribution of 220px is essentially copied and assumed to exist:

  • At grid line 1, contributing 110px to each of the first two tracks.
  • At grid line 2, contributing 120px to the second track.
  • At grid line 3, contributing 120px to the fourth track.
  • At grid line 4, contributing 110px to the fourth and fifth tracks.

Note: This algorithm ensures that each track is at least big enough to accommodate every item that is ultimately placed in it, and does not create dependency cycles between placement and track sizing. However, depending on the variation in sizes, tracks could be larger than necessary: an exact fit is only guaranteed if all items are explicitly placed in the grid axis or all items are the same size (or matching multiples of that size, in the case of spanning items).

Masonry Layout Algorithm

Items are placed in order-modified document order, but items with a definite placement are placed before items with an indefinite position (as in regular grid layout).

For each of the tracks in the grid axis, keep a running position initialized to zero. First for each item with a definite placement in the grid axis, then for each item with an indefinite placement:

  1. If the item has an definite placement in the grid axis, use that placement. Otherwise, resolve its grid axis placement using these substeps:
    1. Starting at the first grid axis line in the implicit grid.
    2. Find the largest running position of the grid axis tracks that the item would span if it were placed at this line, and call this position max_pos.
    3. Increment the line number and repeat step 2 until the item would no longer fit inside the grid.
    4. Pick the line that resulted in the smallest max_pos as the item’s definite placement in the grid axis.
  2. Place the item in its grid axis tracks at the maximum of the running positions of the tracks it spans.
  3. Calculate the size of the item’s containing block and then layout the item. Then calculate its resulting margin box in the masonry axis. Set the running position of the spanned grid axis tracks to max_pos + margin-box-end + grid-gap.

@bfgeek
Copy link
Author

bfgeek commented Sep 7, 2023

I don't think its that straightforward. There are lots of dependencies on placement hidden within the grid track sizing algorithm - for example (after a very quick look):

The contribution size for a grid-item isn't the same value for "all" tracks, it needs to be calculated per placement - I'm a little concerned about the performance of this approach.

@fantasai fantasai added this to Thursday Afternoon in TPAC 2023 agenda Sep 9, 2023
@ethanjv
Copy link

ethanjv commented Sep 14, 2023

My two cents: I feel that the concept of track sizing function in grid is very tied to placement happening before sizing.

To me, it feels meaningless to have a max-content track that ends up being sized much larger than any item placed within that track. The comparison at the beginning of the spec with multi-column layout feels much more appropiate to masonry rather than grid when thinking of @fantasai latter proposal, and it pretty much ties to @bfgeek alternate proposal, where repeat(auto-fill, auto) will do something very similar: consider the intrinsic size of all items to determine an appropiate size for a track that will automatically repeat to fill the available space.

Providing more context on @bfgeek's last point, the final position of a grid item is relevant in case it spans something like minmax(auto, 100px). In such scenario, if we ignore the max track sizing function (because we don't know where the item will end up being placed) we won't constraint its automatic minimum size and accommodating its unconstrained min-content size could overflow the maximum of the track(s) in which we end up placing it.

I would argue that "The full power of grid layout is available in the grid axis" can be tricky to achieve with such approach if two completely different behaviors are expected depending on whether masonry is in the equation or not.

PS. I'm also concerned that considering all non-explicitly placed grid items in every posible start position will likely result in bad performance, that's a guaranteed quadratic algorithm.

@fantasai fantasai moved this from Thursday Afternoon to Friday Afternoon in TPAC 2023 agenda Sep 14, 2023
@fantasai
Copy link
Collaborator

that's a guaranteed quadratic algorithm

It's not quadratic. It's NxM where N is the number of items and M is the number of columns. And reduces to N (same as Grid, or IanK's proposal) if the columns have the same sizing function.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-grid-3] Masonry - Intrinsic sizing of tracks & masonry-grid doesn't produce to good/desirable results. , and agreed to the following:

  • RESOLVED: Remove first-row-is-special handling of intrinsic track sizing in Masonry spec, replace with all auto-placed items contribute to all intrinsically sized tracks
The full IRC log of that discussion <emeyer> iank_: Spec as it was initially introduced has some awkward placement + intrinsic size outcomes
<emeyer> …In grid layout, you have to put everything into areas and then size areas based on that placement
<emeyer> …Masonry layout is kind of backwards, in that you place your item into the highest available track
<emeyer> …The original spec got around this, but by placing everything into the first column after it had placed an item in each track, this had a lot of bad side effects around intrinsic sizing
<emeyer> …You can have intrinsically sized track and something goes into the second column, the column might not be wide enough
<emeyer> TabAtkins: The upshot is part of our larger argument that masonry shouldn’t be defined on top of grid because of the inside-out ness
<astearns> ack emilio
<astearns> ack fantasai
<emeyer> …We think masonry should be its own display type that can have its own layout approach and maybe some new features that aren’t possible as long as it’s based on grid
<emeyer> fantasai: I’d like to sidestep display types and focus on sizing
<emeyer> …We a gree with Ian that it’s not ideal and speccing something out that handles intrinsic sizing on its own terms makes sense
<emeyer> …We’d like to spec that out in the editor’s draft
<emeyer> …Asking for a resolution to remove the “first row is special” thing from the spec
<iank_> q+
<emeyer> …And add l anguage to keep intrinsic sizing from being dependent on placement
<emeyer> …If we embed into grid, this is a bit more complicated
<astearns> ack iank_
<emeyer> astearns: This reworking is necessary whether we change display types?
<emeyer> fantasai: Yes
<emeyer> iank_: I don’t think it’s that simple because there are a lot of touchpoints with the grid specification
<emeyer> …Regarding placement in the non-masonry axis regarding how the element contributes
<emeyer> …I’m a little concerned that implementations-wise, there’s lots and lots of edge cases that rely on grid placement
<astearns> ack fantasai
<emeyer> fantasai: I recognize that this is complicated, but I’d like to at least try
<emeyer> …We can definitely come back to where we are if our proposal isn’t workable
<emeyer> iank_: We could work out the issues in a PR
<emeyer> fantasai: True, but the Draft is a draft and we all agree the current draft isn’t what we want
<emeyer> iank_: I think the Microsoft folks had a look and came to similar conclusions
<emeyer> …I want to avoid falling into quadratic behavior with variable track sizes
<emeyer> …When I originally wrote this up, I thought it could be mitigated, but now I’m pretty strongly in the camp we’ll need a new display type
<emeyer> astearns: I propose we task Elika and Ian to work on a PR and come back to the group
<emeyer> fantasai: Can we please land changes in the ED and iterate until we either have a solution or decide we can’t?
<fantasai> It's an "editor's draft" for a reason.
<fantasai> This is exactly what editor's drafts are for
<emeyer> jensimmons: It sounds like there’s a lot of disagreement between Appole and Google on this
<emeyer> astearns: I was thinkg of it more as “let’s collaborate” so we know whatever gets into the draft won’t have immediate objections
<emeyer> jensimmons: I think there’s good questions about implementability and speed, and Ian’s skepticism is perhaps correct
<emeyer> …We want to push further down the road
<TabAtkins> It wasn't minus, but I said "yes, please work in the ED"
<emeyer> astearns: So we propose Elika fix up the Editor’s Draft and bring it back
<emeyer> iank_: Okay with me
<TabAtkins> s/minus/minuted/
<emeyer> fantasai: If we’re going to stick with the grid model, we should go this direction
<emeyer> astearns: I want to avoid resolving on a direction since we don’t know what the direction will be
<emeyer> fantasai: I think we need to take Ian’s approach contributions of all auto sized items in every auto sized track
<emeyer> …Updating the draft to say that we want to push in that direction seems to make sense to me
<emeyer> iank_: Yes, the direction, like, when I proposed this direction I didn’t see all the complications I now see
<emeyer> …If we want to go down the grid path, I’m not sure this path is workable
<emeyer> …I’m fine with editing the ED with a large scary note that details what’s being worked out
<emeyer> jensimmons: There’s an inconsistency between how this is being handled and how anchor positioning is being handled
<emeyer> …We have real objections about try blocks and they’re not being heard, but here similar objections are being taken seriously
<TabAtkins> We've already said that we'd like fantasai to continue iterating on this in the Grid 3 ED. Unsure what the complaint is.
<fantasai> proposal: Remove first-row-is-special handling of intrinsic track sizing in Masonry spec, replace with all auto-placed items contribute to all intrinsically sized tracks
<emeyer> astearns: Elika, can you state the proposed resolution?
<fantasai> Also: add a scary warning
<fantasai> :)
<emeyer> RESOLVED: Remove first-row-is-special handling of intrinsic track sizing in Masonry spec, replace with all auto-placed items contribute to all intrinsically sized tracks
<TabAtkins> I'm here ://///
<TabAtkins> Dang zoom

@bfgeek
Copy link
Author

bfgeek commented Sep 15, 2023

It's not quadratic. It's NxM where N is the number of items and M is the number of columns. And reduces to N (same as Grid, or IanK's proposal) if the columns have the same sizing function.

If tracks have different sizing functions then it goes quadratic however.

(edit: and I think potentially when subgrid is involved - gets super complex).

@ethanjv
Copy link

ethanjv commented Sep 15, 2023

that's a guaranteed quadratic algorithm

It's not quadratic. It's NxM where N is the number of items and M is the number of columns. And reduces to N (same as Grid, or IanK's proposal) if the columns have the same sizing function.

Well, computing the intrinsic contribution of grid items would be O(N) in such case, but if we completely rely on the grid track sizing algorithm as it is right now, then equally distributing such contribution to all the possible tracks it could be placed, which are all of them, makes it O(NxM) (...unless we change how the contribution of a grid item is accommodated for masonry).

@fantasai
Copy link
Collaborator

fantasai commented Oct 19, 2023

Merged in edits for #8206 (comment) per #8206 (comment)

@ethanjv
Copy link

ethanjv commented Nov 18, 2023

Btw we might want to consider that, in the current draft of masonry, having definite tracks mixed with intrinsic tracks might result in some items overflowing the spanning tracks in the grid axis. E.g., if we have a grid-template-columns: 50px auto definition, if we have two auto placed items, the first one with a min-content size of 100px and the other with 50px, the first will be placed in the first column and overflow it, then the second would be placed in the auto track.

Maybe we want to prioritize placing a grid item in a track that won't overflow?

@fantasai
Copy link
Collaborator

fantasai commented Mar 5, 2024

@ethanjv We currently don't prioritize item placement based on the resolved size of the tracks... if that's something you thin we should add, maybe open a new issue? :)

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 25, 2024
…izing tests for spec update, a=testonly

Automatic update from web-platform-tests
[css-grid-2][masonry] Update intrinsic-sizing tests for spec update

See w3c/csswg-drafts#8206

--

wpt-commits: 035560344cabe6a4e2777a592e498a65663ba932
wpt-pr: 45812
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue Apr 30, 2024
…izing tests for spec update, a=testonly

Automatic update from web-platform-tests
[css-grid-2][masonry] Update intrinsic-sizing tests for spec update

See w3c/csswg-drafts#8206

--

wpt-commits: 035560344cabe6a4e2777a592e498a65663ba932
wpt-pr: 45812
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
TPAC 2023 agenda
Friday Afternoon
Development

No branches or pull requests

4 participants