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 Sep 28)
: Manuela Muntean [Away]
:
Mentors:
http://dev.w3.org/csswg/css-cascade/#...
Depends on:
Blocks: css3test 1211330 842329
  Show dependency treegraph
 
Reported: 2013-09-28 06:55 PDT by Cameron McCormack (:heycam) (away Sep 28)
Modified: 2015-10-04 21:18 PDT (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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
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 Sep 28)
no flags Details | Diff | Splinter Review

Description Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 2013-09-30 00:15:46 PDT
Created attachment 811853 [details] [diff] [review]
Part 0: Add "layout.css.all-value.enabled" pref.
Comment 3 Cameron McCormack (:heycam) (away Sep 28) 2013-09-30 00:16:46 PDT
Created attachment 811854 [details] [diff] [review]
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.
Comment 4 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 2013-09-30 00:18:14 PDT
Created attachment 811862 [details] [diff] [review]
Part 8: Support "unset" in computation of properties.
Comment 11 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 2013-09-30 00:18:40 PDT
Created attachment 811864 [details] [diff] [review]
Part 10: Modify existing style tests to use "unset".
Comment 13 Cameron McCormack (:heycam) (away Sep 28) 2013-09-30 00:21:50 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1835823802cb (includes bug 842329 patches too)
Comment 14 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 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 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 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 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 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 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 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 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 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 Cameron McCormack (:heycam) (away Sep 28) 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 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 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 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 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 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 Cameron McCormack (:heycam) (away Sep 28) 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 Cameron McCormack (:heycam) (away Sep 28) 2013-10-01 21:36:31 PDT
Created attachment 812967 [details] [diff] [review]
Part 10 diff with -w
Comment 30 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 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 Cameron McCormack (:heycam) (away Sep 28) 2013-10-25 05:01:18 PDT
No other tests for this.
Comment 35 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.