Closed Bug 921731 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
relnote-firefox --- 27+

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [DocArea=CSS])

Attachments

(12 files, 1 obsolete file)

3.01 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.31 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
37.35 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.66 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.56 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
121.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.82 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
47.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
34.48 KB, patch
Details | Diff | Splinter Review
"unset", which was previously named "initial-or-inherit", is equivalent to "initial" for a reset property and "inherit" for an inherited property.
Blocks: 842329
Attachment #811522 - Attachment is obsolete: true
Blocks: css3test
Comment on attachment 811853 [details] [diff] [review]
Part 0: Add "layout.css.all-value.enabled" pref.

r=me if you fix the commit message.
Attachment #811853 - Flags: review?(bzbarsky) → review+
Comment on attachment 811854 [details] [diff] [review]
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.

r=me
Attachment #811854 - Flags: review?(bzbarsky) → review+
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
Attachment #811855 - Flags: review?(bzbarsky) → review+
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
Attachment #811857 - Flags: review?(bzbarsky) → review+
Comment on attachment 811858 [details] [diff] [review]
Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete.

r=me
Attachment #811858 - Flags: review?(bzbarsky) → review+
Comment on attachment 811859 [details] [diff] [review]
Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor.

r=me
Attachment #811859 - Flags: review?(bzbarsky) → review+
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
Attachment #811860 - Flags: review?(bzbarsky) → review+
Comment on attachment 811861 [details] [diff] [review]
Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules.

r=me
Attachment #811861 - Flags: review?(bzbarsky) → review+
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.)
(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".
(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 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.  :(
Attachment #811862 - Flags: review?(bzbarsky) → review+
Comment on attachment 811863 [details] [diff] [review]
Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial".

r=me
Attachment #811863 - Flags: review?(bzbarsky) → review+
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?
Attachment #811864 - Flags: review?(bzbarsky) → review+
(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.)
> 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.
Keywords: feature
Flags: in-testsuite+
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!
QA Contact: manuela.muntean
No other tests for this.
Marking this as verified, based on comment 33 and comment 34.
Status: RESOLVED → VERIFIED
Whiteboard: [DocArea=CSS]
You need to log in before you can comment on or make changes to this bug.