Last Comment Bug 806068 - Unprefix -moz-initial
: Unprefix -moz-initial
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: 19 Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Jet Villegas (:jet)
:
Mentors:
http://www.w3.org/TR/css3-values/#com...
Depends on: 804944
Blocks: unprefix 807184 807303
  Show dependency treegraph
 
Reported: 2012-10-27 02:46 PDT by Jet Villegas (:jet)
Modified: 2012-11-28 15:38 PST (History)
7 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Naive patch that parses the unprefixed keyword. We may want to hang this off of a pref or add an alias to the -moz variant for some period of time. Not sure... (1.01 KB, patch)
2012-10-27 03:03 PDT, Jet Villegas (:jet)
no flags Details | Diff | Splinter Review
part 1: parse 'initial' keyword. maintain '-moz-initial' alias. (12.24 KB, patch)
2012-10-29 03:30 PDT, Jet Villegas (:jet)
dholbert: review+
Details | Diff | Splinter Review
part 2: update in-tree usage of '-moz-initial' to 'initial' (52.05 KB, patch)
2012-10-30 09:05 PDT, Jet Villegas (:jet)
dholbert: review+
Details | Diff | Splinter Review
part 2: update in-tree usage of '-moz-initial' to 'initial' (50.09 KB, patch)
2012-10-30 10:36 PDT, Jet Villegas (:jet)
dholbert: review+
dholbert: checkin+
Details | Diff | Splinter Review
part 1: parse 'initial' keyword. maintain '-moz-initial' alias. (12.45 KB, patch)
2012-10-30 11:41 PDT, Jet Villegas (:jet)
dholbert: checkin+
Details | Diff | Splinter Review

Description Jet Villegas (:jet) 2012-10-27 02:46:04 PDT
Time to start parsing the 'initial' keyword. CSS3 Values & Units is now a Candidate Recommendation. -moz-initial should also be removed from our reftest files so we can add them to the W3C repository.
Comment 1 Jet Villegas (:jet) 2012-10-27 03:03:49 PDT
Created attachment 675822 [details] [diff] [review]
Naive patch that parses the unprefixed keyword. We may want to hang this off of a pref or add an alias to the -moz variant for some period of time. Not sure...
Comment 2 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-27 09:49:47 PDT
Comment on attachment 675822 [details] [diff] [review]
Naive patch that parses the unprefixed keyword. We may want to hang this off of a pref or add an alias to the -moz variant for some period of time. Not sure...

I believe the chunk in the patch doesn't change how we _parse_ the keyword, but rather how we display it when we output it to a string. (e.g. for inspecting a stylesheet via script using the CSSOM, I think)

One nit on that chunk:

>diff --git a/layout/style/nsCSSValue.cpp b/layout/style/nsCSSValue.cpp
>--- a/layout/style/nsCSSValue.cpp
>+++ b/layout/style/nsCSSValue.cpp
>@@ -1066,17 +1066,17 @@ nsCSSValue::AppendToString(nsCSSProperty
>         break;
>     }
>   }
> 
>   switch (unit) {
>     case eCSSUnit_Null:         break;
>     case eCSSUnit_Auto:         aResult.AppendLiteral("auto");     break;
>     case eCSSUnit_Inherit:      aResult.AppendLiteral("inherit");  break;
>-    case eCSSUnit_Initial:      aResult.AppendLiteral("-moz-initial"); break;
>+    case eCSSUnit_Initial:      aResult.AppendLiteral("initial"); break;
>     case eCSSUnit_None:         aResult.AppendLiteral("none");     break;

Probably want to add one space before "break" there to make it line up with all the other already-aligned breaks.

To change how we actually parse it, I think you need to tweak this line of code:
  https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#4718
which uses eCSSKeyword__moz_initial, defined (and mapped to the string "-moz-initial") via a macro here:
  https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSKeywordList.h#93

The exact tweak depends on what we want to do:
 - If we're completely ripping out -moz-initial, then we can just change that line and the keyword-definition.
...OR...:
 - If we want to temporarily support "-moz-initial" as an alias for "initial", then we want to add a new "initial" keyword to nsCSSKeywordList.h, and change that chunk of the parser from...
> else if (eCSSKeyword__moz_initial == keyword) {
...to...
> else if (eCSSKeyword__moz_initial == keyword ||
>          eCSSKeyword__initial     == keyword ) {
Comment 3 Masatoshi Kimura [:emk] 2012-10-27 23:54:46 PDT
We also need to change in-tree users if we drop the prefixed keyword right now.
https://mxr.mozilla.org/mozilla-central/search?string=moz-initial
Comment 4 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-28 11:28:04 PDT
Yup, of course.

If I were doing this, I'd structure this in 3 parts:
* Patch 1: Add "initial" to style system impl (in C++), leaving "-moz-initial" only as an alias, per end of comment 2.
* Patch 2: s/-moz-initial/initial/ on all in-tree content (CSS/HTML/etc)
* Patch 3: Drop the "-moz-initial alias (<--- maybe in a separate bug, later on)

That way, each piece is logically-separate & bite-sized, and we should pass tests at each intermediate stage.
Comment 5 Jet Villegas (:jet) 2012-10-29 03:30:05 PDT
Created attachment 676099 [details] [diff] [review]
part 1: parse 'initial' keyword. maintain '-moz-initial' alias.

This patch:

1. Parses 'initial' keyword. 
2. Adds '-moz-initial' alias. 
3. Returns the string 'initial' when queried in JS. 
4. Modifies test_initial_storage.html that tests #3.
Comment 6 Jet Villegas (:jet) 2012-10-29 06:31:49 PDT
5. Modifies test_bug160403.html that also tests #3.

Try server builds/tests:
https://tbpl.mozilla.org/?tree=Try&rev=91a497934287
Comment 7 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-29 08:46:12 PDT
Comment on attachment 676099 [details] [diff] [review]
part 1: parse 'initial' keyword. maintain '-moz-initial' alias.

Looks good!  r=me
Comment 8 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-30 08:08:13 PDT
Actually, one suggestion for "part 1": If it's easy (which I think it is), we should probably copypaste a couple lines in test_initial_storage.html (one of the patch's modified tests) to be sure that "-moz-initial" gets serialized as "initial".   (i.e. that it's a true alias, rather than an independent keyword with the same effect)

(Of course, we'll drop those added lines in the final "remove-the-alias" patch.  This may be more or less important depending on how long the alias stays in the tree.)
Comment 9 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-30 08:10:23 PDT
(In reply to Daniel Holbert [:dholbert] from comment #8)
> This may be more or less important depending on how long the alias
> stays in the tree.

(If it's not clear: by "this", I meant "adding that new check to test_initial_storage.html")
Comment 10 Jet Villegas (:jet) 2012-10-30 08:23:01 PDT
(In reply to Daniel Holbert [:dholbert] from comment #9)
> (In reply to Daniel Holbert [:dholbert] from comment #8)
> > This may be more or less important depending on how long the alias
> > stays in the tree.
> 
> (If it's not clear: by "this", I meant "adding that new check to
> test_initial_storage.html")

I think lines 63-64 in test_bug160403.html already covers that case. This test sets "-moz-initial" in line 63 but gets "initial" in line 64.
Comment 11 Jet Villegas (:jet) 2012-10-30 08:25:54 PDT
part 2: Modify in-tree usage of -moz-initial

I'm using this script to locate the affected files:

find /src -exec grep -l "\-moz\-initial" '{}' \; 

Here's the output (-files modified by the part 1 patch) :

/src/content/xul/document/crashtests/428951-1.xul
/src/editor/libeditor/html/crashtests/407277-1.html
/src/layout/doc/adding-style-props.html
/src/layout/generic/crashtests/383089-1.html
/src/layout/reftests/bugs/455105-1.html
/src/layout/reftests/css-visited/border-1.html
/src/layout/reftests/css-visited/border-collapse-1.html
/src/layout/reftests/css-visited/color-choice-1.html
/src/layout/reftests/css-visited/column-rule-1.html
/src/layout/reftests/css-visited/content-on-link-before-1.html
/src/layout/reftests/css-visited/content-on-visited-before-1.html
/src/layout/reftests/css-visited/outline-1.html
/src/layout/reftests/flexbox/flexbox-align-self-horiz-1-block.xhtml
/src/layout/reftests/flexbox/flexbox-align-self-horiz-1-table.xhtml
/src/layout/reftests/flexbox/flexbox-align-self-horiz-3.xhtml
/src/layout/reftests/flexbox/flexbox-align-self-vert-1.xhtml
/src/layout/reftests/flexbox/flexbox-align-self-vert-rtl-1.xhtml
/src/layout/style/quirk.css
/src/layout/style/test/property_database.js
/src/layout/style/test/test_bug160403.html
/src/layout/style/test/test_bug652486.html
/src/layout/style/test/test_bug74880.html
/src/layout/style/test/test_garbage_at_end_of_declarations.html
/src/layout/style/test/test_initial_computation.html
/src/layout/style/test/test_initial_storage.html
/src/layout/style/test/test_media_queries.html
/src/layout/style/test/test_media_queries_dynamic.html
/src/layout/style/test/test_media_query_list.html
/src/layout/style/test/test_rem_unit.html
/src/layout/style/test/test_transitions_computed_value_combinations.html
/src/layout/style/test/test_transitions_per_property.html
/src/layout/style/test/test_value_cloning.html
/src/mobile/xul/themes/core/forms.css
/src/mobile/xul/themes/core/gingerbread/forms.css
/src/mobile/xul/themes/core/gingerbread/platform.css
/src/mobile/xul/themes/core/honeycomb/forms.css
/src/mobile/xul/themes/core/honeycomb/platform.css
/src/mobile/xul/themes/core/notification.css
/src/mobile/xul/themes/core/platform.css
/src/mobile/xul/themes/core/tablet.css
Comment 12 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-30 08:37:54 PDT
(In reply to Jet Villegas (:jet) from comment #10)
> I think lines 63-64 in test_bug160403.html already covers that case. This
> test sets "-moz-initial" in line 63 but gets "initial" in line 64.

Ah, so it does. Good good.
Comment 13 Jet Villegas (:jet) 2012-10-30 09:05:13 PDT
Created attachment 676626 [details] [diff] [review]
part 2: update in-tree usage of '-moz-initial' to 'initial'

Applies this script to the 40 files found in comment 11

sed -i '' -e "s/-moz-initial/initial/g" filename
Comment 14 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-30 09:57:05 PDT
Comment on attachment 676626 [details] [diff] [review]
part 2: update in-tree usage of '-moz-initial' to 'initial'

Looks good!

One possible tweak: if we're keeping the alias around for a nontrivial amount of time, it'd probably be a good idea to leave test_bug160403.html untouched by this patch, so that we've got at least one test to verify that the prefixed alias behaves as-expected while we support it. (per comment 8 / comment 10)
Comment 15 Jet Villegas (:jet) 2012-10-30 10:36:33 PDT
Created attachment 676667 [details] [diff] [review]
part 2: update in-tree usage of '-moz-initial' to 'initial'

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 676626 [details] [diff] [review]

> One possible tweak: if we're keeping the alias around for a nontrivial
> amount of time, it'd probably be a good idea to leave test_bug160403.html
> untouched by this patch, so that we've got at least one test to verify that
> the prefixed alias behaves as-expected while we support it. (per comment 8 /
> comment 10)

Agreed. Updated patch to unfix that file.
Comment 16 Jet Villegas (:jet) 2012-10-30 11:41:48 PDT
Created attachment 676690 [details] [diff] [review]
part 1: parse 'initial' keyword. maintain '-moz-initial' alias.

Updated part 1 patch to add a check-in comment. No code changes. r = dholbert. checkin-needed
Comment 17 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-30 17:45:33 PDT
Comment on attachment 676690 [details] [diff] [review]
part 1: parse 'initial' keyword. maintain '-moz-initial' alias.

(In reply to Jet Villegas (:jet) from comment #16)
> Updated part 1 patch to add a check-in comment.

For someone unfamiliar with this bug, the checkin comment...
>Bug 806068: Parser Changes.
... is a little vague. :)

Anyway -- I updated it (and added "part 1" / "part 2" labels on the patches) and pushed both parts.

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/017802692817
Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e7c99e44c65
Comment 18 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-10-30 17:53:37 PDT
I filed bug 807184 on this.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-11-12 14:02:04 PST
https://hg.mozilla.org/comm-central/rev/5b72098e0d5d
Comment 22 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2012-11-28 15:38:02 PST
(In reply to Ryan VanderMeulen from comment #21)
> https://hg.mozilla.org/comm-central/rev/5b72098e0d5d

That was actually for bug 807303.  (The patch had the wrong bug #)

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