Closed Bug 1427608 Opened 6 years ago Closed 6 years ago

[css-grid] Intrinsic size of grid container with "minmax(auto, 0px)" is wrong

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: rego, Assigned: MatsPalmgren_bugz)

Details

Attachments

(6 files, 4 obsolete files)

Example to reproduce the issue
168 bytes, text/html
Details
Current vs expected output
7.06 KB, image/png
Details
Example with span=2
1.14 KB, text/html
Details
[css-grid] Fix span=1 'auto' min-sizing for intrinsic sizing
9.29 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
[css-grid] Adjust test expectations
77.36 KB, patch
Details | Diff | Splinter Review
Another testcase
833 bytes, text/html
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36

Steps to reproduce:

Open the attached example, it has an inline grid container with "grid-template-columns: minmax(auto, 0px);".



Actual results:


The item size is properly computed to 0px, however the grid container intrinsic size includes the size of the "foobar" item.


Expected results:


The grid container size should be 0px too, as the only column has 0px.

    
  
Component: Untriaged → Layout
Product: Firefox → Core

    
    

    
  

      
      
      
      

        Attached file
          
        Example with span=2 (obsolete)
        — 
      

    


  
Here's the same example, comparing the span=1 and span=2 cases.
I tend to think they should render the same, which Gecko does,
but not Chrome.

    
    

    
  
So what does the CSS specs say about your testcase?
The container is an inline-grid, so its size is the "shrink-wrap" size,
i.e.  min(max-content size, max(min-content size, stretch-fit size))
https://drafts.csswg.org/css-sizing-3/#fit-content-block-size
Assuming <body> is wide enough, it's the max-content size.
So, what's the grid container's max-content size?
Given that we have an 'auto' min-sizing function and we're sizing
under a max-content constraint, the spec says:
"For auto minimums: set the track’s base size to the maximum of its
items’ max-content contributions"
https://drafts.csswg.org/css-grid/#algo-single-span-items
So what's the item's max-content contribution?
It's the max-content size of its contents (the width of "foobar").

I'm guessing that you think that the clamping in 
"Automatic Minimum Size" (AMS) applies:
https://drafts.csswg.org/css-grid/#min-size-auto
Well, the item has min-width:auto and all the other conditions
for AMS are true and we also have a fixed max track sizing
function.  But, §6.6 starts:
"To provide a more reasonable default minimum size for grid items..."
so it's irrelevant when we ask for the item's max-content size.
The AMS is indeed zero (clamped by the column max-sizing value),
but as I see it, it only converts "min-width:auto" into "min-width:0",
which of course doesn't affect the item's max-content size at all.

So, the max-content contribution of the item is the size of its
text.  Which gives the container its size that you see in Firefox.

OK, now that we have the shrink-wrap size, why do does column
size then end up as zero?  Well, during layout we have neither
a min- or max-content constraint, so now the spec says:
"Otherwise, set its base size to the maximum of its items’ min-size
contributions [...], or else the item’s min-content contribution."
https://drafts.csswg.org/css-grid/#algo-single-span-items
In this case, we should do the "min-content contribution" part there,
so *now* the AMS applies and we set the track base size to zero here.

As far as I can tell, Gecko is doing what the CSS Grid spec says.

Can you explain how you got the rendering in Chrome please?
And why you get a different result in the span=2 case?
Flags: needinfo?(rego)

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #3)
> So what does the CSS specs say about your testcase?
> The container is an inline-grid, so its size is the "shrink-wrap" size,
> i.e.  min(max-content size, max(min-content size, stretch-fit size))
> https://drafts.csswg.org/css-sizing-3/#fit-content-block-size
> Assuming <body> is wide enough, it's the max-content size.
> So, what's the grid container's max-content size?
> Given that we have an 'auto' min-sizing function and we're sizing
> under a max-content constraint, the spec says:
> "For auto minimums: set the track’s base size to the maximum of its
> items’ max-content contributions"
> https://drafts.csswg.org/css-grid/#algo-single-span-items
> So what's the item's max-content contribution?
> It's the max-content size of its contents (the width of "foobar").
> 
> I'm guessing that you think that the clamping in 
> "Automatic Minimum Size" (AMS) applies:
> https://drafts.csswg.org/css-grid/#min-size-auto
> Well, the item has min-width:auto and all the other conditions
> for AMS are true and we also have a fixed max track sizing
> function.  But, §6.6 starts:
> "To provide a more reasonable default minimum size for grid items..."
> so it's irrelevant when we ask for the item's max-content size.
> The AMS is indeed zero (clamped by the column max-sizing value),
> but as I see it, it only converts "min-width:auto" into "min-width:0",
> which of course doesn't affect the item's max-content size at all.
> 
> So, the max-content contribution of the item is the size of its
> text.  Which gives the container its size that you see in Firefox.
> 
> OK, now that we have the shrink-wrap size, why do does column
> size then end up as zero?  Well, during layout we have neither
> a min- or max-content constraint, so now the spec says:
> "Otherwise, set its base size to the maximum of its items’ min-size
> contributions [...], or else the item’s min-content contribution."
> https://drafts.csswg.org/css-grid/#algo-single-span-items
> In this case, we should do the "min-content contribution" part there,
> so *now* the AMS applies and we set the track base size to zero here.
> 
> As far as I can tell, Gecko is doing what the CSS Grid spec says.
> 
> Can you explain how you got the rendering in Chrome please?

I didn't have time to debug the code, but what I get from the spec [1]:
  "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)."

If the track is 0px (or the tracks are 0px 0px in the 2nd case),
the intrinsic size should be the sum of the tracks.

BTW, in Edge the rendering is like in Chromium (for the 1 track case),
and the same output for the 2 tracks case (the spaning item).

> And why you get a different result in the span=2 case?

That's a bug, we only implemented the AMS clamping for non-spaning items:
https://bugs.chromium.org/p/chromium/issues/detail?id=809005

[1] https://drafts.csswg.org/css-grid/#intrinsic-sizes
Flags: needinfo?(rego)

    
    

    
  
(In reply to Manuel Rego Casasnovas from comment #4)
> I didn't have time to debug the code, but what I get from the spec [1]:
>   "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)."

Right, and that is *exactly* what we do to determine the container's
intrinsic size.  My elaboration above began with:
"So, what's the grid container's max-content size?"

The issue is that during the layout phase the Track Sizing Algorithm (TSA)
*isn't* run "under a max-content constraint (min-content constraint)"
so now TSA gives a different column size as a result.  We can't change
the container's intrinsic size in layout -- that happens in a separate
pass before layout starts.

I have a more specific question: do you apply the clamping in
§6.6 when calculating the item's max-content contribution?
and if so, what specific spec text do you think supports that?
Flags: needinfo?(rego)

    
    

    
  
BTW, I'm not opposed to changing our layout here.  On the contrary, I think
the Chrome layout probably makes more sense in this case.  I'm just trying
to pinpoint where exactly our interpretations of the spec are divergent.

    
    

    
  
Yeah, I'm also trying to understand the differences regarding the spec,
as I reported the bug as initially it made more sense Chromium layout here.
I've been reading the spec and probably we're not following it properly.
For example, we never use the item's max-content contribution to calculate the "auto minimums".
Anyway, I didn't have the proper time to investigate it, so I might need a few days
to check this properly and come back to you, sorry.

About your question we apply the clamping only to calculate the min-content contribution,
but as I said before we never use the max-content contribution in this case.

We have also 2 phases (like you):
1) Intrinsic size computation: Where we calculate the min and max preferred widths.
2) Layout.

For the intrinsic size phase, we use the track's base size to calculate the min preferred width
and the growth limit for max preferred width. 
It seems you're doing something different, are you running the algorithm twice
(under min-content and under max-content) or how are you doing it?

BTW, if you do a similar case for rows "grid-template-rows: minmax(auto, 0px)"
the height in Firefox is 0px, which seems inconsistent with the width result.
I know there's no intrinsic height step, so during layout you need to apply the clamping,
but there's always that generic goal of making grid layout as symmetric as possible.
Flags: needinfo?(rego)

    
    

    
  
(In reply to Manuel Rego Casasnovas from comment #7)
> For the intrinsic size phase, we use the track's base size
> to calculate the min preferred width and the growth limit
> for max preferred width. 

Interesting, but using the growth limits seems wrong to me.
I skeptic that it gives the same result as running the TSA under
a max-content constraint and summing up the base sizes.
The spec is clear about this IMO:
"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)."
https://drafts.csswg.org/css-grid/#intrinsic-sizes

> It seems you're doing something different, are you running
> the algorithm twice (under min-content and under max-content)
> or how are you doing it?

Yes, we run TSA twice in phase 1.  Once under a min-content
constraint and once under a max-content constraint, to calculate
the container's min/max-content size respectively.
We use the sum of the base sizes in both cases.

Those two sizes are then fed into the CSS2 shrink-wrap equation
that gives us the intrinsic size.  That result is then used as
the container's definite inline-size in the layout phase.

In the layout phase we run TSA a third time, but now it's neither
under min/max-content constraint.  This phase is what determines
the final track sizes for layout of the grid items.

> BTW, if you do a similar case for rows ...
> the height in Firefox is 0px, which seems inconsistent with
> the width result.

Yeah, the difference between inline/block direction comes from
the definition of the block axis min/max-content size:
"Usually the block size of the content after layout."
https://drafts.csswg.org/css-sizing-3/#auto-box-sizes

So we just treated that as a min-content size and clamped it,
which I agree is a bit inconsistent.

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #8)
> (In reply to Manuel Rego Casasnovas from comment #7)
> > For the intrinsic size phase, we use the track's base size
> > to calculate the min preferred width and the growth limit
> > for max preferred width. 
> 
> Interesting, but using the growth limits seems wrong to me.
> I skeptic that it gives the same result as running the TSA under
> a max-content constraint and summing up the base sizes.
> The spec is clear about this IMO:
> "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)."
> https://drafts.csswg.org/css-grid/#intrinsic-sizes

Yes that doesn't follow the spec, but it was somehow returning good results
at least we didn't detected this interop issue earlier.

I believe the reason why this is implemented like that in Blink/WebKit
is this old mail from Oct 2013:
https://lists.w3.org/Archives/Public/www-style/2013Oct/0581.html

Tab seems to agree on that thread with the proposed approach,
but the specification was never modified to reflect it.

> > It seems you're doing something different, are you running
> > the algorithm twice (under min-content and under max-content)
> > or how are you doing it?
> 
> Yes, we run TSA twice in phase 1.  Once under a min-content
> constraint and once under a max-content constraint, to calculate
> the container's min/max-content size respectively.
> We use the sum of the base sizes in both cases.
> 
> Those two sizes are then fed into the CSS2 shrink-wrap equation
> that gives us the intrinsic size.  That result is then used as
> the container's definite inline-size in the layout phase.
> 
> In the layout phase we run TSA a third time, but now it's neither
> under min/max-content constraint.  This phase is what determines
> the final track sizes for layout of the grid items.

Ok, thanks for the confirmation. I'll close this issue at this point
as I agree Firefox behavior is right according to the spec
(despite as you said the final result doesn't make a lot of sense).

I'll open an CSSWG issue explaining all this, and the differences
between Firefox and Blink/WebKit (and even Edge).
I'll try to look for more examples where we might have inconsistencies.

> > BTW, if you do a similar case for rows ...
> > the height in Firefox is 0px, which seems inconsistent with
> > the width result.
> 
> Yeah, the difference between inline/block direction comes from
> the definition of the block axis min/max-content size:
> "Usually the block size of the content after layout."
> https://drafts.csswg.org/css-sizing-3/#auto-box-sizes
> 
> So we just treated that as a min-content size and clamped it,
> which I agree is a bit inconsistent.

Yes, I understand that part, and I agree that's the only valid option
for height.

    
    

    
  
(In reply to Manuel Rego Casasnovas from comment #9)
> I believe the reason why this is implemented like that in Blink/WebKit
> is this old mail from Oct 2013:
> https://lists.w3.org/Archives/Public/www-style/2013Oct/0581.html
> 
> Tab seems to agree on that thread with the proposed approach,
> but the specification was never modified to reflect it.

What he said is that "we'll take care of it", which I read
as: he will address the reported problem ("there is no
definition for the intrinsic / preferred logical widths on
the grid element").  That doesn't imply he agreed to spec your
suggested implementation.

I think the current spec text for the container's intrinsic size
is actually the only reasonable definition.
Sizing the contents under a min(max)-content constraint is how
the intrinsic size is determined for *all* CSS boxes, so I feel
rather strongly that we should leave that spec text as is.

I guess you can keep your implementation of summing up the limit's
after sizing the container under a min-content constraint if you
think that always leads to the same result, but as I said, I'm
rather skeptical that it does.  I'd humbly suggest that you
size it under a max-content constraint and sum up the base sizes
instead, like we do.

> I'll open an CSSWG issue explaining all this, and the differences
> between Firefox and Blink/WebKit (and even Edge).
> I'll try to look for more examples where we might have inconsistencies.

Thanks, please CC me.  I think the solution here is to always
clamp the item's size regardless of it's under a min-content/
max-content/no constraint.  I've implemented that for Gecko
and it seems to lead to reasonable results, i.e. the container's
size is zero for the testcase you reported.

The only spec change needed for that is in the last paragraph of
§6.6 about the clamping.  It needs to point out that the clamping
always occurs, not just when calculating the "minimum size" for an
item, but also when calculating it's max-content size for example.
(All the required *conditions* for the clamping to occur should be
the same though, i.e. only for max-width/height:auto items that
span a track with auto min-sizing etc.)

> > So we just treated that as a min-content size and clamped it,
> > which I agree is a bit inconsistent.
> 
> Yes, I understand that part, and I agree that's the only valid option
> for height.

With the spec change I suggest above, clamping the block axis
like we do will be consistent actually, which is nice.

    
    

    
  
JFYI, the CSSWG issue has just been reported at: https://github.com/w3c/csswg-drafts/issues/2303

    
    

    
  

      
      
      
      

        Attached file
          
        Example with span=2
        — 
      

    


  

        Attachment #8948292 -
        Attachment is obsolete: true

    
    

    
  
[ Triage 2017/02/20: P3 ]
Priority: -- → P3

    
    

    
  


  
This makes us compatible with Chrome in all the span=1 cases I looked at,
which was quite a few.

Note that this implementation isn't what CSS Grid specifies,
but it's pretty clear that the spec is wrong, see:
https://github.com/w3c/csswg-drafts/issues/2303

It seems to work really well though, so I'll propose it as
the algo to use in the spec.
Assignee: nobody → mats
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

        Attachment #8953784 -
        Flags: review?(dholbert)

    
    

    
  


  
I'm punting on fixing up a few of the tests that contain percentage sizes
here, since I know I'll need to fix those in bug 1434478 soon anyway.
So I just marked them as "fails" here temporarily.

    
    

    
  
Comment on attachment 8953784 [details] [diff] [review]
[css-grid] Fix span=1 'auto' min-sizing for intrinsic sizing

Review of attachment 8953784 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine. A few optional observations/suggestions:

::: layout/generic/nsGridContainerFrame.cpp
@@ +615,5 @@
>                                nscoord aPercentageBasis) const
>    {
> +    const auto* pos = mFrame->IsTableWrapperFrame() ?
> +      mFrame->PrincipalChildList().FirstChild()->StylePosition() :
> +      mFrame->StylePosition();

(This table-related tweak feels like maybe it wants to happen in its own commit, and perhaps needs a testcase? It feels independent of the other changes here.)

@@ +620,4 @@
>      const auto& size = aContainerAxis == eLogicalAxisInline ?
>        pos->ISize(aContainerWM) : pos->BSize(aContainerWM);
> +    // NOTE: if we have a non-'auto' size then our automatic minimum size
> +    // can't affect our size.  Excluding these simplifies applying

I think:
s/If we have a non-'auto' size/If we have a definite size/?  (for correctness & clarity)

(Or: if the 'auto' keyword is really a relevant distinction to make here, can you elaborate on why?  Right now, the grid spec says the automatic minimum size may be the "specified size" [1], which is defined (in the flexbox spec) as only existing if the relevant sizing property is *definite*.[2]  There's no mention of whether the sizing property is 'auto'.)

(It looks like the code below this checks for "IsPercentOfIndefiniteSize" along with 'auto', which matches my intuition.)

[1] https://drafts.csswg.org/css-grid/#min-size-auto
[2] https://drafts.csswg.org/css-flexbox-1/#specified-size

@@ +628,5 @@
>          pos->MinISize(aContainerWM) : pos->MinBSize(aContainerWM);
>        return minSize.GetUnit() == eStyleUnit_Auto &&
>               mFrame->StyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE;
>      }
>      return false;

If you like: consider flipping the logic here, so that it more clearly matches your comment before the if-check. (and to save on indentation and to handle quick-and-easy special cases via early-return).

E.g.:
  // If we have a definite size...
  // ...excluding these simplifies stuff...
  if (size is definite) {
    return false;
  }
  const auto minSize = [etc];
  return minSize.[etc];

        Attachment #8953784 -
        Flags: review?(dholbert) → review+

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #16)
> (This table-related tweak feels like maybe it wants to happen in its own
> commit, and perhaps needs a testcase? It feels independent of the other
> changes here.)

Yeah, I thought about it, but I didn't think it was worth the time
to update grid-item-table-stretch-002-ref.html twice, so I didn't.
(That test now fails without this tweak.)
But yeah, I should've called it out as an existing bug.
(The issue being that styles like 'width', 'min-width' etc doesn't
inherit to the table wrapper frame.  I suspect we have this bug
in other places too...)

> s/If we have a non-'auto' size/If we have a definite size/?  (for
> correctness & clarity)

Yup, fixed.  The spec says "if its specified size (...) is 'auto'"
https://drafts.csswg.org/css-grid/#min-size-contribution
but I'm pretty sure what they mean is "behaves as auto".
https://drafts.csswg.org/css-sizing-3/#behave-auto

Filed spec issue: https://github.com/w3c/csswg-drafts/issues/2367

> If you like: consider flipping the logic here, so that it more clearly
> matches your comment before the if-check. (and to save on indentation and to
> handle quick-and-easy special cases via early-return).

Good suggestion; I did so.

    
    

    
  
> (The issue being that styles like 'width', 'min-width' etc doesn't
> inherit to the table wrapper frame.  I suspect we have this bug
> in other places too...)

Maybe it's an issue for flex items too?
https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/layout/generic/nsFlexContainerFrame.cpp#3281

    
    

    
  
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82bd129fd805
[css-grid] Fix span=1 'auto' min-sizing for intrinsic sizing.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9419024217d
[css-grid] Adjust test expectations.  r=dholbert

    
  
Flags: in-testsuite+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/82bd129fd805
https://hg.mozilla.org/mozilla-central/rev/f9419024217d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

    
    

    
  

      
      
      
      

        Attached file
          
        Another testcase (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        Another testcase (obsolete)
        — 
      

    


  

        Attachment #9105965 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Another testcase (obsolete)
        — 
      

    


  

        Attachment #9105967 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Another testcase
        — 
      

    


  

        Attachment #9105969 -
        Attachment is obsolete: true

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: