Jet Villegas (inactive) Assignee
	Description • 12 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 2 • 12 years ago

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 ) {

	Masatoshi Kimura [:emk]
	Comment 3 • 12 years ago

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

	Daniel Holbert [:dholbert]
	Comment 4 • 12 years ago

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.

	Jet Villegas (inactive) Assignee
	Comment 5 • 12 years ago

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.

	Jet Villegas (inactive) Assignee
	Comment 6 • 12 years ago

5. Modifies test_bug160403.html that also tests #3.

Try server builds/tests:
https://tbpl.mozilla.org/?tree=Try&rev=91a497934287

	Daniel Holbert [:dholbert]
	Comment 7 • 12 years ago

Comment on attachment 676099 [details] [diff] [review]
part 1: parse 'initial' keyword. maintain '-moz-initial' alias.

Looks good!  r=me

	Daniel Holbert [:dholbert]
	Comment 8 • 12 years ago

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

	Daniel Holbert [:dholbert]
	Comment 9 • 12 years ago

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

	Jet Villegas (inactive) Assignee
	Comment 10 • 12 years ago

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

	Jet Villegas (inactive) Assignee
	Comment 11 • 12 years ago

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

	Daniel Holbert [:dholbert]
	Comment 12 • 12 years ago

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

	Jet Villegas (inactive) Assignee
	Comment 13 • 12 years ago

Applies this script to the 40 files found in comment 11

sed -i '' -e "s/-moz-initial/initial/g" filename

	Daniel Holbert [:dholbert]
	Comment 14 • 12 years ago

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)

	Jet Villegas (inactive) Assignee
	Comment 15 • 12 years ago

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

	Jet Villegas (inactive) Assignee
	Comment 16 • 12 years ago

Updated part 1 patch to add a check-in comment. No code changes. r = dholbert. checkin-needed

	Daniel Holbert [:dholbert]
	Comment 17 • 12 years ago

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

	Daniel Holbert [:dholbert]
	Comment 18 • 12 years ago

I filed bug 807184 on this.

	Ed Morley [:emorley]
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/017802692817
https://hg.mozilla.org/mozilla-central/rev/4e7c99e44c65

	Jean-Yves Perrier [:teoli]
	Comment 20 • 12 years ago

Both: https://developer.mozilla.org/en-US/docs/CSS/initial and https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers

have been updated.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 12 years ago

https://hg.mozilla.org/comm-central/rev/5b72098e0d5d

	Daniel Holbert [:dholbert]
	Comment 22 • 12 years ago

(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 #)