1423378 - Grid chrome API incorrectly reports line names for completely collapsed auto-fit tracks

Status: RESOLVED FIXED

Description

[Brad Werth [:bradwerth]]

The work on Bug 1415670 revealed that the grid chrome API tests are not correctly testing the names of lines where all auto-fit tracks are removed (in dom/grid/test/chrome/test_grid_repeats.html). That bug strengthened the test and changed the line names for this case to TODO. In the case of all removed auto-fit tracks, the explicit names before the auto-fit and the ones after the auto-fit should be assigned to two different lines (with any "removed" lines between them).

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

don't collapse line names.

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

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1423378 Part 2: Update tests to check line names around empty auto-fit grids.

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

Comment 3

[Mats Palmgren]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

https://reviewboard.mozilla.org/r/205996/#review213814

::: commit-message-79d3e:1
(Diff revision 1)
> +Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto declaration to ensure they are always applied to the following line.
> +

nit: s/repeat auto/repeat auto-fit/

::: commit-message-79d3e:3
(Diff revision 1)
> +don't collapse line names.
> +

It's not clear to me if you intended to add this sentence or what it's supposed to refer to.

::: dom/grid/GridLines.cpp:161
(Diff revision 1)
>                                                 leadingTrackCount,
>                                                 lineNames);
>        }
>  
> +      // If this line is the one that ends a repeat, then add
> +      // in the mNamesFollowingRepeat names from the aLineInfo.

nit: s/from the aLineInfo/from aLineInfo/

::: dom/grid/GridLines.cpp:166
(Diff revision 1)
> +      // in the mNamesFollowingRepeat names from the aLineInfo.
> +      if (numRepeatTracks > 0 &&
> +          i == (aTrackInfo->mRepeatFirstTrack +
> +                aTrackInfo->mNumLeadingImplicitTracks +
> +                numRepeatTracks - numAddedLines)) {
> +        lineNames.AppendElements(aLineInfo->mNamesFollowingRepeat);

I think we need to handle the case where |lineNames| already contains a name in |mNamesFollowingRepeat|.

::: layout/generic/nsGridContainerFrame.cpp:6237
(Diff revision 1)
>  
>        columnLineNames.AppendElement(explicitNames);
>      }
> +    // Get the explicit names that follow a repeat auto declaration.
> +    nsTArray<nsString> colNamesFollowingRepeat;
> +    if (gridColTemplate.HasRepeatAuto()) {

HasRepeatAuto() is true also for 'auto-fill' right?
Do we really want to populate mNamesFollowingRepeat in that case?
Does the condition in SetLineInfo exclude 'auto-fill' somehow?
Do we have tests similar to test_grid_repeats.html for 'auto-fill'?

Comment 4

[Mats Palmgren]

Comment on attachment 8935150 [details]
Bug 1423378 Part 2: Update tests to check line names around empty auto-fit grids.

https://reviewboard.mozilla.org/r/205998/#review213816

::: dom/grid/test/chrome/test_grid_repeats.html
(Diff revision 1)
> -      // If 'number' is omitted, assume that first line is line 1 and increase from there.
> -      is(grid.cols.lines[i].number, (i + 1), elementName + " line " + (i + 1) + " has expected number.");

I don't think we should degrade this test to not verify that the line numbers are correct.

::: dom/grid/test/chrome/test_grid_repeats.html:389
(Diff revision 1)
>      <div id="boxB" class="box b">B</div>
>    </div>
>  
>    <br/>
>    <div id="wrapper8" class="wrapper grid8">
> +    <div id="boxF" class="box f">F</div>

By adding a grid item here there is no grid in this test that is empty, which we really should have tests for.

Comment 5

[Mats Palmgren]

Attachment: Part 2: Update tests to check line names around empty auto-fit grids.

I want to test a lot more cases where all auto-fit tracks are removed,
so I added those tests here.  I suggest we take this patch instead as part 2.
(It addresses my comments on your patch.)

Comment 6

[Mats Palmgren]

Also, please add tests with duplicate line names that fails with
the first version of the patch.

It seems we also have an existing bug that results in duplicate line names, e.g.:
.grid8 {
  grid-template-columns: [real-before before] repeat(auto-fit, [before] 1000px [after]) [real-after];
}

results in:
wrapper8 line 1 has expected names. - got "real-before,before,before", expected "real-before,before"

Please file a separate bug on that.

Comment 7

[Brad Werth [:bradwerth]]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

https://reviewboard.mozilla.org/r/205996/#review213814

> nit: s/repeat auto/repeat auto-fit/

Since the patch at present also applies to auto-fill declarations, I included that in the message as well.

> I think we need to handle the case where |lineNames| already contains a name in |mNamesFollowingRepeat|.

I agree. As you propose in the bug, I'll open a new bug for this issue.

> HasRepeatAuto() is true also for 'auto-fill' right?
> Do we really want to populate mNamesFollowingRepeat in that case?
> Does the condition in SetLineInfo exclude 'auto-fill' somehow?
> Do we have tests similar to test_grid_repeats.html for 'auto-fill'?

It does include auto-fill, even though that case can never have collapsed tracks, because the logic seemed easier that way.

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

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

Comment 9

[Mats Palmgren]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

https://reviewboard.mozilla.org/r/205996/#review214006

::: dom/grid/GridLines.cpp:166
(Diff revision 2)
> +      // in the mNamesFollowingRepeat names from aLineInfo.
> +      if (numRepeatTracks > 0 &&
> +          i == (aTrackInfo->mRepeatFirstTrack +
> +                aTrackInfo->mNumLeadingImplicitTracks +
> +                numRepeatTracks - numAddedLines)) {
> +        lineNames.AppendElements(aLineInfo->mNamesFollowingRepeat);

To be clear, I suggested filing a separate bug on the /existing/ issue.

This line introduces a /new/ issue, or rather fixes this bug in
a buggy way.  I think we should fix it correctly instead.
Perhaps just add a convenience function "addNames" that does
the necessary Contains check before appending each name?

Comment 10

[Mats Palmgren]

s/"addNames"/"AddNames"/

Comment 11

[Mats Palmgren]

(In reply to Brad Werth [:bradwerth] from comment #7)
> > Do we have tests similar to test_grid_repeats.html for 'auto-fill'?

You didn't answer this bit, so I assume "no" :-)

Please add a new test for auto-fill (probably by just copying
test_grid_repeats.html and replacing auto-fit with auto-fill).

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

https://reviewboard.mozilla.org/r/205996/#review214522

::: dom/grid/GridLines.cpp:166
(Diff revision 2)
> +      // in the mNamesFollowingRepeat names from aLineInfo.
> +      if (numRepeatTracks > 0 &&
> +          i == (aTrackInfo->mRepeatFirstTrack +
> +                aTrackInfo->mNumLeadingImplicitTracks +
> +                numRepeatTracks - numAddedLines)) {
> +        lineNames.AppendElements(aLineInfo->mNamesFollowingRepeat);

I believe I have addressed this concern in Bug 1425954, which adds some helper functions to only add unique names. This patch is rebased on top of that patch and uses those helper methods.

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Bug 1423378 Part 2: Update tests to check line names around empty auto-fit grids.

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

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1423378 Part 3: Duplicate repeat auto-fit test into a repeat auto-fill test (with slightly different expected values).

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

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

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

Comment 16

[Mats Palmgren]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

https://reviewboard.mozilla.org/r/205996/#review214568

::: dom/grid/GridLines.cpp:140
(Diff revision 3)
>        nsTArray<nsString> possiblyDuplicateLineNames(
>          aLineInfo->mNames.SafeElementAt(i, nsTArray<nsString>()));

There's no need for possiblyDuplicateLineNames to be a copy of the array, right?
So it should probably just be declared as "const auto&".

Comment 17

[Brad Werth [:bradwerth]]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

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

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8938107 [details]
Bug 1423378 Part 2: Update tests to check line names around empty auto-fit grids.

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

Comment 19

[Brad Werth [:bradwerth]]

Comment on attachment 8938108 [details]
Bug 1423378 Part 3: Duplicate repeat auto-fit test into a repeat auto-fill test (with slightly different expected values).

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

Comment 20

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #16)
> ::: dom/grid/GridLines.cpp:140
> (Diff revision 3)
> >        nsTArray<nsString> possiblyDuplicateLineNames(
> >          aLineInfo->mNames.SafeElementAt(i, nsTArray<nsString>()));
> 
> There's no need for possiblyDuplicateLineNames to be a copy of the array,
> right?
> So it should probably just be declared as "const auto&".

Agreed. I've done so in the latest version of the patch.

Comment 21

[Mats Palmgren]

Comment on attachment 8938108 [details]
Bug 1423378 Part 3: Duplicate repeat auto-fit test into a repeat auto-fill test (with slightly different expected values).

https://reviewboard.mozilla.org/r/208820/#review216312

Comment 22

[Brad Werth [:bradwerth]]

Comment on attachment 8935149 [details]
Bug 1423378 Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line.

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

Comment 23

[Brad Werth [:bradwerth]]

Comment on attachment 8938107 [details]
Bug 1423378 Part 2: Update tests to check line names around empty auto-fit grids.

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

Comment 24

[Brad Werth [:bradwerth]]

Comment on attachment 8938108 [details]
Bug 1423378 Part 3: Duplicate repeat auto-fit test into a repeat auto-fill test (with slightly different expected values).

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

Comment 25

[Brad Werth [:bradwerth]]

Comment on attachment 8938107 [details]
Bug 1423378 Part 2: Update tests to check line names around empty auto-fit grids.

My mozreview push added a duplicate version of Part 2. The existing version, authored by mats and r+ by me should be the one that lands.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d0c815b980
Part 1: Specially treat the explicit line names following a repeat auto-fit or auto-fill declaration to ensure they are always applied to the following line. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/39df8f124f56
Part 2: Update tests to check line names around empty auto-fit grids. r=bradwerth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97438b671ae
Part 3: Duplicate repeat auto-fit test into a repeat auto-fill test (with slightly different expected values). r=mats

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/60d0c815b980
https://hg.mozilla.org/mozilla-central/rev/39df8f124f56
https://hg.mozilla.org/mozilla-central/rev/d97438b671ae