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

VERIFIED FIXED in mozilla27

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
4 years ago
6 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 4 bugs, {dev-doc-complete, feature})

unspecified
mozilla27
dev-doc-complete, feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 27+)

Details

(Whiteboard: [DocArea=CSS], URL)

Attachments

(12 attachments, 1 obsolete attachment)

3.01 KB, patch
Details | Diff | Splinter Review
10.31 KB, patch
Details | Diff | Splinter Review
37.35 KB, patch
Details | Diff | Splinter Review
4.43 KB, patch
Details | Diff | Splinter Review
1.75 KB, patch
Details | Diff | Splinter Review
1.66 KB, patch
Details | Diff | Splinter Review
3.08 KB, patch
Details | Diff | Splinter Review
2.56 KB, patch
Details | Diff | Splinter Review
121.58 KB, patch
Details | Diff | Splinter Review
3.82 KB, patch
Details | Diff | Splinter Review
47.95 KB, patch
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.
Created attachment 811522 [details] [diff] [review]
WIP (v0.1)

https://tbpl.mozilla.org/?tree=Try&rev=23f59d033287
Blocks: 842329
Created attachment 811853 [details] [diff] [review]
Part 0: Add "layout.css.all-value.enabled" pref.
Attachment #811853 - Flags: review?(bzbarsky)
Created attachment 811854 [details] [diff] [review]
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.
Attachment #811854 - Flags: review?(bzbarsky)
Created attachment 811855 [details] [diff] [review]
Part 2: Parse "unset" in property values whereever "inherit" and "initial" are allowed.
Attachment #811855 - Flags: review?(bzbarsky)
Created attachment 811857 [details] [diff] [review]
Part 3: Support eCSSUnit_Unset in nsRuleNode.cpp's SetCoord.
Attachment #811857 - Flags: review?(bzbarsky)
Created attachment 811858 [details] [diff] [review]
Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete.
Attachment #811858 - Flags: review?(bzbarsky)
Created attachment 811859 [details] [diff] [review]
Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor.
Attachment #811859 - Flags: review?(bzbarsky)
Created attachment 811860 [details] [diff] [review]
Part 6: Treat "unset" as "inherit" when determining rule detail for inherited style structs.
Attachment #811860 - Flags: review?(bzbarsky)
Created attachment 811861 [details] [diff] [review]
Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules.
Attachment #811861 - Flags: review?(bzbarsky)
Created attachment 811862 [details] [diff] [review]
Part 8: Support "unset" in computation of properties.
Attachment #811862 - Flags: review?(bzbarsky)
Created attachment 811863 [details] [diff] [review]
Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial".
Attachment #811863 - Flags: review?(bzbarsky)
Created attachment 811864 [details] [diff] [review]
Part 10: Modify existing style tests to use "unset".
Attachment #811864 - Flags: review?(bzbarsky)
Attachment #811522 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=1835823802cb (includes bug 842329 patches too)
Blocks: 913153
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.)
Created attachment 812967 [details] [diff] [review]
Part 10 diff with -w
> 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.
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
Keywords: dev-doc-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: feature
Flags: in-testsuite+
relnote-firefox: --- → ?
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.

Updated

4 years ago
relnote-firefox: ? → 27+
Marking this as verified, based on comment 33 and comment 34.
Status: RESOLVED → VERIFIED
Whiteboard: [DocArea=CSS]
Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/unset
and
https://developer.mozilla.org/en-US/Firefox/Releases/27#CSS
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1211330

Updated

8 months ago
Blocks: 1312619
Blocks: 1324601
You need to log in before you can comment on or make changes to this bug.