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

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Filing this bug on getting bug 837211's -webkit prefixed property aliases into our mochitests.
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.
Attachment #8669273 - Flags: review?(cam)
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.)
Attachment #8669280 - Flags: review?(cam)
(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.)
Attachment #8669273 - Attachment is obsolete: true
Attachment #8669273 - Flags: review?(cam)
Attachment #8669368 - Flags: review?(cam)
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?
Attachment #8669280 - Flags: review?(cam) → review+
(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.
Attachment #8669368 - Flags: review?(cam) → review+
(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.)
(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.
(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. :))
https://hg.mozilla.org/mozilla-central/rev/798a47128b77
https://hg.mozilla.org/mozilla-central/rev/a23e9cbf415d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.