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

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

2 years ago
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).
Assignee

Updated

2 years ago
Attachment #8935149 - Flags: review?(mats)
Attachment #8935150 - Flags: review?(mats)

Comment 3

2 years ago
mozreview-review
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 4

2 years ago
mozreview-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-

Comment 5

2 years ago
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)

Comment 6

2 years ago
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.
Assignee

Comment 7

2 years ago
mozreview-review-reply
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.
Assignee

Updated

2 years ago
Attachment #8937295 - Flags: review?(bwerth) → review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1425954

Comment 9

2 years ago
mozreview-review
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-

Comment 10

2 years ago
s/"addNames"/"AddNames"/

Comment 11

2 years ago
(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).
Assignee

Updated

2 years ago
No longer blocks: 1425954
Assignee

Comment 12

2 years ago
mozreview-review
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.
Assignee

Updated

2 years ago
Depends on: 1425954
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 years ago
mozreview-review
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+
Assignee

Updated

2 years ago
Attachment #8938108 - Flags: review?(mats)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 20

2 years ago
(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
Assignee

Updated

2 years ago
Attachment #8938107 - Attachment is obsolete: true

Comment 21

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 25

2 years ago
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
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 26

2 years ago
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.