Closed Bug 1416350 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

(Whiteboard: [designer-tools])

Attachments

(6 files)

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.
Attachment #8927477 - Flags: review?(mats)
Attachment #8927478 - Flags: review?(mats)
Attachment #8927479 - Flags: review?(mats)
Priority: -- → P3
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.
Attachment #8927477 - Flags: review?(mats) → review+
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
Attachment #8927478 - Flags: review?(mats) → review+
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.
Attachment #8927479 - Flags: review?(mats) → review-
Attached file 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 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.
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.
I've put up a patch in bug 1417711.  In case you want to include that
locally before writing reftests for this bug.
(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.
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.
Depends on: 1417711
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.
Attachment #8927479 - Flags: review?(mats) → review-
Summary: Grid chrome API incorrectly reports line numbers on grids with leading implicit tracks → Grid incorrectly places items in repeat auto-fit grids with leading implicit tracks
(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.
(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 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.
Attachment #8927479 - Flags: review?(mats)
Attachment #8927479 - Flags: review?(mats)
Attachment #8929659 - Flags: review?(mats)
Attachment #8927479 - Flags: review?(mats)
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
Attachment #8927479 - Flags: review?(mats) → review+
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
Attachment #8933607 - Flags: review?(bwerth)
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.
Attachment #8929659 - Flags: review?(mats) → review+
Attachment #8933607 - Flags: review?(bwerth) → review+
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!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.