Closed Bug 1423378 Opened 3 years ago Closed 3 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
59 bytes, text/x-review-board-request
mats
: review+
Details
Part 2: Update tests to check line names around empty auto-fit grids.
7.41 KB, patch
bradwerth
: review+
Details | Diff | Splinter Review
Bug 1423378 Part 3: Duplicate repeat auto-fit test into a repeat auto-fill test (with slightly different expected values).
59 bytes, text/x-review-board-request
mats
: review+
Details 
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).
Attachment #8935149 - Flags: review?(mats)
Attachment #8935150 - Flags: review?(mats)
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'?
Attachment #8935149 - Flags: review?(mats) → review-
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.
Attachment #8935150 - Flags: review?(mats) → review-
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.)
Attachment #8935150 - Attachment is obsolete: true
Attachment #8937295 - Flags: review?(bwerth)
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 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.
Attachment #8937295 - Flags: review?(bwerth) → review+
Blocks: 1425954
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?
Attachment #8935149 - Flags: review?(mats) → review-
s/"addNames"/"AddNames"/
(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).
No longer blocks: 1425954
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.
Depends on: 1425954
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&".
Attachment #8935149 - Flags: review?(mats) → review+
Attachment #8938108 - Flags: review?(mats)
(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.
Priority: -- → P1
Attachment #8938107 - Attachment is obsolete: true
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
Attachment #8938108 - Flags: review?(mats) → review+
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.
Attachment #8938107 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.