Closed Bug 1341507 Opened 4 years ago Closed 5 months ago

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

Categories

(Core :: Layout, defect, P3)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla76

People

(Reporter: huijing, Assigned: alaskanemily, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, DevAdvocacy, Whiteboard: [DevRel:P1][layout:backlog:quality][layout:backlog:2020q1])

Attachments

(12 files, 2 obsolete files)

1.54 KB, text/html
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
1.43 KB, text/html
Details
6.86 KB, patch
Details | Diff | Splinter Review
Attached file 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.
OS: Unspecified → All
Hardware: Unspecified → All
Summary: grid-template-rows / grid-template-columns does not recognise multiple values within repeat() notation when used with auto-fill → [css-grid] grid-template-rows / grid-template-columns does not recognise multiple values within repeat() notation when used with auto-fill
Blocks: css-grid
Component: Untriaged → Layout
Product: Firefox → Core
Priority: -- → P3
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
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!
Duplicate of this bug: 1456504
Status: UNCONFIRMED → NEW
Ever confirmed: true
This works in Chrome.  Any hope for a fix soon??
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.

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

Depends on: 1519958
Whiteboard: [DevRel:P1] → [DevRel:P1][layout:backlog]

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

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

Flags: needinfo?(phil)

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?

Flags: needinfo?(phil) → needinfo?(kakyou_tensai)

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

Duplicate of this bug: 1593108

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

Flags: needinfo?(phil)
Assignee: nobody → emcdonough
Status: NEW → ASSIGNED

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

Keywords: leave-open

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.

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5245d585b9a
Refactor TrackSizingFunctions::mRepeatEndDelta to be a getter function rather than a variable. r=emilio
Attachment #9119291 - Attachment description: Bug 1341507 - Change offset calculations for repeats with auto-fit and auto-fill so that they will be correct for values other than zero or one. → Bug 1341507 - Change offset calculations for repeats with auto-fit and auto-fill so that they will be correct for repeat track counts other than one.
Whiteboard: [DevRel:P1][layout:backlog] → [DevRel:P1][layout:backlog:quality]
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a3321b64a58
Refactor auto-fit empty track calculations into a separate method r=mats

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/)

Flags: needinfo?(emcdonough)

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.

Flags: needinfo?(emcdonough)
Attachment #9119291 - Attachment description: Bug 1341507 - Change offset calculations for repeats with auto-fit and auto-fill so that they will be correct for repeat track counts other than one. → Bug 1341507 part 1 - Change offset calculations for repeats with auto-fit and auto-fill so that they will be correct for repeat track counts other than one.
Attachment #9120139 - Attachment is obsolete: true

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.

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

This includes when using subgrid layout.

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

Attachment #9113313 - Attachment is obsolete: true
Attached file 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.

Depends on: 1613725
Whiteboard: [DevRel:P1][layout:backlog:quality] → [DevRel:P1][layout:backlog:quality][layout:backlog:2020q1]
Attachment #9119291 - Attachment description: Bug 1341507 part 1 - Change offset calculations for repeats with auto-fit and auto-fill so that they will be correct for repeat track counts other than one. → Bug 1341507 part 1 - Refactor computed DOM for grid layout to make it simpler for handling repeat values, and to handle multiple repeat values.
Flags: needinfo?(kakyou_tensai)

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)

Flags: needinfo?(emcdonough)

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.

Flags: needinfo?(emcdonough)

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?

Flags: needinfo?(mats)

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

Flags: needinfo?(mats)
Pushed by rmaries@mozilla.com:
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
Backout by shindli@mozilla.com:
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
Pushed by cbrindusan@mozilla.com:
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

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

Flags: needinfo?(emcdonough)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Regressions: 1625051
Blocks: COVID-19
Regressions: 1629575
See Also: → 1632578
You need to log in before you can comment on or make changes to this bug.