Closed Bug 1281446 Opened 3 years ago Closed 3 years ago

[css-grid] Resolved value of grid-template-columns/rows should now include removed auto-fit tracks

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: mats, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 10 obsolete files)

Recent spec change:
https://drafts.csswg.org/css-grid/#valdef-repeat-auto-fit

Now all tracks, including empty 'auto-fit' tracks, should be included
in the resolved value for these properties.  Empty 'auto-fit' tracks
should have zero size and include line names as if they were non-empty.

I think we only need to update (probably simplify)
nsComputedDOMStyle::GetGridTemplateColumnsRows:
http://searchfox.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#2538

Background:
https://github.com/w3c/csswg-drafts/issues/172
Assignee: nobody → bwerth
I'm constructing a patch to implement this, and one of the cleanest ways to do it is to treat all zero-width columns as collapsed, even if they were specified as an explicit 0px track. The spec at https://drafts.csswg.org/css-grid/#collapsed-track now reads:

> A collapsed track is treated as having a fixed track sizing function of 0px, and the gutters on either side of it collapse.

Which says collapsed => 0px, and collapsed => no gutter, but doesn't say 0px => no gutter. Is there somewhere in the spec that makes clear if 0px tracks get gutters?
Flags: needinfo?(mats)
Attached patch collapseAllZeroes1.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4c36a703950

This patch collapses ALL tracks that resolve to a 0 width, including repeat auto-fit columns that are left blank. It breaks all the reftests at layout/reftests/css-grid/grid-repeat-auto-fill-fit-*, but in a way that I believe matches the revised spec at https://drafts.csswg.org/css-grid/#auto-repeat.

There only reftest I can find that uses a zero-sized track is layout/reftests/css-grid/grid-repeat-auto-fill-fit-005.html. The failure in this test is limited to the auto-fit half of the content, like all its sibling reftests, so it doesn't reveal anything about the correct display of zero-sized tracks.
Attachment #8770637 - Flags: feedback?(mats)
Attached patch collapseEmptyAutoFits1.patch (obsolete) — Splinter Review
This is an alternative to attachment 8770637 [details] [diff] [review]. It only collapses removed auto-fit tracks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35a36ccf6cc1
Attachment #8770667 - Flags: feedback?(mats)
Attached patch collapseEmptyAutoFits2.patch (obsolete) — Splinter Review
This is an alternative to attachment 8770637 [details] [diff] [review] that only collapsess empty auto-fit tracks. Fixes a problem with the previous version that counted removed tracks instead of remaining tracks, resulting in a division-by-zero.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2c987054eba
Attachment #8770667 - Attachment is obsolete: true
Attachment #8770667 - Flags: feedback?(mats)
Attachment #8770741 - Flags: feedback?(mats)
I realized I never gave an argument for why I think we should alter layout instead of just patch up the resolved value properties. Here it is:

When the grid inspector is ready (bug 1181227), it will be helpful to have it report actionable index numbers for tracks and lines. If a developer is using auto-fit, and the tracks and lines are disappearing in the properties used by the inspector, then the grid inspector can't show where the removed tracks and lines are, relative to the retained tracks and lines. For example, CSS that puts a grid item in column 4 might resolve in the inspector to place an item in column 2, which is a big disconnect for the user. If we retain the full track and line counts in layout, this problem is avoided.
Attached patch collapseEmptyAutoFits3.patch (obsolete) — Splinter Review
This is an alternative to attachment 8770637 [details] [diff] [review] that only collapses empty auto-fit tracks. It alters layout such that removed tracks appear at either the start or the end of real tracks, which makes the lines always appear at the edge of real tracks (as they should). It now passes reftests.

Try was down when posted... I'll include a link to treeherder when possible.
Attachment #8770741 - Attachment is obsolete: true
Attachment #8770741 - Flags: feedback?(mats)
Attachment #8771159 - Flags: feedback?(mats)
(In reply to Brad Werth [:bradwerth] from comment #1)
> I'm constructing a patch to implement this, and one of the cleanest ways to
> do it is to treat all zero-width columns as collapsed, even if they were
> specified as an explicit 0px track.
> [The spec] says collapsed => 0px, and collapsed => no gutter, but doesn't say 0px
> => no gutter. Is there somewhere in the spec that makes clear if 0px tracks
> get gutters?

In the absence of spec text saying otherwise, I fully expect that 0px tracks *should* get gutters. Otherwise, that would create an awkward discontinuity between track-width of 0 and a track width of 0.01px, which would prevent authors from being able to gracefully animate/transition explicit track sizes between 0 and other values. (There'd be a jarring "snap" at the beginning/end of the animation, as the gutters collapse/uncollapse.)

So: your later strategy in comment 6 (which you say "only collapses empty auto-fit tracks") is probably closer to what the spec calls for.
> I fully expect that 0px tracks *should* get gutters

No, they shouldn't, see the github issue:
https://github.com/w3c/csswg-drafts/issues/172#issuecomment-227585089
Flags: needinfo?(mats)
Oh sorry, I misread your comment.  What I mean is that "normal" 0px tracks
of course have gutters, but empty 'auto-fit' tracks should NOT, because they
essentially do not exist.
Comment on attachment 8770637 [details] [diff] [review]
collapseAllZeroes1.patch

I really don't want to make any changes at all in the track sizing /
distribution code to fix this bug, because it makes that code more
complex and error prone.  It seems much simpler to just store which
'auto-fit' tracks were empty and then inject zero sized tracks for
them in the CSSOM values.
Attachment #8770637 - Flags: feedback?(mats) → feedback-
Attachment #8771159 - Flags: feedback?(mats) → feedback-
(In reply to Mats Palmgren (vacation) from comment #11)
> I really don't want to make any changes at all in the track sizing /
> distribution code to fix this bug, because it makes that code more
> complex and error prone.

Are you not persuaded by the argument laid out in comment 5?
Flags: needinfo?(mats)
(In reply to Brad Werth [:bradwerth] from comment #12)
> Are you not persuaded by the argument laid out in comment 5?

I don't see how presentation by the Grid Inspector is relevant here.
The computed value should be identical in both alternatives (since
it's defined per spec).

Whether or not we should include empty auto-fit tracks in the API
the Inspector use is a separate issue.  Whatever the Inspector
folks thinks is best for web devs is fine with me.  That doesn't
affects the lines though, which should always be included.

I still think synthesizing values for the collapsed tracks is
a simpler solution than teaching layout / distribution algorithms
to ignore them.  Literally removing them internally also provides
a guarantee that we follow the spec that these tracks are treated
as non-existent.
Flags: needinfo?(mats)
Attached patch restoreAutoFits1.patch (obsolete) — Splinter Review
Adds a todo-style testcase for expected outcome. Updated testcase also illustrates the mixed messages this change will send by comparing getComputedStyle(<elem>).gridTemplateColumns vs <grid>.cols.tracks. The template in the test case reports 7 tracks, but our API reports 3. There's also no way to distinguish between the "real" 0 size column 2 and the removed 0 size column 3 in in the resolved template. Are we okay with this? If not, two courses of action:

a) Update our API to report removed tracks, and give them a state of "removed" or something.
b) Actually maintain the 0 size tracks through layout, as attempted in attachment 8771159 [details] [diff] [review].
Attachment #8787008 - Flags: feedback?(mats)
Comment on attachment 8787008 [details] [diff] [review]
restoreAutoFits1.patch

(In reply to Brad Werth [:bradwerth] from comment #14)
> [...] Are we okay with this?

Yes, I don't see any value in listing removed 'auto-fit' tracks
in the devtools UI.  But you should ask the devtools UI developers
what they think.  They might see some value in it that I don't.

> If not, two courses of action:
> 
> a) Update our API to report removed tracks, and give them a state of
> "removed" or something.

a) is OK with me in that case.  b) is not.
Attachment #8787008 - Flags: feedback?(mats) → feedback+
Attached patch retainEmptyAutoFits1.patch (obsolete) — Splinter Review
This patch does only what was proposed originally; layout is unchanged but getComputedStyle(<elem>).gridTemplateColumns reports the removed tracks as '0'. If this is the right direction to go, I'll also update the dev tools API to report these removed tracks.

The test illuminates a strange issue with this approach. Consider this template:

grid-template-columns: 50px 0 [real-before] repeat(auto-fit, [before] 100px [after]) [real-after];

...with room for 5 repeat columns. If columns 4 and 6 have items, this patch reports:

50px 0px 0 [real-before before] 100px 0 [after before] 100px 0 [after real-after]

...which makes the following assumptions:

a) Real 0px columns are reported as "0px". Removed columns are reported as "0".
b) Real lines that get assigned multiple names are listed only once, even if there are removed tracks between the two names in the template. In this example, the template around columns 4 to 6 isn't "100px [after] 0 [before] 100px", because that looks too much like there are two lines with two names, instead of one line with two names. But if we like this stylistically better, we can change the code to report it that way.

Checked against Chrome and it is not yet stripping out the auto-fit tracks, so it's not any help to test compatibility. The returned template there is:

50px 0px [real-before before] 100px [after before] 100px [after before] 100px [after before] 100px [after before] 100px [after real-after]

Let me know if this patch is delivering desired behavior, and if so I'll adjust the dev tools API and the associated tests to match.
Attachment #8770637 - Attachment is obsolete: true
Attachment #8787008 - Attachment is obsolete: true
Attachment #8787788 - Flags: feedback?(mats)
(In reply to Brad Werth [:bradwerth] from comment #16)
> If this is the right direction to go, I'll also update the dev tools
> API to report these removed tracks.

Let's handle changes in the devtools API in a separate bug for tracking
purposes (it's not blocking release, whereas this bug is).

> a) Real 0px columns are reported as "0px". Removed columns are reported as
> "0".

Removed tracks should also be reported a 0px, not 0.
Use SetAppUnits instead of SetNumber?

> b) Real lines that get assigned multiple names are listed only once, even if
> there are removed tracks between the two names in the template.

Yeah, that seems wrong.  I think the auto-fit result should be the same
as if you replaced it with auto-fill, except that any removed auto-fit
tracks are 0px.
... i.e. the result should be what you see in Chrome except that the empty
auto-fit tracks should be 0px.
Attached patch retainEmptyAutoFits2.patch (obsolete) — Splinter Review
Addresses the issues raised in comment 17 and comment 18. Reports tracks and lines similar to how Chrome implements it, with the difference that removed auto-fit tracks become "0px" tracks.
Attachment #8787788 - Attachment is obsolete: true
Attachment #8787788 - Flags: feedback?(mats)
Attachment #8788601 - Flags: review?(mats)
Blocks: 1300877
layout/reftests/css-grid/grid-repeat-auto-fill-fit-006 through -008 need updating to match new behavior. I'll put that in the next patch.
Attachment #8771159 - Attachment is obsolete: true
Attached patch retainEmptyAutoFits3.patch (obsolete) — Splinter Review
Removed an unneccesary addition to LineNameMap, and updated reftests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=899133f78cb9
Attachment #8788601 - Attachment is obsolete: true
Attachment #8788601 - Flags: review?(mats)
Attachment #8789017 - Flags: review?(mats)
Comment on attachment 8789017 [details] [diff] [review]
retainEmptyAutoFits3.patch

> layout/generic/nsGridContainerFrame.cpp
>-  void SetNumRepeatTracks(uint32_t aNumRepeatTracks)
>+  void SetNumKeptRepeatTracks(uint32_t aNumRepeatTracks)

I prefer the old name, because we don't actually know at the point
this is called which tracks will be kept or not.  That will be
determined later and some might not be kept, which makes the new
name promise too much IMO.

> layout/style/nsComputedDOMStyle.cpp
>+          // List this track as a 0px.

Nit: we might as well spell it out for readability:
// Removed 'auto-fit' tracks are reported as 0px.

>+              // Append any removed auto-fits.
>+              AddRemovedAutoFits(false);

The comment is superfluous and thus makes the code less readable, IMO.
Please remove it (twice), and rename AddRemovedAutoFits to
AppendRemovedAutoFits.

>+
>         if (uint32_t(i) == numExplicitTracks) {

Nit: spurious newline


I suspect the result is still wrong for empty grids:
data:text/html,<div style="display:grid; grid: 100px / repeat(auto-fit, [a] 10px [b]) ; width:100px"></div>
still computes to 'grid-template-columns:none'.  I think this is supposed to
compute to ten 0px columns now, right?

There is an early return in nsComputedDOMStyle::GetGridTemplateColumnsRows
for this case.  Probably just need to add "!aTrackList.HasRepeatAuto()"
to the condition there and make the code that follows deal with zero
explicit tracks?

r- for that last issue, other than that this looks good!
Flags: needinfo?(bwerth)
Attachment #8789017 - Flags: review?(mats) → review-
Attached patch retainEmptyAutoFits4.patch (obsolete) — Splinter Review
Addresses issues in commment 24. Cleaned up some function names, fixed some comments, and changed the 'none' report for grid-template-rows and -columns when all auto-fit columns are empty.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9460fc471312
Attachment #8789017 - Attachment is obsolete: true
Flags: needinfo?(bwerth)
Attachment #8789099 - Flags: review?(mats)
Comment on attachment 8789099 [details] [diff] [review]
retainEmptyAutoFits4.patch

Not a big deal, but in the future, please add tests of
grid-template-columns/rows computed values to
layout/style/test/test_grid_computed_values.html
instead.  We don't need chrome privilege to test these and the above file
could be converted to a wpt test for cross-browser testing.

> layout/style/nsComputedDOMStyle.cpp
> if ((numSizes == 0) && !aTrackList.HasRepeatAuto()) {

nit: drop the redundant () for code style consistency
Attachment #8789099 - Flags: review?(mats) → review+
Attached patch retainEmptyAutoFits5.patch (obsolete) — Splinter Review
Addresses issues in comment 26; now ready for checkin.
Attachment #8789099 - Attachment is obsolete: true
Keywords: checkin-needed
Please add proper commit metadata (author,commit message) into the patch for checkin.
Keywords: checkin-needed
Fixed the patch header, per comment 28. Once again ready for checkin.
Attachment #8789428 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31fe1120ff9a
Resolved value of grid-template-columns/rows now lists removed auto-fit tracks as 0px. r=mats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/31fe1120ff9a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.