New issue
Advanced search Search tips

Issue 648814 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 79180



Sign in to add a comment

[css-grid] wrong number of repeat(auto-fill) rows with grid-auto-flow:column

Project Member Reported by mpalmg...@mozilla.com, Sep 21 2016

Issue description

Version: Version 55.0.2859.0 dev (64-bit)
OS: Linux

What steps will reproduce the problem?
(1) load https://people.mozilla.org/~mpalmgren/tests/chrome-bug-intrinsic-repeat-row.html
(2)
(3)

What is the expected output?
https://people.mozilla.org/~mpalmgren/tests/chrome-bug-intrinsic-repeat-row.png
(this screenshot is from my local build of Firefox where I've just fix this, it should be in Nightly in a few days)

What do you see instead?
Only two rows, causing the blue item to be placed in the second column.

Please use labels and text to provide additional information.

 

Comment 1 by svil...@igalia.com, Sep 29 2016

That's an interesting example. I think FF is producing a bit more sensible results but I think we have strictly followed what the specs say. It mentions: "if the grid container has a definite size or max size in the relevant axis, then the number of repetitions is the largest possible positive integer that does not cause the grid to overflow its grid container"

So as there is a definite max size (50px) we use it to compute just 2 repetitions. According to the specs the definite min size should only be used if there is no definite size and max size (note that it says "Otherwise".
> According to the specs the definite min size should only be used
> if there is no definite size and max size (note that it says "Otherwise"

Not exactly.  I agree it says that you should use the min size for
calculating the number of tracks when the size and max size are
indefinite.  But, it does NOT say anything about how the min size
should be used when a size or max size is also specified.

In the absence of anything explicit, I think it's reasonable to assume
that the min size trumps the used value of size/max size like it
always does, per CSS 2.2:
https://www.w3.org/TR/CSS22/visudet.html#min-max-widths
"The following algorithm describes how the two properties influence
the used value of the 'width' property: ..."

BTW, Chrome already applies the 'min-height' value when a 'height'
is specified. The bug only occurs for 'max-height', but 'min-height'
should trump that too per CSS 2.2.

Here's the 'height' + 'min-height' test, which works as expected.
chrome-bug-intrinsic-repeat-row2.html
1.0 KB View Download
Components: Blink>Layout>Grid
Labels: Hotlist-Interop

Comment 5 by drott@chromium.org, Oct 10 2016

Owner: svillar@chromium.org
Status: Assigned (was: Untriaged)
Would you take this one, svillar@ and work out a clarification of the spec or WontFix this issue? Otherwise perhaps set to Available, thanks.

Comment 6 by r...@igalia.com, Oct 12 2016

Blocking: 79180
Cc: jfernan...@igalia.com r...@igalia.com
Owner: svil...@igalia.com

Comment 7 by svil...@igalia.com, Oct 13 2016

Regarding what Mats mentions in https://bugs.chromium.org/p/chromium/issues/detail?id=648814#c2 then this means that both Blink and FF are doing it wrong when having a definite size and a definite min-size.

Check http://jsbin.com/reduhitira/edit?html,css,output

In the second grid both engines are computing 3 repetitions because the width is 80px, but according to your reasoning they should be computing 4 (because min-width is 100px)
> In the second grid both engines are computing 3 repetitions because the width
> is 80px, but according to your reasoning they should be computing 4 (because
> min-width is 100px)

No, it should be 3 repetitions in that case.  The spec is very clear that when
the size or max size is definite then we should only fill as many repetitions
that fits *within* that size:
https://drafts.csswg.org/css-grid/#auto-repeat
"if the grid container has a definite size or max size in the relevant axis, then the number of repetitions is the largest possible positive integer that does not cause the grid to overflow..."

It's only when *both* the size and max size are indefinite that we should use
the 2nd part of the algo that says "Otherwise, if the grid container has a
definite min size in the relevant axis, the number of repetitions is the smallest
possible positive integer that fulfills that minimum requirement.".

You seem to argue that when min-width "wins" over a specified width/max-width
we should use the second part of the algo (for min size), but there is no spec
text to support that as far as I can see.  Please note that the second part
starts with a very clear "Otherwise".

Comment 9 by svil...@igalia.com, Oct 13 2016

Regarding the example, yeah it's clearly 3, I somehow mixed some things up there.

Regarding the second thing, I think I am not understanding you. I said in comment 2 
"So as there is a definite max size (50px) we use it to compute just 2 repetitions. According to the specs the definite min size should only be used if there is no definite size and max size (note that it says "Otherwise"."

I think you're saying exactly the same in comment 8, something that is really confusing to me because it's totally the opposite to what you have said in comment 2 unless I'm missing something.

Comment 10 Deleted

Comment 11 by svil...@igalia.com, Oct 13 2016

I replied too fast, I think I didn't mix things up. I think that it can never be 3, actually it should be either 2 or 4 depending on how the specs are interpreted.

There are 2 possibilities:
a) min-size is used always (as you defend on comment 2): then the number of repetitions must be 4, because even if width:80px you must obey min-width:100px, then you need at least 4 repetitions to fulfill it.

b) min-size is used only if size and max-size are indefinite (as I defend, and as you seem to suggest in comment 8 contradicting comment 2?): then the number of repetitions is 2, because we're using 80px and any extra repetition will overflow.
I think you're confused by CSS specified values and used values.
When you have "width: 80px; min-width: 100px;" the _used width_
is 100px (per CSS2.1 as I explained in comment 2).
This is the value we should use (as "size" in part 1 of the algo)
when calculating the number of repeat tracks.

When the size or max size is definite, (i.e. not 'auto'/'none'
respectively, and not a percentage of an indefinite percent basis)
then the value to use in part 1 of the repeat algorithm is the
'used value' (as defined per CSS2.1), and that includes applying
the computed 'min-width' to calculate that used value.

That doesn't imply that just because a 'min-width' was specified
we should use part 2 of the algorithm.  The spec clearly does
not want that since part 2 starts with "Otherwise".

Comment 13 by svil...@igalia.com, Oct 13 2016

OK I got your point, I was confused by comment 2, but I guess we're on the same page now. I thought that you were defending using the min-size unconditionally but now I understand what you meant.

Fixing it...
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f030c73af4b8b82752c373cc98ef60a7e87a780

commit 8f030c73af4b8b82752c373cc98ef60a7e87a780
Author: svillar <svillar@igalia.com>
Date: Fri Oct 14 15:24:54 2016

[css-grid] Constrain by min-height on auto-repeat computation

The max-height (if definite) is used to compute the number of auto repeat
rows whenever the height is indefinite. We were using the min-height only in
case both values were indefinite.

However, it's reasonable to assume that the min-height trumps the used value
of height/max-height like it always does, per CSS 2.2. Note that the number
of rows still needs to fit within that size even if using min-height,
because we're just using min-height to compute the used value of the height
property. If both height and max-height are indefinite we keep doing the
same, i.e., compute the minimum number of rows that fulfill min-height (if
definite).

BUG= 648814 

Review-Url: https://codereview.chromium.org/2421593002
Cr-Commit-Position: refs/heads/master@{#425327}

[modify] https://crrev.com/8f030c73af4b8b82752c373cc98ef60a7e87a780/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-rows.html
[modify] https://crrev.com/8f030c73af4b8b82752c373cc98ef60a7e87a780/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash-expected.txt
[modify] https://crrev.com/8f030c73af4b8b82752c373cc98ef60a7e87a780/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html
[modify] https://crrev.com/8f030c73af4b8b82752c373cc98ef60a7e87a780/third_party/WebKit/Source/core/layout/LayoutGrid.cpp

Comment 15 by svil...@igalia.com, Oct 14 2016

Status: Fixed (was: Assigned)
Closing

Sign in to add a comment