1211101 - Extend style system mochitests to test (& deal with) -webkit aliases

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug on getting bug 837211's -webkit prefixed property aliases into our mochitests.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: fixup existing tests so that they won't break when -webkit aliases appear

The first thing we need to do here is fix up existing tests, so that they'll still work when -webkit prefixed aliases appear.

Explanations of the changes here:
=================================
 - test_bug1112014.html: This test sets up some exemptions for certain properties.  My patch adds the -webkit aliased versions of those properties.

 - test_property_database.html: This test checks (among other things) that DOM accessors all follow a particular pattern, in particular that "foo-bar" ends up looking like "fooBar" and "-foo-bar" ends up "FooBar".  However, webkit-prefixed properties (unlike moz-prefixed properties) name their DOM accessors with a lowercase "w".  My patch adds a special case for these properties to lowercase their first letter when generating the expected DOM accessor.

 - test_unprefixing_service.html: This test checks that -webkit aliases are supported [via the prefixing service] on whitelisted domains, and not supported on other domains.  Once we enable native -webkit aliases, the test's whitelist-testing breaks (because our native support is universal, without domain whitelisting).  So, I'm turning off the native support for this test.  (This test ultimately will probably want to be removed, along with the CSSUnprefixingService itself.)

 - test_value_computation.html: As above, my patch just copies some existing property special-cases in this test to also apply to -webkit prefixed aliases.

 - test_value_storage.html: This test has the following enigmatic check:
     sh.replace("-moz-","") == property.replace("-moz-","")
...which I think is just trying to test whether sh & property are aliases of each other.  My patch refactors this out into a helper function and uses the "alias_for" field to make the check more explicit & robust.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 2: add webkit aliases to property_database.js

With those fixes, this part:
 (1) Adds the aliases (from bug 837211) to property_database.js, if the pref is enabled.
 (2) Enables the pref during mochitests (as we tend to for in-progress css features).

This patch benefits from my cleanup in bug 1210905 -- happily, none of the aliases here need to specify *_values or prerequisites fields, because those fields will now be cloned from their alias targets.

In cases where we have equivalent -moz-prefixed aliases, I just copypasted those aliases and replaced "moz" with "webkit", basically. (With one exception -- I didn't copy our -moz-transform "valid only when prefixed" quirk. I'm letting -webkit-transform be a true alias for transform, which it seems like it should per bug 774169 comment 24 - bug 774169 comment 25).

And in cases where we don't have a moz alias to crib from, I just copied data from the alias target, and tagged the alias either a TRUE_SHORTHAND or a SHORTHAND_AND_LONGHAND, as we do for other aliases.  (I think the dodgy SHORTHAND_AND_LONGHAND categorization for aliases is just a hack to get value-lookups to work correctly.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1 v2: fixup existing tests so that they won't break when -webkit aliases appear

(A Try run revealed that there was one more test I needed to fixup for part 1 -- a second CSSUnprefixingService test, test_unprefixing_service_prefs.html.

This updated patch fixes that test by disabling native webkit prefixes during the test, just like in the other unprefixing_service mochitest mentioned in comment 1.)

Comment 4

[Daniel Holbert [:dholbert]]

Green try run with updated patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54d02f56d9e2

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8669280 [details] [diff] [review]
part 2: add webkit aliases to property_database.js

Review of attachment 8669280 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the transform-style fix or an explanation if I overlooked some reason for it being missing.

::: layout/style/test/property_database.js
@@ +6442,5 @@
> +    domProp: "webkitTransformStyle",
> +    inherited: false,
> +    type: CSS_TYPE_SHORTHAND_AND_LONGHAND,
> +    alias_for: "transform-style",
> +  };

Should this one have subproperties: [ "transform-style" ]?

@@ +6513,5 @@
> +    type: CSS_TYPE_SHORTHAND_AND_LONGHAND,
> +    alias_for: "border-bottom-right-radius",
> +    subproperties: [ "border-bottom-right-radius" ],
> +  };
> +  gCSSProperties["-webkit-appearance"] = {

Do we actually parse values like "button" for -webkit-appearance?  Or does the CSS unprefixing service only translate -webkit-appearance:none?

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> And in cases where we don't have a moz alias to crib from, I just copied
> data from the alias target, and tagged the alias either a TRUE_SHORTHAND or
> a SHORTHAND_AND_LONGHAND, as we do for other aliases.  (I think the dodgy
> SHORTHAND_AND_LONGHAND categorization for aliases is just a hack to get
> value-lookups to work correctly.)

Yeah, it'd be great to rename and add new values so that SHORTHAND_AND_LONGHAND is both clear in what it means and can distinguish these alias/weirdo-shorthand cases.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #5)
> Comment on attachment 8669280 [details] [diff] [review]
> part 2: add webkit aliases to property_database.js
> 
> Review of attachment 8669280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the transform-style fix or an explanation if I overlooked some
> reason for it being missing.

The reason it's missing is that I copypasted it (with the surrounding transform-related properties) from the "-moz-transform-style" definition there, and it's missing there as well!
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js?rev=5d303b961af1#4299

I'll fix up -moz-transform-style as a "part 0" here, and then let -webkit-transform-style be correct (and match its -moz equivalent) when it's added. I also filed bug 1212191 on adding a test to catch this sort of issue in the future.

> Do we actually parse values like "button" for -webkit-appearance?  Or does
> the CSS unprefixing service only translate -webkit-appearance:none?

We do -- it's one of a handful of properties that we'll treat as true aliases in bug 837211 (though it'll all be behind a pref for the time being). So, -webkit-appearance will behave exactly like -moz-appearance.

(If you have concerns about this, let me know and we can address them before unpreffing the aliases added in bug 837211.)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> The reason it's missing is that I copypasted it (with the surrounding
> transform-related properties) from the "-moz-transform-style" definition
> there, and it's missing there as well!

...and it's missing there because I accidentally removed it in bug 1210905, along with other arrays that I wanted to remove:
 http://hg.mozilla.org/mozilla-central/diff/1d65a48db29b/layout/style/test/property_database.js#l1.213

Oops! I double-checked to be sure that's the only subproperties array that I removed there.

I'll land the subproperties-restoration patch as a followup on bug 1210905, instead of a "part 0" here as I'd previously said.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Oops! I double-checked to be sure that's the only subproperties array that I
> removed there.

(double-checked just now, I mean -- to be sure I didn't mess anything else up. :))

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/798a47128b77
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23e9cbf415d

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/798a47128b77
https://hg.mozilla.org/mozilla-central/rev/a23e9cbf415d