842329 - [css3-cascade] implement the 'all' shorthand

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Jesse Ruderman]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://dev.w3.org/csswg/css3-cascade/#all-shorthand would fix this

Comment 2

[Justin Dolske [:Dolske]]

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.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

https://tbpl.mozilla.org/?tree=Try&rev=9f062c5ac291

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 0: Add "layout.css.all-shorthand.enabled" pref.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add "all" shorthand property.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 2: Parse the "all" shorthand property.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 3: Serialize the "all" shorthand property as the empty string unless all components are inherit/initial/unset.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 4: Test.

Comment 9

[Cameron McCormack (:heycam)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Cameron McCormack (:heycam)]

(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 "".

Comment 16

[Cameron McCormack (:heycam)]

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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 19

[Cameron McCormack (:heycam)]

OK.  I'll keep the pref for the moment but not conditionalise it on RELEASE_BUILD in all.js.

Comment 20

[Cameron McCormack (:heycam)]

(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.

Comment 21

[Cameron McCormack (:heycam)]

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

Comment 22

[Phil Ringnalda (:philor)]

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

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

Comment 24

[Cameron McCormack (:heycam)]

(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.

Comment 25

[Manuela Muntean [Away]]

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 26

[Cameron McCormack (:heycam)]

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

Comment 27

[Manuela Muntean [Away]]

Marking this as verified, based on comment 25 and comment 26.

Comment 28

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/27
https://developer.mozilla.org/en-US/docs/Web/CSS/all