Last Comment Bug 842329 - [css3-cascade] implement the 'all' shorthand
: [css3-cascade] implement the 'all' shorthand
Status: VERIFIED FIXED
[DocArea=CSS]
: dev-doc-complete, feature
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla27
Assigned To: Cameron McCormack (:heycam)
: Manuela Muntean [Away]
Mentors:
Depends on: 921731 921797 924885
Blocks: css3test
  Show dependency treegraph
 
Reported: 2013-02-18 09:12 PST by Jesse Ruderman
Modified: 2014-09-12 05:11 PDT (History)
14 users (show)
cam: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
27+


Attachments
WIP (v0.1) (12.34 KB, patch)
2013-09-29 01:43 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
Part 0: Add "layout.css.all-shorthand.enabled" pref. (1.10 KB, patch)
2013-09-30 00:22 PDT, Cameron McCormack (:heycam)
bzbarsky: review+
Details | Diff | Splinter Review
Part 1: Add "all" shorthand property. (8.42 KB, patch)
2013-09-30 00:22 PDT, Cameron McCormack (:heycam)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Parse the "all" shorthand property. (2.28 KB, patch)
2013-09-30 00:23 PDT, Cameron McCormack (:heycam)
bzbarsky: review+
Details | Diff | Splinter Review
Part 3: Serialize the "all" shorthand property as the empty string unless all components are inherit/initial/unset. (1.13 KB, patch)
2013-09-30 00:23 PDT, Cameron McCormack (:heycam)
bzbarsky: review+
Details | Diff | Splinter Review
Part 4: Test. (7.64 KB, patch)
2013-09-30 00:23 PDT, Cameron McCormack (:heycam)
bzbarsky: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2013-02-18 09:12:33 PST
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+2 (mostly busy through August 4; review requests must explain patch) 2013-02-18 11:59:44 PST
http://dev.w3.org/csswg/css3-cascade/#all-shorthand would fix this
Comment 2 Justin Dolske [:Dolske] 2013-02-18 21:57:30 PST
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) 2013-09-29 01:43:29 PDT
Created attachment 811655 [details] [diff] [review]
WIP (v0.1)

https://tbpl.mozilla.org/?tree=Try&rev=9f062c5ac291
Comment 4 Cameron McCormack (:heycam) 2013-09-30 00:22:36 PDT
Created attachment 811865 [details] [diff] [review]
Part 0: Add "layout.css.all-shorthand.enabled" pref.
Comment 5 Cameron McCormack (:heycam) 2013-09-30 00:22:50 PDT
Created attachment 811866 [details] [diff] [review]
Part 1: Add "all" shorthand property.
Comment 6 Cameron McCormack (:heycam) 2013-09-30 00:23:02 PDT
Created attachment 811867 [details] [diff] [review]
Part 2: Parse the "all" shorthand property.
Comment 7 Cameron McCormack (:heycam) 2013-09-30 00:23:15 PDT
Created attachment 811868 [details] [diff] [review]
Part 3: Serialize the "all" shorthand property as the empty string unless all components are inherit/initial/unset.
Comment 8 Cameron McCormack (:heycam) 2013-09-30 00:23:28 PDT
Created attachment 811869 [details] [diff] [review]
Part 4: Test.
Comment 9 Cameron McCormack (:heycam) 2013-09-30 00:24:34 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1835823802cb (includes bug 921731 patches too)
Comment 10 Boris Zbarsky [:bz] 2013-09-30 14:02:53 PDT
Comment on attachment 811865 [details] [diff] [review]
Part 0: Add "layout.css.all-shorthand.enabled" pref.

r=me
Comment 11 Boris Zbarsky [:bz] 2013-09-30 14:10:19 PDT
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 [:bz] 2013-09-30 14:11:51 PDT
Comment on attachment 811867 [details] [diff] [review]
Part 2: Parse the "all" shorthand property.

r=me
Comment 13 Boris Zbarsky [:bz] 2013-09-30 14:13:34 PDT
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 [:bz] 2013-09-30 14:27:34 PDT
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) 2013-09-30 23:20:06 PDT
(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) 2013-09-30 23:23:19 PDT
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 [:bz] 2013-10-01 06:25:51 PDT
> 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+2 (mostly busy through August 4; review requests must explain patch) 2013-10-02 15:01:23 PDT
(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) 2013-10-02 22:57:41 PDT
OK.  I'll keep the pref for the moment but not conditionalise it on RELEASE_BUILD in all.js.
Comment 20 Cameron McCormack (:heycam) 2013-10-03 06:07:47 PDT
(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 23 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-10-08 21:36:19 PDT
(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) 2013-10-09 18:30:09 PDT
(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] 2013-10-25 04:56:40 PDT
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) 2013-10-25 05:00:54 PDT
No, that's all the tests for 'all' so far.
Comment 27 Manuela Muntean [Away] 2013-11-04 00:13:20 PST
Marking this as verified, based on comment 25 and comment 26.

Note You need to log in before you can comment on or make changes to this bug.