1341507 - [css-grid] grid-template-rows / grid-template-columns does not recognise multiple values within repeat() notation when used with auto-fill

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Chen Hui Jing]

Attachment: test.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3013.3 Safari/537.36

Steps to reproduce:

Create grid container with the following properties:
.grid {
  display: grid;
  grid-template-columns: repeat(auto-fill, 50px 25px);
  grid-auto-rows: 50px;
}


Actual results:

Firefox indicated that the value for grid-template-columns was invalid.


Expected results:

A grid with alternating columns of 50px and 25px should have been generated and fill up the available space before starting on the next row.

Comment 1

[Roman Komarov]

Wanted to mention my use case for this: https://codepen.io/kizu/pen/paayMm?editors=1100:

    grid-template-columns: repeat(auto-fill, minmax(64px, 1fr) minmax(64px, 1fr));

This can be really useful when you need to have an even number of columns which take all the available space. Right now this works perfectly in Edge, has bugs in webkit/blink when used with `grid-gap`, and don't work at all in Firefox. I don't think there are any other ways to work around this, so having this feature would be really nice!

Comment 3

[Mike Moening]

This works in Chrome.  Any hope for a fix soon??

Comment 4

[Phil McCloghry-Laing]

I'm interested in working on a fix for this.

The approach I'm looking at is replacing the RepeatAutoIndex with RepeatAutoStartIndex and RepeatAutoEndIndex. It means that some of items in the LineNameLists can be for the repeat track list (where RepeatAutoStartIndex < index < RepeatAutoEndIndex), similar to the Min/MaxTrackSizingFunctions.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

First step for this is probably keeping these values around for layout. Bug 1519958 is about that.

Comment 6

[Mats Palmgren (inactive)]

The fix here would probably be quite similar the one in bug 1339672, if that helps.

Comment 7

[Boris Chiou [:boris]]

(In reply to Phil McCloghry-Laing from comment #4)

I'm interested in working on a fix for this.

The approach I'm looking at is replacing the RepeatAutoIndex with
RepeatAutoStartIndex and RepeatAutoEndIndex. It means that some of items in
the LineNameLists can be for the repeat track list (where
RepeatAutoStartIndex < index < RepeatAutoEndIndex), similar to the
Min/MaxTrackSizingFunctions.

Hi Phil,
Do you still have interest in this? We also have to update the parser of TrackRepeat (which was wroten in Rust), and update something like ExpandNonRepeatAutoTracks() and others if needed. Thanks.

Comment 8

[Phil McCloghry-Laing]

Hi (In reply to Boris Chiou [:boris] from comment #7)

(In reply to Phil McCloghry-Laing from comment #4)

I'm interested in working on a fix for this.

The approach I'm looking at is replacing the RepeatAutoIndex with
RepeatAutoStartIndex and RepeatAutoEndIndex. It means that some of items in
the LineNameLists can be for the repeat track list (where
RepeatAutoStartIndex < index < RepeatAutoEndIndex), similar to the
Min/MaxTrackSizingFunctions.

Hi Phil,
Do you still have interest in this? We also have to update the parser of TrackRepeat (which was wroten in Rust), and update something like ExpandNonRepeatAutoTracks() and others if needed. Thanks.

I'm still interested in doing this. I did start working on it, including the parsing in rust. I haven't worked on any Mozilla code previously - is someone available to review changes and what's the best way to submit them?

Comment 9

[Boris Chiou [:boris]]

(In reply to Phil McCloghry-Laing from comment #8)

I'm still interested in doing this. I did start working on it, including the parsing in rust. I haven't worked on any Mozilla code previously - is someone available to review changes and what's the best way to submit them?

Great. You will update grid code and part of Rust parser, so you could send the review request to Mats. I think the best way is to use Phabricator for sending patches and reviewing them. There is a document about how to set up: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html.

Once you set it up, you can use git or hg to push your patches into Phabricator, and Bugzilla will display the link automatically. Send the review request in Phabricator, and then I believe the reviewer will handle the rest part.

Comment 10

[Mats Palmgren (inactive)]

FTR, we should also fix repeat(auto-fill, <line-names>+) in subgridvalues:
https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/servo/components/style/values/generics/grid.rs#l662
which can have names for multiple lines per:
https://drafts.csswg.org/css-grid-2/#subgrid-per-axis
https://drafts.csswg.org/css-grid-2/#typedef-name-repeat

Comment 12

[Sean Voisen (:svoisen)]

Hi Phil — Just wanted to check in to see if you are still working on this?

Comment 13

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 - Enable multiple CSS grid repeat auto-fill values in servo and use the correct repeat auto-fill value when calculating grid sizing

Enable several reftests which expected failure because of this
Update a mochitest which seems to be expecting earlier CSS draft behavior.

Comment 14

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 - Refactor auto-fit empty track calculations into a separate method

Comment 15

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 - Refactor TrackSizingFunctions::mRepeatEndDelta to be a getter function rather than a variable.

The information in it is always derivable from the values of mRepeatAutoStart
and mRepeatAutoEnd. Additionally, its value is used in some cases where it has
not yet been set properly (such as CalculateRepeatFillCount).

This works out currently because the default value is zero we only accept
repeat(auto-fill, ...) and repeat(auto-fit, ...) CSS values that have a single
element in the repeat. In that case, zero is the correct value for
RepeatEndDelta.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5245d585b9a
Refactor TrackSizingFunctions::mRepeatEndDelta to be a getter function rather than a variable. r=emilio

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/e5245d585b9a

Comment 18

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 part 1 - Refactor computed DOM for grid layout to make it simpler for handling repeat values, and to handle multiple repeat values.

Comment 19

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 - Add support for repeat tracks with more than value to computed dom style.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a3321b64a58
Refactor auto-fit empty track calculations into a separate method r=mats

Comment 21

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4a3321b64a58

Comment 22

[Mats Palmgren (inactive)]

I'm a bit confused about this patch set. D55725 makes a bunch of changes to nsComputedDOMStyle.cpp which appears to be very similar to the changes in D59547. Could you rebase the patch set on current m-c and tell me which order I should apply these patches please?

(FYI, it's common practice to add "part 1" "part 2" etc to the patches to indicate the order and also to highlight in the commit log that a patch is part of a series of related changes.)

(Also, a minor typo in the last patch summary: s/more than value/more than one value/)

Comment 23

[Emily McDonough [:alaskanemily]]

Sorry about that, I should have made what I was trying to do more clear.

I'm trying to break some of D55725 into smaller parts, and also make it so that it's easier to just "flip the switch" in servo when everything else is ready.

That's the idea behind D59070 and D59547. Those changes are supposed to be safe to make even if we aren't getting multiple repeat values from CSS yet.

I will also mark those as "part 1"/"part 2" etc.

Comment 24

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 part 2 - Take multiple repeat track sizes into account when computing how many repetitions will fit.

This also means that the result of CalculateRepeatFillCount is specified to be
a number of repetitions of all repeat tracks, not the total number of tracks.

Comment 25

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 part 3 - Support multiple repeat values when getting the sizing of a track by index.

Comment 26

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 part 4 - Add auto-fill length field to line name lists returned from Servo.

The test to ensure only a single repeat value is allowed will be removed by a later commit.

Comment 27

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 part 5 - Support multiple tracks in repeat-auto in line name maps

This includes when using subgrid layout.

Comment 28

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 part 6 - Enable multiple grid repeat values in Servo

To complete support, also finish the TODO about this in nsGridContainerFrame
when calculating track sizes and properly enumerate repeat values in the
computed dom.

Comment 29

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1341507 part 7 - Update mochitests and wpt for supporting repeat-auto with multiple values in grid and subgrid.

Comment 30

[Mats Palmgren (inactive)]

Attachment: Testcase #2

After applying all the patches, I see the following results in the devtools Console:

[x a b] 50px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 50px [b a b] 0px [d e y]
[x a b] 0px [b a b] 0px [b a b] 50px [d e y]
[x a b] 0px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 0px [b a b] 0px [d e y]
[x a b] 50px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 50px [b a b] 0px [d e y]
[x a b] 0px [b a b] 0px [b a b] 50px [d e y]
[x a b] 0px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 0px [b a b] 0px [d e y]
[x a b] 50px [b a b] 0px [b a b] 0px [b a b] 0px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 50px [b a b] 0px [b a b] 0px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 0px [b a b] 50px [b a b] 0px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 0px [b a b] 0px [b a b] 50px [b a b] 0px [b a b] 0px [d e y]
[x a b] 0px [b a b] 0px [b a b] 0px [b a b] 0px [b a b] 50px [b a b] 0px [d e y]

whereas Chrome Canary says:

[x a b] 50px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 100px [c] 0px [d e y]
[x a b] 0px [b] 0px [c] 150px [d e y]
[x a b] 0px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 0px [c] 0px [d e y]
[x a b] 50px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 100px [c] 0px [d e y]
[x a b] 0px [b] 0px [c] 150px [d e y]
[x a b] 0px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 0px [c] 0px [d e y]
[x a b] 50px [b] 0px [c] 0px [d e a b] 0px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 100px [c] 0px [d e a b] 0px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 0px [c] 150px [d e a b] 0px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 0px [c] 0px [d e a b] 50px [b] 0px [c] 0px [d e y]
[x a b] 0px [b] 0px [c] 0px [d e a b] 0px [b] 100px [c] 0px [d e y]

Chrome's results looks correct to me.

Comment 31

[Mats Palmgren (inactive)]

Attachment: repeat-auto-fill-008.html.diff

So I added a few tests to testing/web-platform/tests/css/css-grid/subgrid/repeat-auto-fill-008.html and I'm not sure I agree with the current results. A repeat(auto-fill) with multiple values in the non-subgrid case only repeats if all the values fit - shouldn't subgrid do the same?
That's my understanding of the spec text here, fwiw:
https://drafts.csswg.org/css-grid-2/#typedef-line-name-list

For example, given a subgrid that spans four tracks and has subgrid repeat(auto-fill, [x] [y]) [z] [z], shouldn't the used value be subgrid [x] [y] [z] [z] since we can only repeat all the auto-fill names once? (repeating it once more: subgrid [x] [y] [x] [y] [z] [z] is clearly invalid since that implies we have five tracks, and the value I get with these patches subgrid [x] [y] [x] [z] [z] also seems invalid since the 2nd repetition isn't complete)

Comment 32

[Emily McDonough [:alaskanemily]]

You are right, I am looking at fixing that now. I think the problem is because the constructor for LineNameMap when using subgrid doesn't ensure the difference between mRepeatStart and mRepeatEnd is a multiple of the repeat's length, although I'm not entirely certain that will be sufficient. If you have a better solution (or don't think that's the right place to handle that) let me know.

I will update the tests for bug 1613725 to include those cases.

Comment 33

[Emily McDonough [:alaskanemily]]

Actually, for some of those cases I see you have fewer than the span of tracks in the expectations. Shouldn't the remaining tracks be filled with empty name lists?

Comment 34

[Mats Palmgren (inactive)]

Oh right, I forgot that we fill those out for subgrid.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b35af9ecb38
part 1 - Refactor computed DOM for grid layout to make it simpler for handling repeat values, and to handle multiple repeat values. r=mats
https://hg.mozilla.org/integration/autoland/rev/08d38f05b160
part 2 - Take multiple repeat track sizes into account when computing how many repetitions will fit. r=mats
https://hg.mozilla.org/integration/autoland/rev/e798ebf91eca
part 3 - Support multiple repeat values when getting the sizing of a track by index. r=mats
https://hg.mozilla.org/integration/autoland/rev/55432ee0cd4b
part 4 - Add auto-fill length field to line name lists returned from Servo. r=mats,emilio
https://hg.mozilla.org/integration/autoland/rev/eff4ad47440c
part 5 - Support multiple tracks in repeat-auto in line name maps r=mats
https://hg.mozilla.org/integration/autoland/rev/6cafdef7eb79
part 6 - Enable multiple grid repeat values in Servo r=emilio
https://hg.mozilla.org/integration/autoland/rev/e4e968fabe2b
part 7 - Update mochitests and wpt for supporting repeat-auto with multiple values in grid and subgrid. r=mats

Comment 36

[Stefan Hindli [:stefan_hindli]]

Backed out 7 changesets (Bug 1341507) for mochitest failures in dom/grid/test/chrome/test_grid_repeat_auto_fill.html CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=b50418331e042dc2e82a59bf0384d5b6eada886a&selectedJob=293788324
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=293788324&repo=autoland&lineNumber=2014

Comment 37

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/098d364dfd2c
Backed out 7 changesets for mochitest failures in dom/grid/test/chrome/test_grid_repeat_auto_fill.html CLOSED TREE

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac09f1b654eb
part 1 - Refactor computed DOM for grid layout to make it simpler for handling repeat values, and to handle multiple repeat values. r=mats
https://hg.mozilla.org/integration/autoland/rev/3af2a45f142f
part 2 - Take multiple repeat track sizes into account when computing how many repetitions will fit. r=mats
https://hg.mozilla.org/integration/autoland/rev/7f70bf534b1f
part 3 - Support multiple repeat values when getting the sizing of a track by index. r=mats
https://hg.mozilla.org/integration/autoland/rev/8c1c830a9b51
part 4 - Add auto-fill length field to line name lists returned from Servo. r=mats,emilio
https://hg.mozilla.org/integration/autoland/rev/3d7db672d69e
part 5 - Support multiple tracks in repeat-auto in line name maps r=mats
https://hg.mozilla.org/integration/autoland/rev/3d385f974d44
part 6 - Enable multiple grid repeat values in Servo r=emilio
https://hg.mozilla.org/integration/autoland/rev/d52f097a39dc
part 7 - Update mochitests and wpt for supporting repeat-auto with multiple values in grid and subgrid. r=mats

Comment 39

[Emily McDonough [:alaskanemily]]

Should be fixed now, I hadn't realized that there were even grid tests in dom.

Comment 40

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ac09f1b654eb
https://hg.mozilla.org/mozilla-central/rev/3af2a45f142f
https://hg.mozilla.org/mozilla-central/rev/7f70bf534b1f
https://hg.mozilla.org/mozilla-central/rev/8c1c830a9b51
https://hg.mozilla.org/mozilla-central/rev/3d7db672d69e
https://hg.mozilla.org/mozilla-central/rev/3d385f974d44
https://hg.mozilla.org/mozilla-central/rev/d52f097a39dc