921731 - implement the "unset" value from CSS Cascading and Inheritance Level 3

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

"unset", which was previously named "initial-or-inherit", is equivalent to "initial" for a reset property and "inherit" for an inherited property.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

https://tbpl.mozilla.org/?tree=Try&rev=23f59d033287

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 0: Add "layout.css.all-value.enabled" pref.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 2: Parse "unset" in property values whereever "inherit" and "initial" are allowed.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 3: Support eCSSUnit_Unset in nsRuleNode.cpp's SetCoord.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 6: Treat "unset" as "inherit" when determining rule detail for inherited style structs.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 8: Support "unset" in computation of properties.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial".

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Part 10: Modify existing style tests to use "unset".

Comment 13

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=1835823802cb (includes bug 842329 patches too)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811853 [details] [diff] [review]
Part 0: Add "layout.css.all-value.enabled" pref.

r=me if you fix the commit message.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811854 [details] [diff] [review]
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811855 [details] [diff] [review]
Part 2: Parse "unset" in property values whereever "inherit" and "initial" are allowed.

Some of the stuff in ParseFont() is over-parenthesized.

>+  //     so we should check it without the -moz- prefix).

Didn't we generally drop the prefix on "initial"?

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811857 [details] [diff] [review]
Part 3: Support eCSSUnit_Unset in nsRuleNode.cpp's SetCoord.

Again, this stuff seems to be over-parenthesized.

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811858 [details] [diff] [review]
Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete.

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811859 [details] [diff] [review]
Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor.

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811860 [details] [diff] [review]
Part 6: Treat "unset" as "inherit" when determining rule detail for inherited style structs.

>+  if (aSID < nsStyleStructID_Reset_Start) {

 if (!nsCachedStyleData::IsReset(aSID)) {

r=me with that

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811861 [details] [diff] [review]
Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules.

r=me

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 811864 [details] [diff] [review]
Part 10: Modify existing style tests to use "unset".

>+if (SpecialPowers.getBoolPref("layout.css.unset-value.enabled")) {
>+  gCSSProperties["animation-direction"].invalid_values.push("normal, unset");
>+  gCSSProperties["animation-name"].invalid_values.push("bounce, unset", "unset, bounce");
...

What's this bit for?


(I might have preferred the test_*_storage.html modifications instead go in a test_unset_storage, though merging the computation stuff into test_{inherit,initial}_computation.html is good.  It's probably ok as is, though.)

Comment 23

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #22)
> Comment on attachment 811864 [details] [diff] [review]
> Part 10: Modify existing style tests to use "unset".
> 
> >+if (SpecialPowers.getBoolPref("layout.css.unset-value.enabled")) {
> >+  gCSSProperties["animation-direction"].invalid_values.push("normal, unset");
> >+  gCSSProperties["animation-name"].invalid_values.push("bounce, unset", "unset, bounce");
> ...
> 
> What's this bit for?

I just looked at the existing invalid_values that used "initial", and added corresponding values using "unset".

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #23)
> I just looked at the existing invalid_values that used "initial", and added
> corresponding values using "unset".

Oh, right, I wasn't reading the commas and quotes right.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811862 [details] [diff] [review]
Part 8: Support "unset" in computation of properties.

In ComputeColorData, maybe common up the unset case with the currentColor case, assuming you can't just push it down into SetColor?

In ComputeContentData, fix the |// "normal", "none", and "initial" all mean no content| comment?

r=me.  I hope I didn't miss anything, but no guarantees.  I didn't cross-check that you got all the possible places, for one thing.  :(

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811863 [details] [diff] [review]
Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial".

r=me

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811864 [details] [diff] [review]
Part 10: Modify existing style tests to use "unset".

r=me, I think, but would you mind attaching a diff -w of this?

Comment 28

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #25)
> r=me.  I hope I didn't miss anything, but no guarantees.  I didn't
> cross-check that you got all the possible places, for one thing.  :(

The changes to test_inherit_computation.html and test_initial_computation.html should hopefully verify that I've got every property covered.  (It did find a couple of mistaken SETCOORD_UNSET_INITIAL/INHERIT mixups while I was working on it.)

Comment 29

[Cameron McCormack (:heycam)]

Attachment: Part 10 diff with -w

Comment 30

[Boris Zbarsky [:bzbarsky]]

> It did find a couple of mistaken SETCOORD_UNSET_INITIAL/INHERIT mixups 

I'm _pretty_ sure I checked all of those carefully.  What I'd miss is someplace that forgets to add the flag at all.

Comment 31

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42e64d1f0356
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0346f3534e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/da73f1015d88
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5fabafa2b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f28b5b9719bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/6599d8912af7
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdba21420a1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e318646554
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7d0a17817e
https://hg.mozilla.org/integration/mozilla-inbound/rev/22498ff939e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb59e63e61c

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1cb59e63e61c
https://hg.mozilla.org/mozilla-central/rev/22498ff939e0
https://hg.mozilla.org/mozilla-central/rev/bf7d0a17817e
https://hg.mozilla.org/mozilla-central/rev/19e318646554
https://hg.mozilla.org/mozilla-central/rev/cdba21420a1b
https://hg.mozilla.org/mozilla-central/rev/6599d8912af7
https://hg.mozilla.org/mozilla-central/rev/f28b5b9719bd
https://hg.mozilla.org/mozilla-central/rev/1c5fabafa2b6
https://hg.mozilla.org/mozilla-central/rev/da73f1015d88
https://hg.mozilla.org/mozilla-central/rev/f0346f3534e5
https://hg.mozilla.org/mozilla-central/rev/42e64d1f0356

Comment 33

[Manuela Muntean [Away]]

I've looked through the test runs on tbpl containing the tests from the patch of this bug, and they all look ok. 

Are there any other tests not included in this patch, that I should be looking for? Thanks!

Comment 34

[Cameron McCormack (:heycam)]

No other tests for this.

Comment 35

[Manuela Muntean [Away]]

Marking this as verified, based on comment 33 and comment 34.

Comment 36

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/unset
and
https://developer.mozilla.org/en-US/Firefox/Releases/27#CSS