1416350 - Grid incorrectly places items in repeat auto-fit grids with leading implicit tracks

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Brad Werth [:bradwerth]]

The logic for calculating line numbers is incorrect for grids with leading implicit tracks. Fix this and provide test coverage that will ensure it stays fixed.

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1416350 Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

Review commit: https://reviewboard.mozilla.org/r/198794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198794/

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1416350 Part 2: Correct logic for Grid API line numbering with leading implicit tracks.

Review commit: https://reviewboard.mozilla.org/r/198796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198796/

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

Review commit: https://reviewboard.mozilla.org/r/198798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198798/

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8927477 [details]
Bug 1416350 Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

https://reviewboard.mozilla.org/r/198794/#review204942

r=mats with the comments addressed.

::: commit-message-0e184:1
(Diff revision 1)
> +Bug 1416350 Part 1: Correct logic for Grid TrackSizingFunctions mRemovedRepeatTracks for leading implicit tracks.

It's not just an issue with mRemovedRepeatTracks.  It also affects
layout since the colAdjust/rowAdjust numbers are wrong we'll translate
the items' offsets wrong.  So we need a commit message that covers
the full issue, which seems to be that we forgot to add in
mExplicitGridOffset to mRepeatAutoStart/End to get a zero-based
implicit grid offset.  So perhaps something like this:
Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

::: layout/generic/nsGridContainerFrame.cpp:3304
(Diff revision 1)
> +  // mGridColEnd: index of last track
> +  // mExplicitGridOffset: index of first explicit track
> +  // mRepeatAutoStart: index of first repeat track relative to
> +  //   mExplicitGridOffset (repeat tracks are explicit by definition).
> +  // mRemovedRepeatTracks: array of repeat tracks relative to
> +  //   mExplicitGridOffset + mRepeatAutoStart.

I don't think we should repeat documentation for members that lives elsewhere here.  So please remove this chunk and amend the documentation for these members where they are declared if necessary.

::: layout/generic/nsGridContainerFrame.cpp:3315
(Diff revision 1)
> +
> +  // Since this loop is concerned with just the repeat tracks, we
> +  // iterate from 0..NumRepeatTracks() which is the natural range of
> +  // mRemoveRepeatTracks. This means we have to add
> +  // (mExplicitGridOffset + mRepeatAutoStart) to map to the other
> +  // "true" arrays like mCellMap and colAdjust. We'll then fill out

nit: s/to map to the other "true" arrays like/to get a zero-based index for/ would be clearer IMO

::: layout/generic/nsGridContainerFrame.cpp:3322
(Diff revision 1)
> -    for (uint32_t col = aState.mColFunctions.mRepeatAutoStart,
> -           endRepeat = aState.mColFunctions.mRepeatAutoEnd,
> +    const uint32_t trueOffset = (aState.mColFunctions.mExplicitGridOffset +
> +                                 aState.mColFunctions.mRepeatAutoStart);

nit: "trueOffset" isn't very informative IMO, I'd suggest "repeatStart" instead

::: layout/generic/nsGridContainerFrame.cpp:3326
(Diff revision 1)
>        aState.mColFunctions.NumRepeatTracks() > 0) {
> -    for (uint32_t col = aState.mColFunctions.mRepeatAutoStart,
> -           endRepeat = aState.mColFunctions.mRepeatAutoEnd,
> -           numColLines = mGridColEnd + 1;
> -         col < numColLines; ++col) {
> +    const uint32_t trueOffset = (aState.mColFunctions.mExplicitGridOffset +
> +                                 aState.mColFunctions.mRepeatAutoStart);
> +    const uint32_t numRepeats = aState.mColFunctions.NumRepeatTracks();
> +    const uint32_t numColLines = mGridColEnd + 1;
> +    for (uint32_t col = 0; col < numRepeats; ++col) {

nit: "col" seems like a misnomer since it's a simple loop counter now.  I'd suggest "i" instead.

::: layout/generic/nsGridContainerFrame.cpp:3328
(Diff revision 1)
> -           endRepeat = aState.mColFunctions.mRepeatAutoEnd,
> -           numColLines = mGridColEnd + 1;
> -         col < numColLines; ++col) {
> +                                 aState.mColFunctions.mRepeatAutoStart);
> +    const uint32_t numRepeats = aState.mColFunctions.NumRepeatTracks();
> +    const uint32_t numColLines = mGridColEnd + 1;
> +    for (uint32_t col = 0; col < numRepeats; ++col) {
>        if (numEmptyCols) {
> -        (*colAdjust)[col] = numEmptyCols;
> +        (*colAdjust)[col + trueOffset] = numEmptyCols;

and "repeatStart + i" here.  Then it should be clear that it's the zero-based index for each repeat track.

::: layout/generic/nsGridContainerFrame.cpp:3338
(Diff revision 1)
>          MOZ_ASSERT(aState.mColFunctions.mRemovedRepeatTracks.Length() >
> -                   repeatIndex);
> -        aState.mColFunctions.mRemovedRepeatTracks[repeatIndex] = true;
> +                   col);
> +        aState.mColFunctions.mRemovedRepeatTracks[col] = true;

I think we can remove this assertion actually.  I think TArray[] already does that.

::: layout/generic/nsGridContainerFrame.cpp:3343
(Diff revision 1)
> +    // Fill out the colAdjust array for all the columns after the
> +    // repeats. numEmptyCols will not change further.

The "numEmptyCols will not change further." doesn't seem necessary IMO.  It sort of follows from the first sentence and it's obvious that's what the one line in this loop is doing.

After removing the second sentence, the first will fit on one line?

::: layout/generic/nsGridContainerFrame.cpp:3346
(Diff revision 1)
> +      for (uint32_t trueCol = numRepeats + trueOffset;
> +           trueCol < numColLines; ++trueCol) {

s/numRepeats + trueOffset/repeatStart + numRepeats/ is probably a more natural order.
Also, I'd prefer "col" over "trueCol".

(And ditto for the above comments in the rowAdjust section.)

::: layout/generic/nsGridContainerFrame.cpp:3380
(Diff revision 1)
>          MOZ_ASSERT(aState.mRowFunctions.mRemovedRepeatTracks.Length() >
> -                   repeatIndex);
> -        aState.mRowFunctions.mRemovedRepeatTracks[repeatIndex] = true;
> +                   row);
> +        aState.mRowFunctions.mRemovedRepeatTracks[row] = true;
> +      }
> +    }
> +    // Fill out the rolAdjust array for all the rows after the

s/rolAdjust/rowAdjust/
Actually, I think we can drop this comment since the rowAdjust section is the same as the colAdjust section anyway, so it doesn't need to be commented in both places.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8927478 [details]
Bug 1416350 Part 2: Correct logic for Grid API line numbering with leading implicit tracks.

https://reviewboard.mozilla.org/r/198796/#review205092

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

https://reviewboard.mozilla.org/r/198798/#review205096

This looks fine so far, but given that grid layout is affected I think
we also need layout reftests to that covers this bug.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase

Here's a few grids where the current rendering is wrong.
It appears there are still a couple of abs.pos. items
that seems wrong to me, so please regard the previous
r+ as pending for now, until we've investigated these.

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8927477 [details]
Bug 1416350 Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198794/diff/1-2/

Comment 9

[Brad Werth [:bradwerth]]

Comment on attachment 8927478 [details]
Bug 1416350 Part 2: Correct logic for Grid API line numbering with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198796/diff/1-2/

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198798/diff/1-2/

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8927477 [details]
Bug 1416350 Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

https://reviewboard.mozilla.org/r/198794/#review205126

::: layout/generic/nsGridContainerFrame.cpp:3304
(Diff revision 1)
> +  // mGridColEnd: index of last track
> +  // mExplicitGridOffset: index of first explicit track
> +  // mRepeatAutoStart: index of first repeat track relative to
> +  //   mExplicitGridOffset (repeat tracks are explicit by definition).
> +  // mRemovedRepeatTracks: array of repeat tracks relative to
> +  //   mExplicitGridOffset + mRepeatAutoStart.

I added some additional detail at the point of declaration.

Comment 12

[Mats Palmgren (inactive)]

A more reduced testcase that shows that the abs.pos. area is wrong
also *without* leading implicit tracks (I think).  So it appears
this is an existing issue.  I filed bug 1417711.

Comment 13

[Mats Palmgren (inactive)]

I've put up a patch in bug 1417711.  In case you want to include that
locally before writing reftests for this bug.

Comment 14

[Brad Werth [:bradwerth]]

Comment on attachment 8927477 [details]
Bug 1416350 Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198794/diff/2-3/

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8927478 [details]
Bug 1416350 Part 2: Correct logic for Grid API line numbering with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198796/diff/2-3/

Comment 16

[Brad Werth [:bradwerth]]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198798/diff/2-3/

Comment 17

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #6)

> This looks fine so far, but given that grid layout is affected I think
> we also need layout reftests to that covers this bug.

I've added a reftest, and numbered it to make room for the test you added in Bug 1417711, which will probably land first.

Comment 18

[Brad Werth [:bradwerth]]

I had a try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ed1fc8756c2d10c65a7f17da0a1f139b8493831&selectedJob=145403819 that failed on layout/reftests/css-grid/grid-repeat-auto-fill-fit-008-ref.html in a section with absolute children over auto-fit tracks. I'll make this bug dependent on Bug 1417711.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

https://reviewboard.mozilla.org/r/198798/#review205666

I'd like to see more variations tested in the reftest please.
Right now we only test the case where a grid item is placed
after a removed track.  We should also test grid-aligned abs.pos.
children (i.e. with position:relative on the container and
left/top/right/bottom:0 on the child).
And also with two normal flow grid items, in a way so that
you get three separate spans of removed tracks - e.g.:
(removed track(s)) item1 (removed track(s)) item2 (removed track(s)
Try placing abs.pos. children in each of those removed spans. etc.

Also, we only test "repeat(auto-fit, 100px)" currently.
It'd be good to also test:
Npx repeat(auto-fit, Mpx)
Npx repeat(auto-fit, Mpx) Npx
repeat(auto-fit, Mpx) Npx

(each with all the item variations mentioned above).

Note that abs.pos. cases may need bug 1416350 in case all tracks
that child spans were removed.  It would be good to test a few
variations actually: all tracks removed / some tracks removed
from the start side / some tracks removed from the end side /
some tracks removed in the interior / multiple spans, e.g.:
(removed track(s)) item1 (removed track(s)) item2 (removed track(s)
   |----------------  abs.pos. child span  --------------|

Throw in anything else you can think of... :-)

Testing multiple abs.pos. cases in the same grid is OK,
no need to create a separate grid for each of those.
All grids should have implicit tracks before line 1 though,
which is where our current reftests are lacking.

Comment 20

[Brad Werth [:bradwerth]]

Attachment: Bug 1416350 Part 4: Add a reftest of repeat:auto-fit grids with leading implicit tracks.

Review commit: https://reviewboard.mozilla.org/r/200942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200942/

Comment 21

[Brad Werth [:bradwerth]]

Comment on attachment 8927477 [details]
Bug 1416350 Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198794/diff/3-4/

Comment 22

[Brad Werth [:bradwerth]]

Comment on attachment 8927478 [details]
Bug 1416350 Part 2: Correct logic for Grid API line numbering with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198796/diff/3-4/

Comment 23

[Brad Werth [:bradwerth]]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198798/diff/3-4/

Comment 24

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #19)
> It'd be good to also test:
> Npx repeat(auto-fit, Mpx)
> Npx repeat(auto-fit, Mpx) Npx
> repeat(auto-fit, Mpx) Npx

Good call. The expanded reftest shows that the patch fails on the fixed-fit-fixed template: Npx repeat(auto-fit, Mpx) Npx. I'll figure that out. The patch is also causing the abs pos children to stretch as Bug 1417711 is supposed to fix (even though this patch is based on that patch). More work for me to do here.

Comment 25

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #24)
> The patch is also causing the abs pos children to stretch as Bug
> 1417711 is supposed to fix (even though this patch is based on that patch).
> More work for me to do here.

I determined those stretch cases are happening even without this patch. I added a reduced testcase to Bug 1417711, and I'll wait for that bug to land before pursuing the remaining issues on this bug.

Comment 26

[Brad Werth [:bradwerth]]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

Clearing review flags since tests will change after blocking bugs land.

Comment 27

[Brad Werth [:bradwerth]]

Comment on attachment 8927477 [details]
Bug 1416350 Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198794/diff/4-5/

Comment 28

[Brad Werth [:bradwerth]]

Comment on attachment 8927478 [details]
Bug 1416350 Part 2: Correct logic for Grid API line numbering with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198796/diff/4-5/

Comment 29

[Brad Werth [:bradwerth]]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198798/diff/4-5/

Comment 30

[Brad Werth [:bradwerth]]

Comment on attachment 8929659 [details]
Bug 1416350 Part 4: Add a reftest of repeat:auto-fit grids with leading implicit tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200942/diff/1-2/

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 8927479 [details]
Bug 1416350 Part 3: Add test to verify line numbers of grids with leading implicit tracks.

https://reviewboard.mozilla.org/r/198798/#review210134

Comment 32

[Mats Palmgren (inactive)]

Attachment: Part 5: Correct the expected results for grids that have leading implicit tracks

Your patches makes layout/reftests/css-grid/grid-repeat-auto-fill-fit-008.html
fail since it has some cases with leading implicit tracks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d56b5750e622f661be14d442dea9eebcf7dec95d&selectedJob=149065516

I've investigated it and I think the current expected values there are
simply wrong, so I've updated them to the new values.  Also, there was
one case that I commented out in bug 1417711 that I'm now enabling again.
This test should now pass with your patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0671e37cce711983dc4321502181ba5e5b146b95

Comment 33

[Mats Palmgren (inactive)]

Comment on attachment 8929659 [details]
Bug 1416350 Part 4: Add a reftest of repeat:auto-fit grids with leading implicit tracks.

https://reviewboard.mozilla.org/r/200942/#review210136

::: layout/reftests/css-grid/grid-repeat-auto-fill-fit-013.html:9
(Diff revision 2)
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html><head>
> +<meta charset="utf-8">
> +<title>CSS Grid Test: test placement in repeat auto-fit grids with leading implicit tracks</title>
> +<link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1416350">

Please correct the value for @title.

Comment 34

[Brad Werth [:bradwerth]]

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

Comment 35

[Brad Werth [:bradwerth]]

I'm not sure how to land a bug with patches from multiple contributors, some of the patches in MozReview and some not. So I'm setting checkin-needed flag. Thank you!

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a547572fe0
Part 1: Correctly account for removed 'auto-fit' tracks also when there are leading implicit tracks. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/295d20cab632
Part 2: Correct logic for Grid API line numbering with leading implicit tracks. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/f56a01a459b7
Part 3: Add test to verify line numbers of grids with leading implicit tracks. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb4742e5d9c
Part 4: Add a reftest of repeat:auto-fit grids with leading implicit tracks. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f86bfbd74f
Part 5: Correct the expected results for grids that have leading implicit tracks. r=bwerth

Comment 37

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/57a547572fe0
https://hg.mozilla.org/mozilla-central/rev/295d20cab632
https://hg.mozilla.org/mozilla-central/rev/f56a01a459b7
https://hg.mozilla.org/mozilla-central/rev/edb4742e5d9c
https://hg.mozilla.org/mozilla-central/rev/48f86bfbd74f