Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[css3-cascade] implement the 'all' shorthand

VERIFIED FIXED in mozilla27

Status

()

Core
CSS Parsing and Computation
--
enhancement
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: heycam)

Tracking

(Blocks: 1 bug, {dev-doc-complete, feature})

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

Firefox Tracking Flags

(relnote-firefox 27+)

Details

(Whiteboard: [DocArea=CSS])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
In bug 807880, CtP tries to avoid inheriting page styles by specifying "initial" for all the properties they could think of.  This isn't robust or future-proof.
http://dev.w3.org/csswg/css3-cascade/#all-shorthand would fix this
Summary: XBL should have a way to inherit no styles → [css3-cascade] implement the 'all' shorthand
This has also been an issue for video controls. It would be most handy to have this capability, I've seen the 'all' spec before and it sounds perfect.

Updated

4 years ago
Blocks: 913153
(Assignee)

Comment 3

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

https://tbpl.mozilla.org/?tree=Try&rev=9f062c5ac291
Assignee: nobody → cam
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 921731
(Assignee)

Updated

4 years ago
Depends on: 921797
(Assignee)

Comment 4

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

Comment 5

4 years ago
Created attachment 811866 [details] [diff] [review]
Part 1: Add "all" shorthand property.
Attachment #811866 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
Created attachment 811867 [details] [diff] [review]
Part 2: Parse the "all" shorthand property.
Attachment #811867 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

4 years ago
Created attachment 811868 [details] [diff] [review]
Part 3: Serialize the "all" shorthand property as the empty string unless all components are inherit/initial/unset.
Attachment #811868 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

4 years ago
Created attachment 811869 [details] [diff] [review]
Part 4: Test.
Attachment #811869 - Flags: review?(bzbarsky)
(Assignee)

Updated

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

Comment 9

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

Comment 10

4 years ago
Comment on attachment 811865 [details] [diff] [review]
Part 0: Add "layout.css.all-shorthand.enabled" pref.

r=me
Attachment #811865 - Flags: review?(bzbarsky) → review+

Comment 11

4 years ago
Comment on attachment 811866 [details] [diff] [review]
Part 1: Add "all" shorthand property.

Maybe call this macro CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND ?

>+#endif

I'd appreciate a "// !defined(CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND)" after this.

Why are some internal props excluded and some not?

r=me with the above addressed
Attachment #811866 - Flags: review?(bzbarsky) → review+

Comment 12

4 years ago
Comment on attachment 811867 [details] [diff] [review]
Part 2: Parse the "all" shorthand property.

r=me
Attachment #811867 - Flags: review?(bzbarsky) → review+

Comment 13

4 years ago
Comment on attachment 811868 [details] [diff] [review]
Part 3: Serialize the "all" shorthand property as the empty string unless all components are inherit/initial/unset.

The code and commit message don't agree...  Seems to me like this _always_ serializes "all" as the empty string, no?

r=me with (presumably) the comment comment fixed.
Attachment #811868 - Flags: review?(bzbarsky) → review+

Comment 14

4 years ago
Comment on attachment 811869 [details] [diff] [review]
Part 4: Test.

> +function set(array) {

Why not use actual JS Set() here?

>+  var initialComputedValues = { };
>+  var otherComputedValues = { };

And Map() here?

>+  // Test setting all:inherit through setProperty.
...
>+    parentRule.style.setProperty(prop, info.other_values[0], "");
>+    childRule2.style.setProperty("all", "inherit");

I think you should childRule1.style.setProperty(prop, "initial") here.  Otherwise you could pass the test for inherited properties even if "all: inherit" did absolutely nothing.

>+  // Test setting all:initial through setProperty.
...
>+    parentRule.style.setProperty(prop, info.other_values[0], "");
>+    childRule2.style.setProperty("all", "initial");

And childRule1.style.setProperty(prop, "inherit") here, or info.other_values[0] instead of "inherit", for the same reason but for reset properties.

r=me with those last two comments fixed.  Either way on Map and Set, but seems cleaner to use those, in general.
Attachment #811869 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #11)
> Why are some internal props excluded and some not?

I found that doing

  button { all: unset; }

would still result in some borders being painted if I excluded the internal border properties, not exactly sure why though.

(In reply to Boris Zbarsky [:bz] from comment #13)
> The code and commit message don't agree...  Seems to me like this _always_
> serializes "all" as the empty string, no?

I should just update the comment.  Earlier in the function we serialise to "inherit", "initial" or "unset", if all of the longhand components are one of those values.  In all other cases we serialise to "".
(Assignee)

Comment 16

4 years ago
I notice that CSS Cascading and Inheritance Level 3, where this (and "unset" from bug 921731) are defined, is in Candidate Recommendation.  Does that means I don't need to hide this behind an off-for-release-builds pref?
Flags: needinfo?(bzbarsky)

Comment 17

4 years ago
> I found that doing

So I think the key is that some props are internal in the sense that pages can't set them directly, but are actually set by exposed shorthand props, right?  The definition of the macro should probably document this, so people adding new props know when to use it or not.

> Does that means I don't need to hide this behind an off-for-release-builds pref?

That's a question for dbaron; I haven't been following the WG's work that closely.
Flags: needinfo?(bzbarsky) → needinfo?(dbaron)
(In reply to Boris Zbarsky [:bz] from comment #17)
> > Does that means I don't need to hide this behind an off-for-release-builds pref?
> 
> That's a question for dbaron; I haven't been following the WG's work that
> closely.

Yeah, I think we're fine to ship this.  That's generally the case for CRs, though there might be a few ancient ones where that's not the case.
Flags: needinfo?(dbaron)
(Assignee)

Comment 19

4 years ago
OK.  I'll keep the pref for the moment but not conditionalise it on RELEASE_BUILD in all.js.
(Assignee)

Comment 20

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #17)
> So I think the key is that some props are internal in the sense that pages
> can't set them directly, but are actually set by exposed shorthand props,
> right?  The definition of the macro should probably document this, so people
> adding new props know when to use it or not.

That makes sense.  The UA style sheet sets the internal property via the shorthand, and unset:all only targets longhands.
(Assignee)

Comment 21

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08adaae0322
https://hg.mozilla.org/integration/mozilla-inbound/rev/998003ad5546
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5c63580196
https://hg.mozilla.org/integration/mozilla-inbound/rev/3139f05cbe1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/60c43e6a1489
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/60c43e6a1489
https://hg.mozilla.org/mozilla-central/rev/3139f05cbe1d
https://hg.mozilla.org/mozilla-central/rev/ff5c63580196
https://hg.mozilla.org/mozilla-central/rev/998003ad5546
https://hg.mozilla.org/mozilla-central/rev/a08adaae0322
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #11)
> Comment on attachment 811866 [details] [diff] [review]
> Part 1: Add "all" shorthand property.
> 
> Maybe call this macro CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND ?

> Why are some internal props excluded and some not?

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #17)
> So I think the key is that some props are internal in the sense that pages
> can't set them directly, but are actually set by exposed shorthand props,
> right?  The definition of the macro should probably document this, so people
> adding new props know when to use it or not.

It seems like the key distinction is that:
 * internal properties that are set indirectly by some other shorthand (but used to internally divide it up) should be part of 'all'
 * internal properties that represent things that aren't supposed to be part of CSS, but that we implement as part of CSS, should not be part of 'all'

By this logic, I see one mistake:  -x-system-font should be part of the 'all' shorthand.  It represents the part of the 'font' shorthand that is not represented by any of the subproperties.

heycam, any chance you could:
 * extend the comment to reflect this distinction, and
 * fix -x-system-font
Flags: needinfo?(cam)

Updated

4 years ago
Depends on: 924885
(Assignee)

Comment 24

4 years ago
(In reply to David Baron [:dbaron] (needinfo? me) from comment #23)
> heycam, any chance you could:
>  * extend the comment to reflect this distinction, and
>  * fix -x-system-font

Yes, filed bug 925218 for it.
Flags: needinfo?(cam)
(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 26

4 years ago
No, that's all the tests for 'all' so far.

Updated

4 years ago
relnote-firefox: ? → 27+
Marking this as verified, based on comment 25 and comment 26.
Status: RESOLVED → VERIFIED
Whiteboard: [DocArea=CSS]
Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/27
https://developer.mozilla.org/en-US/docs/Web/CSS/all
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.