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)
: Manuela Muntean [Away]
: Jet Villegas (:jet)
Mentors:
http://dev.w3.org/csswg/css-cascade/#...
Depends on:
Blocks: css3test 1211330 css-cascade-3 842329
  Show dependency treegraph
 
Reported: 2013-09-28 06:55 PDT by Cameron McCormack (:heycam)
Modified: 2016-10-24 18:35 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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
bzbarsky: review+
Details | Diff | Splinter Review
Part 10 diff with -w (34.48 KB, patch)
2013-10-01 21:36 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review

Description Cameron McCormack (:heycam) 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) 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) 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) 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) 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) 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) 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) 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) 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) 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) 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) 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) 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) 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-10 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) 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-10 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) 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) 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) 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.