Closed Bug 1117538 Opened 5 years ago Closed 5 years ago

[css-grid] Remove 'grid-auto-flow: stack' and accept 'dense' by itself meaning 'row dense'

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

We should remove 'grid-auto-flow: stack' because it has been removed from
the spec: http://dev.w3.org/csswg/css-grid/#propdef-grid-auto-flow
Attached patch fix (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c64ec9268770
Attachment #8543660 - Flags: review?(simon.sapin)
Comment on attachment 8543660 [details] [diff] [review]
fix

Stealing review.

>Bug 1117538 - [css-grid] Remove 'grid-auto-flow: stack'. r=simon.sapin
>
>Because it has been removed from the spec:
>http://dev.w3.org/csswg/css-grid/#propdef-grid-auto-flow

Might be better to make the commit message link to the www-style post that announced the removal, here:
 http://lists.w3.org/Archives/Public/www-style/2014Dec/0321.html
...since commit messages are largely for future archeologists, and the spec ED on dev.w3.org could change completely (e.g. maybe the "grid-auto-flow" property gets renamed at the last minute -- stranger things have happened) by the time someone visits that link when poking at your commit message.

(Also: that www-style announcement included a caveat: "Unless the WG objects [...]". I'm not sure how much time needs to pass before we can conclude that the WG doesn't object; I suppose in the unlikely event that "stack" is added back due to an objection, we can just revert this patch.)

>+++ b/layout/style/nsCSSParser.cpp
>@@ -8524,18 +8517,17 @@ CSSParserImpl::ParseGrid()
>   if (!GetToken(true)) {
>     return false;
>   }
> 
>   // The values starts with a <'grid-auto-flow'> if and only if
>   // it starts with a 'stack', 'dense', 'column' or 'row' keyword.
>   if (mToken.mType == eCSSToken_Ident) {
>     nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
>-    if (keyword == eCSSKeyword_stack ||
>-        keyword == eCSSKeyword_dense ||
>+    if (keyword == eCSSKeyword_dense ||

The comment here needs updating to remove 'stack'.

r=me with that comment-fix.
Attachment #8543660 - Flags: review?(simon.sapin) → review+
Comment on attachment 8543660 [details] [diff] [review]
fix

Review of attachment 8543660 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +7659,2 @@
>    if (!(bitField & NS_STYLE_GRID_AUTO_FLOW_ROW ||
> +        bitField & NS_STYLE_GRID_AUTO_FLOW_COLUMN)) {

(Code change needed here.) The new grammar is:

    [ row | column ] || dense

So 'grid-auto-flow: dense;' is acceptable, but would be rejected by this code. On the other hand, this code accepts 'grid-auto-flow: column row;' which should be invalid. So this should be changed to

    if (bitField & NS_STYLE_GRID_AUTO_FLOW_ROW &&
        bitField & NS_STYLE_GRID_AUTO_FLOW_COLUMN) {

(ParseBitmaskValues already returns false when it can not parse at least one keyword.)

@@ -7661,5 @@
> -        bitField & NS_STYLE_GRID_AUTO_FLOW_STACK)) {
> -    return false;
> -  }
> -
> -  // 'stack' without 'row' or 'column' defaults to 'stack row'

(Code change not necessarily needed here.) Note that the spec now says:

> If neither row nor column is provided, row is assumed.

So 'grid-auto-flow: dense;' is identical to 'grid-auto-flow: dense row;', but it’s up to you how you want to represent it internally.

Rather than a bitfield with one bit per keyword, I think it would make sense to have two bits: one for column v.s. row (defaulting to row), and one for sparse packing v.s. dense packing.
Attachment #8543660 - Flags: review+ → review-
Comment on attachment 8543660 [details] [diff] [review]
fix

Review of attachment 8543660 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +7646,5 @@
>    }
>  
>    static const int32_t mask[] = {
>      NS_STYLE_GRID_AUTO_FLOW_ROW | NS_STYLE_GRID_AUTO_FLOW_COLUMN,
> +    NS_STYLE_GRID_AUTO_FLOW_DENSE,

This should be removed: 'mask' (should be plural) is an "array of masks for mutually-exclusive property values" (quoting the ParseBitmaskValues comment) so a mask with only one value does nothing.

@@ +7659,2 @@
>    if (!(bitField & NS_STYLE_GRID_AUTO_FLOW_ROW ||
> +        bitField & NS_STYLE_GRID_AUTO_FLOW_COLUMN)) {

Please disregard my earlier comment here, I misremembered how the 'mask' parameter of ParseBitmaskValues works. 'mask' already takes care of rejecting 'grid-auto-flow: column row;', so this 'if' black can be remove entirely.
[marking my comment 4 as obsolete, since comment 5 expresses the same thing in a more actionable way]
Comment on attachment 8543660 [details] [diff] [review]
fix

Review of attachment 8543660 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, sorry Daniel for the redundant comments/review. It’s twice now that I start typing something up, go do something else, and by the time I hit submit you’ve commented as well :]

::: layout/style/test/property_database.js
@@ +5075,5 @@
>        "column",
>        "column dense",
>        "row dense",
>        "dense column",
>        "dense row",

Please add a value here with just "dense".
(In reply to Simon Sapin (:SimonSapin) from comment #3)
> (Code change needed here.) The new grammar is:
> 
>     [ row | column ] || dense

Good catch, I didn't notice the grammar changed in that way too.
I'll do that change as a separate patch...
Fixing dholbert's nits, carrying r+.
Attachment #8543660 - Attachment is obsolete: true
Attachment #8544040 - Flags: review+
Attachment #8544041 - Flags: review?(simon.sapin) → review+
I think you still need to remove the "NS_STYLE_GRID_AUTO_FLOW_DENSE" entry from the mask[] array, in ParseGridAutoFlow(), as noted at beginning of comment 5 (and the very end of hidden comment 4).

This should probably happen in "part 1", since that array-entry becomes useless as of that patch.
Flags: needinfo?(mats)
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Now that I focus on that code, too, I'm realizing the second entry (for
> "_DENSE") in that mask array is now useless -- that line should just be
> removed.

I think you're mistaken; ParseBitmaskValues accepts zero or one value from
each group, so a single value makes perfect sense.  I think the code:

  static const int32_t mask[] = {
    NS_STYLE_GRID_AUTO_FLOW_ROW | NS_STYLE_GRID_AUTO_FLOW_COLUMN,
    NS_STYLE_GRID_AUTO_FLOW_DENSE,
    MASK_END_VALUE
  };

implements the desired "[ row | column ] || dense" exactly.

If I remove the second group, then we'd have to manually check for
'dense' before the ParseBitmaskValues call, and then again after it,
but only if we didn't already find it before...  which seems 
unnecessary when ParseBitmaskValues can do that for us.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #12)
> I think you're mistaken; ParseBitmaskValues accepts zero or one value from
> each group

I agree that things *do work* if you include this entry, but I'm asserting that things *also* work if you drop the entry. (And perf will be slightly better, since we have one fewer mask-array-entry to iterate through, in MergeBitmaskValue().)  Hence, we should remove it.

> If I remove the second group, then we'd have to manually check for
> 'dense' before the ParseBitmaskValues call, and then again after it,

Why would you need to do that? I think ParseBitmaskValues handles this correctly on its own, by deferring to ParseEnum(), which finds the keyword/bit mapping in kGridAutoFlowKTable, and then MergeBitmaskValue(), which immediately fails if the bit has already been set.

Could you clarify what sorts of values you think would be incorrectly parsed/rejected, if we removed NS_STYLE_GRID_AUTO_FLOW_DENSE from the mask array?  (and/or, what code relies on that mask being present in order for things to parse properly)
Ah, now I see what you mean.  I didn't realize that ParseBitmaskValues accepted
all values from the keyword table unless you mentioned them in the masks somewhere.
I agree it's better without NS_STYLE_GRID_AUTO_FLOW_DENSE then.
Removed the NS_STYLE_GRID_AUTO_FLOW_DENSE line, as requested so r+ implied.
Attachment #8544040 - Attachment is obsolete: true
Attachment #8544216 - Flags: review+
Summary: [css-grid] Remove 'grid-auto-flow: stack' → [css-grid] Remove 'grid-auto-flow: stack' and accept 'dense' by itself meaning 'row dense'
Bah, right file this time...
Attachment #8544216 - Attachment is obsolete: true
Attachment #8544219 - Flags: review+
Looks good -- thanks!
https://hg.mozilla.org/mozilla-central/rev/5f96dee075de
https://hg.mozilla.org/mozilla-central/rev/576c21be1808
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.