Closed
Bug 1423378
Opened 5 years ago
Closed 5 years ago
Grid chrome API incorrectly reports line names for completely collapsed auto-fit tracks
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: bradwerth, Assigned: bradwerth)
References
Details
Attachments
(3 files, 2 obsolete files)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8935149 -
Flags: review?(mats)
Attachment #8935150 -
Flags: review?(mats)
Comment 3•5 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•5 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•5 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•5 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•5 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•5 years ago
|
Attachment #8937295 -
Flags: review?(bwerth) → review+
Comment hidden (mozreview-request) |
Comment 9•5 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•5 years ago
|
||
s/"addNames"/"AddNames"/
Comment 11•5 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 | ||
Comment 12•5 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•5 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•5 years ago
|
Attachment #8938108 -
Flags: review?(mats)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•5 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.
Updated•5 years ago
|
Priority: -- → P1
Assignee | ||
Updated•5 years ago
|
Attachment #8938107 -
Attachment is obsolete: true
Comment 21•5 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•5 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•5 years ago
|
Keywords: checkin-needed
Comment 26•5 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
Comment 27•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/60d0c815b980 https://hg.mozilla.org/mozilla-central/rev/39df8f124f56 https://hg.mozilla.org/mozilla-central/rev/d97438b671ae
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•