Last Comment Bug 921731 - implement the "unset" value from CSS Cascading and Inheritance Level 3
: implement the "unset" value from CSS Cascading and Inheritance Level 3
Status: VERIFIED FIXED
[DocArea=CSS]
: dev-doc-complete, feature
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla27
Assigned To: Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
: Manuela Muntean [Away]
: Jet Villegas (:jet)
Mentors:
http://dev.w3.org/csswg/css-cascade/#...
Depends on:
Blocks: css3test 1211330 css-cascade-3 1324601 842329
  Show dependency treegraph
 
Reported: 2013-09-28 06:55 PDT by Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
Modified: 2016-12-19 16:23 PST (History)
6 users (show)
cam: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
27+


Attachments
WIP (v0.1) (241.08 KB, patch)
2013-09-28 07:02 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
no flags Details | Diff | Splinter Review
Part 0: Add "layout.css.all-value.enabled" pref. (3.01 KB, patch)
2013-09-30 00:15 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues. (10.31 KB, patch)
2013-09-30 00:16 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Parse "unset" in property values whereever "inherit" and "initial" are allowed. (37.35 KB, patch)
2013-09-30 00:16 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 3: Support eCSSUnit_Unset in nsRuleNode.cpp's SetCoord. (4.43 KB, patch)
2013-09-30 00:17 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete. (1.75 KB, patch)
2013-09-30 00:17 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor. (1.66 KB, patch)
2013-09-30 00:17 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 6: Treat "unset" as "inherit" when determining rule detail for inherited style structs. (3.08 KB, patch)
2013-09-30 00:17 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules. (2.56 KB, patch)
2013-09-30 00:18 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 8: Support "unset" in computation of properties. (121.58 KB, patch)
2013-09-30 00:18 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial". (3.82 KB, patch)
2013-09-30 00:18 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 10: Modify existing style tests to use "unset". (47.95 KB, patch)
2013-09-30 00:18 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
bzbarsky: review+
Details | Diff | Splinter Review
Part 10 diff with -w (34.48 KB, patch)
2013-10-01 21:36 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
no flags Details | Diff | Splinter Review

Description User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-28 06:55:53 PDT
"unset", which was previously named "initial-or-inherit", is equivalent to "initial" for a reset property and "inherit" for an inherited property.
Comment 1 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-28 07:02:09 PDT
Created attachment 811522 [details] [diff] [review]
WIP (v0.1)

https://tbpl.mozilla.org/?tree=Try&rev=23f59d033287
Comment 2 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:15:46 PDT
Created attachment 811853 [details] [diff] [review]
Part 0: Add "layout.css.all-value.enabled" pref.
Comment 3 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:16:46 PDT
Created attachment 811854 [details] [diff] [review]
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.
Comment 4 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:16:59 PDT
Created attachment 811855 [details] [diff] [review]
Part 2: Parse "unset" in property values whereever "inherit" and "initial" are allowed.
Comment 5 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:17:12 PDT
Created attachment 811857 [details] [diff] [review]
Part 3: Support eCSSUnit_Unset in nsRuleNode.cpp's SetCoord.
Comment 6 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:17:23 PDT
Created attachment 811858 [details] [diff] [review]
Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete.
Comment 7 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:17:35 PDT
Created attachment 811859 [details] [diff] [review]
Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor.
Comment 8 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:17:47 PDT
Created attachment 811860 [details] [diff] [review]
Part 6: Treat "unset" as "inherit" when determining rule detail for inherited style structs.
Comment 9 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:18:00 PDT
Created attachment 811861 [details] [diff] [review]
Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules.
Comment 10 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:18:14 PDT
Created attachment 811862 [details] [diff] [review]
Part 8: Support "unset" in computation of properties.
Comment 11 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:18:27 PDT
Created attachment 811863 [details] [diff] [review]
Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial".
Comment 12 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:18:40 PDT
Created attachment 811864 [details] [diff] [review]
Part 10: Modify existing style tests to use "unset".
Comment 13 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-09-30 00:21:50 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1835823802cb (includes bug 842329 patches too)
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 19:30:14 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 20:11:51 PDT
Comment on attachment 811854 [details] [diff] [review]
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.

r=me
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 20:20:22 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 20:22:36 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 20:23:18 PDT
Comment on attachment 811858 [details] [diff] [review]
Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete.

r=me
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 20:24:07 PDT
Comment on attachment 811859 [details] [diff] [review]
Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor.

r=me
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 20:25:45 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 20:26:35 PDT
Comment on attachment 811861 [details] [diff] [review]
Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules.

r=me
Comment 22 User image David Baron :dbaron: ⌚️UTC-8 2013-10-01 21:06:44 PDT
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 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-10-01 21:09:55 PDT
(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 User image David Baron :dbaron: ⌚️UTC-8 2013-10-01 21:15:17 PDT
(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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 21:27:07 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 21:28:03 PDT
Comment on attachment 811863 [details] [diff] [review]
Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial".

r=me
Comment 27 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 21:32:27 PDT
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 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-10-01 21:34:40 PDT
(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 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-10-01 21:36:31 PDT
Created attachment 812967 [details] [diff] [review]
Part 10 diff with -w
Comment 30 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 21:44:16 PDT
> 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 33 User image Manuela Muntean [Away] 2013-10-25 04:57:04 PDT
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 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2013-10-25 05:01:18 PDT
No other tests for this.
Comment 35 User image Manuela Muntean [Away] 2013-11-04 00:13:41 PST
Marking this as verified, based on comment 33 and comment 34.

Note You need to log in before you can comment on or make changes to this bug.