Closed Bug 1176792 Opened 10 years ago Closed 9 years ago

[css-grid] Implement "Gutters: the grid-column-gap, grid-row-gap, and grid-gap properties"

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 5 obsolete files)

part 3 - [css-grid] Reftests for the 'grid-column-gap' and 'grid-row-gap' properties
25.62 KB, patch
Details | Diff | Splinter Review
part 2 - [css-grid] Implement layout for the 'grid-column-gap' and 'grid-row-gap' properties
10.09 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
screenshot of reference case for grid-column-gap-001.html
34.54 KB, image/png
Details
'border: inherit' testcase
358 bytes, text/html
Details
part 1 - [css-grid] Implement the 'grid-column-gap', 'grid-row-gap', and 'grid-gap' properties in the style system
26.27 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
http://dev.w3.org/csswg/css-grid/#gutters Note: this is a recent addition to the spec and still being discussed on www-style. We should probably wait for the spec to settle before implementing this.
FTR, I've posted some questions / comments on these properties here: https://lists.w3.org/Archives/Public/www-style/2015Oct/0028.html
Summary: [css-grid] Implement "Gutters: the column-gap and row-gap properties" → [css-grid] Implement "Gutters: the grid-column-gap, grid-row-gap, and grid-gap properties"
The CSSWG hasn't responded to my questions yet (URL above), so I made the following choices in their absence: * disallow <percent> (for now) * disallow negative values * don't implement 'normal', zero is the initial value for 'grid-gap' * make it animatable (coord) because I see no reason not to This is a small and simple patch, but there is one thing out of the ordinary: https://drafts.csswg.org/css-grid/#grid-shorthand says: "Also, the gutter properties are reset by this shorthand, even though they can't be set by it." so I made ParseGrid() also produce an initial value for grid-[row|column]-gap when the parsing of 'grid' succeeds. I'm guessing we don't want to reset these properties when it fails.
Assignee: nobody → mats
Attachment #8681231 - Flags: review?(cam)
Comment on attachment 8681231 [details] [diff] [review] part 1 - [css-grid] Implement the 'grid-column-gap', 'grid-row-gap', and 'grid-gap' properties in the style system Review of attachment 8681231 [details] [diff] [review]: ----------------------------------------------------------------- Gah, I misread the review request and started reviewing this part which is really r?heycam. Here are my notes, up to the point where I realized this: ::: layout/style/nsCSSParser.cpp @@ +9405,5 @@ > + AppendValue(eCSSProperty_grid_column_gap, first); > + nsCSSValue second; > + bool gotSecond = ParseNonNegativeVariant(second, VARIANT_LCALC, nullptr) == > + CSSParseResult::Ok; > + AppendValue(eCSSProperty_grid_row_gap, gotSecond ? second : first); I think you need to account for the possibility that ParseNonNegativeVariant() might returns CSSParseResult::Error in this second call (i.e. it walked past some tokens, failed to parse what it was looking for, & was unable to put back what it found). In this case, the current code is wrong -- we append the first value twice & declare success, and let our caller proceed with parsing -- but that's bad, because we already parsed past some stuff which our caller won't get to see. We really need to declare failure in this scenario. (I'm not sure offhand how to actually trigger this scenario here -- perhaps with a broken or unbalanced 'calc' expression? -- but it's a failure-mode that's allowed to happen for ParseNonNegativeVariant & which we need to handle.) SO, anyway -- I think we need to do move the "ParseNonNegativeVariant(second, ...)" call up, to happen *before* we've made our first AppendValue call. And we need to capture that second ParseNonNegativeVariant return-value, and then react to it by either bailing (in case of Error), or appending the first value twice (in case of NotFound) or append the first & second value separately (in case of Ok). @@ +9413,5 @@ > +// https://drafts.csswg.org/css-grid/#grid-shorthand > +// "Also, the gutter properties are reset by this shorthand, > +// even though they can't be set by it." > +bool > +CSSParserImpl::ResetGridGap() The "bool" return value is a bit confusing (and asking-to-be-removed) at first glance, since this method can't fail. Maybe add a comment explaining it? Something like: // Returns type 'bool', despite never failing, for brevity & // convenience in ParseGrid's compound return statements. ::: layout/style/test/property_database.js @@ +6146,5 @@ > + inherited: false, > + type: CSS_TYPE_LONGHAND, > + initial_values: [ "0" ], > + other_values: [ "2px", "1em", "calc(1px + 1em)" ], > + invalid_values: [ "-1px", "2%", "auto", "none", "calc(1%)" ], Note: The very last value in invalid_values here is different, between the row & column properties. Did you mean to have them in sync? (The rests of the lists here are the same -- just this one last value differs.) (I'd lean towards making them the same, for consistency/robustness. If they were longer lists, then maybe we'd want to remove some overlap to avoid redundant testing. But they're pretty short right now, so that's not really a concern.)
Comment on attachment 8681231 [details] [diff] [review] part 1 - [css-grid] Implement the 'grid-column-gap', 'grid-row-gap', and 'grid-gap' properties in the style system I actually made it most of the way through the review, so I just finished off the last few files & didn't have any additional feedback, & I'll mark this r=me. Though of course feel free to re-request review from heycam if you wanted feedback from him as well. (and heycam can feel free to chime in if he has anything to add)
Attachment #8681231 - Flags: review?(cam) → review+
(r=me with at least the first part of comment 5 addressed, I mean)
Comment on attachment 8681232 [details] [diff] [review] part 2 - [css-grid] Implement layout for the 'grid-column-gap' and 'grid-row-gap' properties Review of attachment 8681232 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 2 with the following addressed: ::: layout/generic/nsGridContainerFrame.cpp @@ +2698,2 @@ > } > + MOZ_ASSERT(!roundingError, "we didn't distribute all space?"); The assertion text here needs an adjustment. The current message-text is referring to the condition that you removed (pos == space). Maybe just s/all space/all rounding error/ (or "all remainder") @@ +3026,5 @@ > state.mCols.CalculateSizes(state, mGridItems, state.mColFunctions, > NS_UNCONSTRAINEDSIZE, &GridArea::mCols, > aConstraint); > + const nscoord gridGap = state.mCols.mGridGap; > + nscoord length = -gridGap; // no grid-gap before the first track This "-gridGap" thing isn't correct if mSizes is empty, I think? (Then we'll end up returning -gridGap as our IntrinsicISize which seems wrong). Can't we do away with all the local gridGap-handling here, and just add state.mCols.SumOfGridGaps() before we return?
Attachment #8681232 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #5) > Gah, I misread the review request and started reviewing this part... I'm very happy you did. This is first time in *years* that I've got a timely Style System review. > I think you need to account for the possibility that > ParseNonNegativeVariant() might returns CSSParseResult::Error in this second > call Fixed. > Maybe add a comment explaining it? Something like: > // Returns type 'bool', despite never failing, for brevity & > // convenience in ParseGrid's compound return statements. Meh, that feels a bit like ELI5 on reddit. :-) I think that if people actually wonder about it they will take a quick look at the call sites and quickly realize what you just said. > Note: The very last value in invalid_values here is different, between the > row & column properties. Did you mean to have them in sync? Not really; but sure, we can check these for both properties.
Attachment #8681231 - Attachment is obsolete: true
Attachment #8681448 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #8) > The assertion text here needs an adjustment. Fixed. > > + nscoord length = -gridGap; // no grid-gap before the first track > > This "-gridGap" thing isn't correct if mSizes is empty, I think? Correct, but mSizes can't be empty at this point, because we have this at the top: if (mGridColEnd == 0) { return 0; } Anyway, your SumOfGridGaps() suggestion is good, thanks. (minor changes here, so I'm just moving the r+ forward)
Attachment #8681232 - Attachment is obsolete: true
Attachment #8681451 - Flags: review+
Comment on attachment 8681448 [details] [diff] [review] part 1 - [css-grid] Implement the 'grid-column-gap', 'grid-row-gap', and 'grid-gap' properties in the style system (In reply to Mats Palmgren (:mats) from comment #9) > > // Returns type 'bool', despite never failing, for brevity & > > // convenience in ParseGrid's compound return statements. > > Meh, that feels a bit like ELI5 on reddit. :-) > I think that if people actually wonder about it they will take a quick > look at the call sites and quickly realize what you just said. Pushing back slightly: the reason I brought it up is that we *do* have (or used to have, anyway) lots of functions that return 'bool' or 'nsresult' for no good reason (largely COM gunk, but other stuff too). Do-gooders & new contributors looking to simplify code may peridically look for & fix these (I hope). And this particular function *looks* like it's in that category, so I marginally fear that it will be repeatedly re-discovered as a de-bool-ification opportunity -- and each time, someone will have to do a bit of research to find out that no it's actually not, which is a waste of their time. So, a short comment explaining the bool return-value could save those do-gooders a bit of time/research. But, r=me regardless; thanks for fixing the ParseNonNegativeVariant issue.
Attachment #8681448 - Flags: review?(dholbert) → review+
Comment on attachment 8681233 [details] [diff] [review] part 3 - [css-grid] Reftests for the 'grid-column-gap' and 'grid-row-gap' properties Review of attachment 8681233 [details] [diff] [review]: ----------------------------------------------------------------- Two drive-by nits on the test patch: ::: layout/reftests/css-grid/grid-column-gap-001.html @@ +64,5 @@ > +]; > +var cols = [ "0", "1", "2", "3", "8", "9" ]; > +var widths = [ "0", "1", "5", "6" ]; > +var gaps = [ "1", "2" ]; > +for (var j = 0; j < justify.length; ++j) { Nit: this test is perhaps a little too wide -- it has "rows" of floated elements, but unless your window is > ~1080px, those "rows" wrap somewhat awkwardly. And on Desktop, our reftest window is only 800px wide, so that'll definitely happen. (and will happen to a much greater extent on mobile, where a lot of the test will probably be off the bottom of the screen, too). You might consider splitting this into two tests (which would help keep more of the test onscreen for mobile), or possibly inserting "clear:left" elements halfway across each line of floats, to make wrapping occur at more predictable/defined points. ::: layout/reftests/css-grid/grid-row-gap-001-ref.html @@ +61,5 @@ > +.aspace-evenly item3 { offset-block-start:16.75px; } > + > +.astretch2 { grid-template-rows: 1fr 1px 5px 1px 7px; } > +.astretch3 { grid-template-rows: 14.5px 1px 16.3333px 1px 7px; } > +.astretch4 { grid-template-rows: 10.66666px 1px 12.66666px 1px 14.66666px; } (If there's a not-too-difficult way of tweaking the testcase to avoid the need for imprecise repeating-decimal values like this in the reference case, it seems like it'd be worth doing...)
> Nit: this test is perhaps a little too wide Are you sure? When I test these locally they all fit in 800x1000px (which is my goal).
Here's what I'm talking about, w.r.t. float wrapping -- here's how wide the testcase "wants" to be. Probably not a big deal, feel free to disregard.
Yeah, it's organized that way to make it easier for humans to inspect. Granted, you need to resize the window for optimal viewing.
Flags: in-testsuite+
TEST-UNEXPECTED-FAIL | /web-animations/keyframe-effect/constructor.html | a KeyframeEffectReadOnly can be constructed with a Keyframe sequence where lesser shorthand precedes greater shorthand - assert_equals: value for 'borderLeftColor' on ComputedKeyframe #0 expected "rgb(1, 2, 3)" but got "rgb(4, 5, 6)" The failed test doesn't use any grid properties at all AFAICT. The patches in this bug only affects CSS Grid and are platform agnostic so a Windows-only failure seems very unlikely. Unless I screwed up royally and caused a memory corruption bug or something of that nature. My Try push that preceded the landing had green W-tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=add179436ad8
Flags: needinfo?(mats)
FYI, it's not a 100%-permafail. Here's a green W4 after this landing: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=21a2e66bf784 (but granted, it's near 100% failure rate on W4. Weird....)
Well, at least it's reproducible on Try now, which is good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=155bbc9886b7
The test fails also without part 2 (layout): https://treeherder.mozilla.org/#/jobs?repo=try&revision=281ba8a2b5a0 FWIW, I also tried with eStyleAnimType_None (instead of _Coord) on the new properties but the test still fails.
Unfortunately I can't reproduce locally on my Windows 7 machine. I tried both debug and optimized builds.
Comment on attachment 8681448 [details] [diff] [review] part 1 - [css-grid] Implement the 'grid-column-gap', 'grid-row-gap', and 'grid-gap' properties in the style system I don't see anything wrong in this patch. Perhaps something is _missing_ though? (see also earlier comments in case they give any clues) Let me know if you have any ideas on how to debug this further.
Attachment #8681448 - Flags: review?(dbaron)
Hmm, just adding the properties in nsCSSPropList makes the test fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c2aa5bd48bf
Mats, can you try that again but without adding the grid-gap shorthand? The KeyframeEffectReadOnly subtest is doing something with shorthands, so it might be related to that.
... but it's green with just one of the properties: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ba7984234c
... and the other property, just as a sanity check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e378edfbb68 So, adding either of the properties individually pass the test, but adding them both makes it fail.
Comment on attachment 8681448 [details] [diff] [review] part 1 - [css-grid] Implement the 'grid-column-gap', 'grid-row-gap', and 'grid-gap' properties in the style system I think this patch is fine. It seems more likely that we're hitting an existing bug elsewhere.
Attachment #8681448 - Flags: review?(dbaron)
(In reply to Mats Palmgren (:mats) from comment #19) > TEST-UNEXPECTED-FAIL | /web-animations/keyframe-effect/constructor.html | a > KeyframeEffectReadOnly can be constructed with a Keyframe sequence where > lesser shorthand precedes greater shorthand - assert_equals: value for > 'borderLeftColor' on ComputedKeyframe #0 expected "rgb(1, 2, 3)" but got > "rgb(4, 5, 6)" I was seeing this failure on try on Thursday as well, but it didn't happen once I landed on inbound. See bug 1221436 comment 15.
... though maybe that's because I was pushing on top of one of these changesets, I guess. It actually stopped failing when I rebased to a different changeset. I'm a little confused by the Declaration::GetValue change in the patch. I think that change is wrong; if the properties have non-default values, then the getter for the shorthand should return the empty string. See https://drafts.csswg.org/cssom/#serialize-a-css-value . I don't see how that error would be related to the test failure, though.
... and I would expect you to have to change the case eCSSProperty_grid: in Declaration::GetValue (but not the beginning of the function).
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #33) > I think that change is wrong; if the properties have non-default values, then > the getter for the shorthand should return the empty string. Thanks. Yes, this is a bit more complex than I first thought. The style system makes the assumption that all the sub-properties are to be set/get in a lot of places. (In reply to David Baron [:dbaron] ⌚UTC+8 from comment #34) > ... and I would expect you to have to change the case eCSSProperty_grid: in > Declaration::GetValue (but not the beginning of the function). No, we really need to ignore these two sub-properties up front because otherwise we'll bail out on something like "grid:inherit" because the loop here counts the number of inherit/unset/initial to make sure all sub-properties have that value. There really is no support for "reset, but not set/get" sub-properties in the style system currently.
Attached patch wip1 (obsolete) — — Splinter Review
So, there are two alternatives for implementing this: either we add the properties to gGridSubpropTable and add exceptions in a number of places where these sub-properties should be excluded because they are not to be set/get. Or...
Attachment #8681448 - Attachment is obsolete: true
Flags: needinfo?(dbaron)
Attached patch wip2 (obsolete) — — Splinter Review
... we don't add them to gGridSubpropTable and add code in a number of places where these properties should be reset. Maybe you have some other suggestions on how to implement it? BTW, look for "CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES" to find the relevant places for this topic in the wip1/wip2 patches.
BTW, "wip1" does fix the W4 test failure we saw earlier so I think it's clear something in the original "part 1" patch caused it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=229089ef7520
(In reply to Mats Palmgren (:mats) from comment #35) > (In reply to David Baron [:dbaron] ⌚UTC+8 from comment #33) > > I think that change is wrong; if the properties have non-default values, then > > the getter for the shorthand should return the empty string. > > Thanks. Yes, this is a bit more complex than I first thought. > The style system makes the assumption that all the sub-properties > are to be set/get in a lot of places. That's how shorthands always work. I don't see anything in the spec that says the 'grid' shorthand is any different. Could you point out the text? > (In reply to David Baron [:dbaron] ⌚UTC+8 from comment #34) > > ... and I would expect you to have to change the case eCSSProperty_grid: in > > Declaration::GetValue (but not the beginning of the function). > > No, we really need to ignore these two sub-properties up front > because otherwise we'll bail out on something like "grid:inherit" > because the loop here counts the number of inherit/unset/initial > to make sure all sub-properties have that value. Could you point out particular spec text that says these properties behave differently from every other CSS property that works like this? 'grid: inherit' should make all properties that are set by the 'grid' shorthand 'inherit', including ones that can't be set to anything other than their initial value or to (and by) the CSS-wide keywords. > There really is no support for "reset, but not set/get" sub-properties > in the style system currently. No, there are plenty of other properties like that. Consider, e.g., the 'border' shorthand resetting all the border-image-* properties.
Flags: needinfo?(dbaron) → needinfo?(mats)
Quoting the spec text that is relevant to the gutter properties (grid-*-gap): https://drafts.csswg.org/css-grid/#propdef-grid "The 'grid' property is a shorthand that sets all of the explicit grid properties [...] and the gutter properties [...] in a single declaration. [...] Other omitted values are set to their initial values." (The grid-*-gap sub-properties are always omitted since they can't be specified in the 'grid' shorthand.) The Note in the green box continues: "Also, the gutter properties are reset by this shorthand, even though they can't be set by it."
Flags: needinfo?(mats) → needinfo?(dbaron)
The spec might be poorly worded (could you email www-style), but I'm pretty sure you're interpreting that wording in ways its authors didn't intend.
Flags: needinfo?(dbaron) → needinfo?(mats)
The relevant spec text here is really from https://drafts.csswg.org/css-cascade/#shorthand : In some cases, a shorthand might have different syntax or special keywords that don’t directly correspond to values of its sub-properties. (In such cases, the shorthand will explicitly define the expansion of its values.) In other cases, a property might be a reset-only sub-property of the shorthand: Like other sub-properties, it is reset to its initial value by the shorthand when unspecified, but the shorthand might not include syntax to set the sub-property to any of its other values. For example, the border shorthand resets border-image to its initial value of none, but has no syntax to set it to anything else. [CSS3BG] If a shorthand is specified as one of the CSS-wide keywords [CSS3VAL], it sets all of its sub-properties to that keyword, including any that are reset-only sub-properties. (Note that these keywords cannot be combined with other values in a single declaration, not even in a shorthand.)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #39) > No, there are plenty of other properties like that. Consider, e.g., the > 'border' shorthand resetting all the border-image-* properties. I suspect that we implement that part of 'border' wrong too. https://drafts.csswg.org/css-backgrounds-3/#border "The ‘border’ shorthand also resets ‘border-image’ to its initial value." Compare this testcase with Chrome where 'border-image' is reset to its initial value as the spec says.
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #42) > The relevant spec text here is really from > https://drafts.csswg.org/css-cascade/#shorthand : > If a shorthand is specified as one of the CSS-wide keywords [CSS3VAL], it > sets all of its sub-properties to that keyword, including any that are > reset-only sub-properties. Oh, OK. I agree that this part is utterly clear that 'inherit' propagates to the reset-only sub-properties. I will proceed with that and assume Chrome is buggy.
Flags: needinfo?(mats)
(FWIW, I think the spec text for 'border' and 'grid' are rather misleading if the quoted css-cascade spec text is in fact the correct behavior.)
Thanks, I filed https://code.google.com/p/chromium/issues/detail?id=557256 on the Grid properties (since I suspect it's different people working on that).
Only a couple of minor changes to the version that dholbert already reviewed: In Declaration::GetValue : moved the grid_[row|column]_gap tests down into the eCSSProperty_grid case. In CSSParserImpl::ParseGrid() : return true in the VARIANT_INHERIT branch, rather than doing a ResetGridGap() there. Thanks for your help sorting out the spec confusion. https://treeherder.mozilla.org/#/jobs?repo=try&revision=833a33030d66
Attachment #8686565 - Attachment is obsolete: true
Attachment #8686566 - Attachment is obsolete: true
Attachment #8688631 - Flags: review?(dbaron)
Comment on attachment 8688631 [details] [diff] [review] part 1 - [css-grid] Implement the 'grid-column-gap', 'grid-row-gap', and 'grid-gap' properties in the style system In nsCSSParser.cpp, you can call ResetGridGap earlier in ParseGrid -- it's ok to modify subproperties in value parsing functions even when they still might fail; we'll throw away the results. Given that, you shouldn't bother making ResetGridGap a separate function; you should just write it inline in ParseGrid, probably just after the return-false-if-empty-value. nsCSSPropList.h: >+ CSS_PROPERTY_VALUE_NONNEGATIVE, I don't see anywhere the spec calls for this. If you think it should, you should raise it on www-style, and add comments to the code explaining that the spec discussion isn't settled yet. Same for the clampNegative in nsComputedDOMStyle.cpp, and SETCOORD_CALC_CLAMP_NONNEGATIVE in nsRuleNode.cpp. ... and of course also the tests in property_database.js nsStyleStruct.h: >+ nsStyleCoord mGridColumnGap; // [reset] coord, calc >+ nsStyleCoord mGridRowGap; // [reset] coord, calc These should just be nscoord, since there's only a single type in the computed value. calc() gets computed to a length since there are no percentages in the computed value. (See outline-offset or border-spacing for examples.) test_grid_shorthand_serialization.html: >+grid_important_test_cases = [ var, please r=dbaron with that
Attachment #8688631 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #50) > >+ CSS_PROPERTY_VALUE_NONNEGATIVE, > > I don't see anywhere the spec calls for this. If you think it > should, you should raise it on www-style, and add comments to > the code explaining that the spec discussion isn't settled yet. I already asked this on www-style over a month ago... https://lists.w3.org/Archives/Public/www-style/2015Oct/0028.html
Tab said (in repsonse) that he specified that negative values are invalid: https://lists.w3.org/Archives/Public/www-style/2015Nov/0072.html I'm not immediately seeing that in the spec, though it might be that I haven't had any coffee yet. (If it's not there, maybe this spec-edit got lost accidentally?)
(Ah yes, there's a response to Tab's email pointing out that his changes didn't make it into the spec yet, probably by accident: https://lists.w3.org/Archives/Public/www-style/2015Nov/0175.html )
Odd, if you follow the link in comment 51 and click "Next in thread" his reply doesn't show up, but if you click "In reply to" in his reply my mail shows up.
Depends on: 1227099
Thanks! I filed bug 1227099.
Depends on: 1230695
Blocks: css-grid-1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: