Manuel Rego Casasnovas Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.66 Safari/537.36

Steps to reproduce:

Open the attached example, the first grid container has "position: absoulte;" so its size is indefinite.




Actual results:


The first grid use percentage tracks and they're treated as "0px".

The second grid use "auto" tracks.


Expected results:


The first grid should render the same than the second one, as percentage tracks should be treated as "auto" per spec (https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-percentage):
"If the inline or block size of the grid container is indefinite, <percentage> values relative to that size are treated as auto."

	Mats Palmgren (inactive) Assignee
	Comment 3 • 8 years ago

Thanks for the report.

	Mats Palmgren (inactive) Assignee
	Comment 4 • 8 years ago

It looks like Chrome has a few bugs here as well...

	Mats Palmgren (inactive) Assignee
	Comment 6 • 8 years ago

Rego, it seems like Chrome has three bugs: 
1. testcase 2 have identical column/row sizing, yet the rows have intrinsic sizing
   in all cases, whereas the columns does not. (row sizes are wrong)
2. 0% should never be treated as 'auto' - it's always treated as zero.
   https://drafts.csswg.org/css-values-3/#lengths
   "However, for zero lengths the unit identifier is optional
   (i.e. can be syntactically represented as the <number> 0)"
   which implies that 0% is identical to 0
3. calc(N%) should be treated exactly as N%

	Manuel Rego Casasnovas Reporter
	Comment 7 • 8 years ago

Hi Maths, thank you very much for reporting those issues.

(In reply to Mats Palmgren (:mats) from comment #6)
> 1. testcase 2 have identical column/row sizing, yet the rows have intrinsic
> sizing
>    in all cases, whereas the columns does not. (row sizes are wrong)

I think the problem is that width is wrong,
at least in the first example "0" from your test case.
The width is 0px right now in Blink, but it should be treated as "auto".
The problem is that Blink is not considering as indefinite width
the floated grid container.
I've reported the issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=603854 

> 2. 0% should never be treated as 'auto' - it's always treated as zero.
>    https://drafts.csswg.org/css-values-3/#lengths
>    "However, for zero lengths the unit identifier is optional
>    (i.e. can be syntactically represented as the <number> 0)"
>    which implies that 0% is identical to 0

I'm not 100% sure about this one, note that <percentage> section
of the spec (https://drafts.csswg.org/css-values-3/#percentages)
doesn't say anything about zero:
"When written literally, a percentage consists of a number
 immediately followed by a percent sign %."

On top of that in a regular block if we use "height: 0%"
it's not treated as "height: 0px", but treaed as "auto".
This is from the CSS 2 spec (https://www.w3.org/TR/CSS2/visudet.html#propdef-height):
"If the height of the containing block is not specified explicitly
 (i.e., it depends on content height), and this element
 is not absolutely positioned, the value computes to 'auto'."

See the following example comparing the different results:
http://jsbin.com/xojobix/1/edit?html,css,output

> 3. calc(N%) should be treated exactly as N%

In the previous example I added a case of calc()
("height: calc(50% + 20px);") and it's still working as "auto".

I don't really know if it should behave differently for grid layout.

	Mats Palmgren (inactive) Assignee
	Comment 8 • 8 years ago

Fair enough, compatibility with block 'height' seems like the way to go.
So, a track size that contains a '%' anywhere behave as 'auto' when
the percentage basis is indefinite.

	Mats Palmgren (inactive) Assignee
	Comment 9 • 8 years ago

Regarding float sizing, it's a bit more complicated than I first thought.
The containing block width is only considered indefinite during intrinsic
sizing.  Then it's flowed with that size as a definite size.

Here are some float display:block examples to illustrate.
Firefox and Chrome have identical rendering here.

	Mats Palmgren (inactive) Assignee
	Comment 10 • 8 years ago

Here's the corresponding float display:grid test.
If I fix our grid layout per comment 8, then this example renders
identically to the block example above.  This feels intuitively
right to me.  (It's far from what Chrome is doing though.)

	Mats Palmgren (inactive) Assignee
	Comment 12 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf1fece3fb4f

	Daniel Holbert [:dholbert]
	Comment 13 • 8 years ago

Comment on attachment 8741932 [details] [diff] [review]
fix

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +157,5 @@
> +  auto maxSizeUnit = aMaxCoord.GetUnit();
> +  if (percentIsAuto) {
> +    // https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-percentage
> +    // "If the inline or block size of the grid container is indefinite,
> +    //  <percentage> values relative to that size are treated as auto."

Perhaps-too-extreme nit:  IMO we should just drop "percentIsAuto" & fold it directly into this "if" check.

Reasons:
 (1) it's only used once, so we're not saving anything by caching it in a local variable.
 (2) You've got this nice explanatory documentation about "If the size is indefinite", which is great, but it's a little confusing because it's ~5 lines after we've already checked for whether the size is indefinite (so it's not alongside the check that it's actually documenting.)  But if you folded this variable into the "if" condition, then the comment *would* be adjacent to the check that it's documenting (the NS_UNCONSTRAINEDSIZE comparison).  IMO this would make things clearer.

Not a big deal, though; feel free to disregard & keep this as-is.

	Mats Palmgren (inactive) Assignee
	Comment 14 • 8 years ago

Sure, no problem.

	Mats Palmgren (inactive) Assignee
	Comment 15 • 8 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb17b758fee2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6fbfa5372b24

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cb17b758fee2
https://hg.mozilla.org/mozilla-central/rev/6fbfa5372b24

	Manuel Rego Casasnovas Reporter
	Comment 17 • 8 years ago

(In reply to Mats Palmgren (:mats) from comment #9)
> Created attachment 8741898 [details]
> float block test
> 
> Regarding float sizing, it's a bit more complicated than I first thought.
> The containing block width is only considered indefinite during intrinsic
> sizing.  Then it's flowed with that size as a definite size.
> 
> Here are some float display:block examples to illustrate.
> Firefox and Chrome have identical rendering here.

I was trying to understand this, but I'm not sure if I'm missing something.

I've created a very simple example (attached) of a grid with "grid: 50% / 50%;" vs "grid: 200% / 200%;"
and I've made the grid container positioned and floated.
For the 50% case, both positioned and floated behave like "auto".
For the 200% case, none of them behave like "auto".

Is this the expected behavior?
At least for the positioned grid container I think
it should be considered a "indefinite" width and be treated as "auto".

You can check the example live in the following jsbin:
http://jsbin.com/demeda/1/edit?html,css,output

	Mats Palmgren (inactive) Assignee
	Comment 18 • 8 years ago

> Is this the expected behavior?

AFAICT, our rendering in the latest Nightly is correct for that test.

Both the 50% case and the 200% case behave the same actually.
I think you might be confused about the column size for the 50% case.
Here's an updated test that shows that the column size is 50% of
the container size (the added "." items stretch to 50% width):
http://jsbin.com/qelegagixa/edit?html,css,output

> At least for the positioned grid container I think
> it should be considered a "indefinite" width and be treated as "auto".

The intrinsic size of the grid containers in these tests are
the result of treating the percentage track sizes as 'auto'.
Then when we flow the container the percentages are resolved
using that intrinsic size.  This is how float / abs.pos.
intrinsic sizing normally works.  Why should Grid be different?

I guess you're arguing that the percentage should be treated
as 'auto' also when flowing, but that would remove a useful
feature IMO - i.e. to size a track as a percent of the intrinsic
size of the container.  We have that feature for float blocks,
why shouldn't we have it for float grids?

The spec is fairly clear that the grid container intrinsic
sizing should work the same as for blocks:
https://drafts.csswg.org/css-grid/#intrinsic-sizes
I think that implies that percentages of that intrinsic size
should work the same too.  I'm aware that the spec says that
percentages of indefinite sizes should be treated as 'auto',
but an intrinsic size is *definite* once it is resolved IMO.

	Manuel Rego Casasnovas Reporter
	Comment 19 • 8 years ago

(In reply to Mats Palmgren (:mats) from comment #18)
> The intrinsic size of the grid containers in these tests are
> the result of treating the percentage track sizes as 'auto'.
> Then when we flow the container the percentages are resolved
> using that intrinsic size.  This is how float / abs.pos.
> intrinsic sizing normally works.  Why should Grid be different?

Sorry for the back and forth but I was not understanding this properly.

Thanks for the detailed explanation, I'll be fixing Blink and WebKit
to behave as Firefox for this case too.
The idea is:
* Width is only indefinite while computing intrinsic sizes
  (so we use "auto" for percentage tracks in that phase).
* Once we've the intrinsic sizes of the grid container,
  we resolve the percentage tracks against them.