[css-grid] Percentage tracks should be treated as auto if grid container size is indefinite

RESOLVED FIXED in Firefox 48

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Manuel Rego Casasnovas, Assigned: mats)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla48
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8741327 [details]
indefinite-size-percentage-tracks.html

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."
(Reporter)

Comment 1

2 years ago
Created attachment 8741328 [details]
Current output
(Reporter)

Comment 2

2 years ago
Created attachment 8741329 [details]
Expected output
(Reporter)

Updated

2 years ago
Blocks: 616605

Updated

2 years ago
Component: Untriaged → Layout
Product: Firefox → Core
(Assignee)

Comment 3

2 years ago
Thanks for the report.
Assignee: nobody → mats
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
(Assignee)

Comment 4

2 years ago
Created attachment 8741527 [details]
testcase 2

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

Comment 5

2 years ago
Created attachment 8741530 [details]
Expected rendering for testcase 2
(Assignee)

Comment 6

2 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%
(Reporter)

Comment 7

2 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.
(Assignee)

Comment 8

2 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.
(Assignee)

Comment 9

2 years ago
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.
Attachment #8741527 - Attachment is obsolete: true
Attachment #8741530 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Created attachment 8741899 [details]
float grid test

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.)
(Assignee)

Comment 11

2 years ago
Created attachment 8741932 [details] [diff] [review]
fix
Attachment #8741932 - Flags: review?(dholbert)
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.
Attachment #8741932 - Flags: review?(dholbert) → review+
(Assignee)

Comment 14

2 years ago
Sure, no problem.

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cb17b758fee2
https://hg.mozilla.org/mozilla-central/rev/6fbfa5372b24
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Reporter)

Comment 17

2 years ago
Created attachment 8742493 [details]
Positioned and floated grid container with percentage tracks

(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
(Assignee)

Comment 18

2 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.
(Reporter)

Comment 19

2 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.
You need to log in before you can comment on or make changes to this bug.