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] Track sizing algorithm and gutters #2201

Closed
mrego opened this Issue Jan 18, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@mrego
Member

mrego commented Jan 18, 2018

There's some text about this in gutters definition:

For the purpose of track sizing, each gutter is treated as an extra, empty track of the specified size.

And the track sizing algorithm has a note about gutters:

Note: Gutters are treated as empty fixed-size tracks for the purpose of the track sizing algorithm.

But they're not mentioned in the rest of the algorithm.

I believe the text in the spec is correct, but it has lead to confusion in 2 implementations (Blink/WebKit and Firefox), so probably it can be clarified to avoid this kind of mistakes.
The problem is detailed in the following Chromium bug, but basically if you have a content based grid container (e.g. a floated grid container for columns or one with height: auto for rows) both implementations were wrongly calculating the size of a flexible track.
The text on the spec says:

  1. Let leftover space be the space to fill minus the base sizes of the non-flexible grid tracks.

Implementations were subtracting non-flexible tracks, but forgetting about gutters (which should be considered non-flexible tracks too for this step.
@MatsPalmgren suggested I open this issue to try to clarify things if possible.

FWIW, Edge is right on this, and their behavior is the expected one.

@mrego mrego added the css-grid-1 label Jan 18, 2018

@MatsPalmgren

This comment has been minimized.

MatsPalmgren commented Jan 18, 2018

I think the fact that two independent implementations had the exact same bug is enough proof that the spec is misleading. I see several issues with the spec:
Let leftover space be the space to fill minus the base sizes of the non-flexible grid tracks.

  1. the use of "base sizes" -- I don't generally consider gutters to have base sizes (or limits)
  2. "grid tracks" in the quote above links to https://drafts.csswg.org/css-grid/#grid-track which explicitly defines the term as a separate entity from gutters
  3. the fact that the above quote talks about a subset of tracks distracts from the fact gutters between all the tracks the item spans should be subtracted

It might be easier to leave §11.7.1. "Find the Size of an 'fr'" as is though and instead fix it in §11.7. "Expand Flexible Tracks" by changing "space to fill of the available grid space" to "space to fill of the available grid space minus the size of all the gutters" (the definite case), and "space to fill of the item’s max-content contribution" to "space to fill of the item’s max-content contribution minus the size of the gutters it spans" (the indefinite case).

The spec should also point out that if "space to fill" is negative then the "hypothetical fr size" is zero.

@javifernandez

This comment has been minimized.

Contributor

javifernandez commented Jan 18, 2018

I totally agree with Mats here and also share his point of view of what the spec should state. In fact, our final implementation to fix this bug is precisely subtracting the gutters to the 'space to fill' value. We also assumed that if "space to fill" is negative, then the "hypothetical fr size" is zero. Hence, I think these changes make sense.

@fantasai

This comment has been minimized.

Contributor

fantasai commented Jan 18, 2018

My concern about integrating gutters explicitly into the track sizing algorithm is that some places will remember to integrate them and others won't, and then we'll have actual problems. The spec very explicitly defines that gutters behave exactly like fixed-size tracks for the purpose of the grid sizing algorithm, which avoids any such synchronization problem. I'm happy to add a note reminding the reader of this fact at the top of the grid sizing algorithm, but I do not want to introduce spurious errors by pretending that gutters are potentially not exactly equivalent to fixed-size grid tracks for the purpose of sizing.

the use of "base sizes" -- I don't generally consider gutters to have base sizes (or limits)

Gutters are defined exactly to behave as fixed-size tracks of the absolute gutter width. So of course they have base sizes and limits. As with any fixed-size track, these are defined to be the same.

@javifernandez

This comment has been minimized.

Contributor

javifernandez commented Jan 18, 2018

I understand the concerns. Actually, that is precisely what happened in two implementations, so both forgot to consider gutters as fixed-sized tracks for the purpose of computing the flex fraction. I guess both approaches have similar issues; perhaps that note at the beginning of the track sizing algorithm is a good compromise.

@svillar

This comment has been minimized.

svillar commented Jan 19, 2018

I understand @MatsPalmgren position from the implementor POV but I also share @fantasai concerns. I'm reluctant to include anything related to gutters in the track sizing algorithm. Actually that's what we have been doing in Blink/WebKit, substract the size of the gutters from the available space and then run the algorithm normally.

However, can we totally forget about gutters? Unfortunatelly not, as implementors we need to consider them in several places. Actually we're currently doing it without an explicit mention in the specs like [here] for example. IMO it's implementors work to detect where we need to consider gutters gutters from computations.

Trying to reach some consensus what do you think about changing the status of the note mentioned by @mrego above, and transform it into a proper paragraph for implementors explaining that the gutters should be treated as fixed size tracks and that they should be likely considered in any computation which involves computing the size of spanning items or the available space in general.

@mrego

This comment has been minimized.

Member

mrego commented Jan 19, 2018

I'm aligned with @fantasai and @svillar opinions.

For example in the text proposed by @MatsPalmgren:

"space to fill of the available grid space minus the size of all the gutters"

Then when you do:

  1. Let leftover space be the space to fill minus the base sizes of the non-flexible grid tracks.

If you consider that the gutters are fixed tracks, you'll be subtracting their size again, which would be wrong.

fantasai added a commit that referenced this issue Jan 27, 2018

[css-grid-1] Add note reminding implementers that gutters are treated…
… like fixed-size tracks, and need to be incorporated into track size calculations accordingly. #2201

fantasai added a commit that referenced this issue Jan 27, 2018

[css-grid-1] Clarify that gutters, which are treated as fixed-size tr…
…acks, are spanned by any items which span across their corresponding grid line. #2201
@fantasai

This comment has been minimized.

Contributor

fantasai commented Jan 27, 2018

OK, I've added a note to the Track Sizing section reminding everyone that they need to consider gutters as fixed-size tracks as defined in the Gutters section; and clarified the gutters section to note the relationship of the gutter to items which span across the line (which is, to span the gutter).

@MatsPalmgren @mrego @svillar @javifernandez Let me know if these changes are sufficient! Thanks!

@mrego

This comment has been minimized.

Member

mrego commented Feb 5, 2018

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment