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

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?

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.

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

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

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.

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.

(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

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.

(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?

(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.

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].

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.

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.

(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.

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.

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.

Removed an unneccesary addition to LineNameMap, and updated reftests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=899133f78cb9

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!

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

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

Addresses issues in comment 26; now ready for checkin.

Please add proper commit metadata (author,commit message) into the patch for checkin.

Fixed the patch header, per comment 28. Once again ready for checkin.

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