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
8 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
(Assignee)

Description

4 years ago
"unset", which was previously named "initial-or-inherit", is equivalent to "initial" for a reset property and "inherit" for an inherited property.
(Assignee)

Comment 1

4 years ago
Created attachment 811522 [details] [diff] [review]
WIP (v0.1)

https://tbpl.mozilla.org/?tree=Try&rev=23f59d033287
(Assignee)

Updated

4 years ago
(Assignee)

Updated

4 years ago
Blocks: 842329
(Assignee)

Comment 2

4 years ago
Created attachment 811853 [details] [diff] [review]
Part 0: Add "layout.css.all-value.enabled" pref.
Attachment #811853 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

4 years ago
Created attachment 811854 [details] [diff] [review]
Part 1: Add new eCSSUnit_Unset unit for nsCSSValues.
Attachment #811854 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

4 years ago
Created attachment 811855 [details] [diff] [review]
Part 2: Parse "unset" in property values whereever "inherit" and "initial" are allowed.
Attachment #811855 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

4 years ago
Created attachment 811857 [details] [diff] [review]
Part 3: Support eCSSUnit_Unset in nsRuleNode.cpp's SetCoord.
Attachment #811857 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
Created attachment 811858 [details] [diff] [review]
Part 4: Support eCSSUnit_Unset in nsRuleNode.cpp's SetDiscrete.
Attachment #811858 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

4 years ago
Created attachment 811859 [details] [diff] [review]
Part 5: Support eCSSUnit_Unset in nsRuleNode.cpp's SetFactor.
Attachment #811859 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

4 years ago
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)
(Assignee)

Comment 9

4 years ago
Created attachment 811861 [details] [diff] [review]
Part 7: Treat "unset" on inherited properties like "inherit" in nsRuleNode::HasAuthorSpecifiedRules.
Attachment #811861 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

4 years ago
Created attachment 811862 [details] [diff] [review]
Part 8: Support "unset" in computation of properties.
Attachment #811862 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

4 years ago
Created attachment 811863 [details] [diff] [review]
Part 9: Serialize shorthands using "unset" like those containing "inherit" or "initial".
Attachment #811863 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

4 years ago
Created attachment 811864 [details] [diff] [review]
Part 10: Modify existing style tests to use "unset".
Attachment #811864 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #811522 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=1835823802cb (includes bug 842329 patches too)
(Assignee)

Updated

4 years ago
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.)
(Assignee)

Comment 23

4 years ago
(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+
(Assignee)

Comment 28

4 years ago
(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.)
(Assignee)

Comment 29

4 years ago
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.
(Assignee)

Comment 31

4 years ago
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
(Assignee)

Updated

4 years ago
Keywords: feature
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 34

4 years ago
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

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