Make -webkit-appearance as an alias of -moz-appearance with pref off in stylo

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: lochang, Assigned: jwatt)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [webkitcompat] [webcompat])

Attachments

(2 attachments, 1 obsolete attachment)

Make -webkit-appearance as an alias of -moz-appearance under preference "layout.css.webkit-appearance.enabled" default to false in stylo.
Hi cameron, could you review the patch for me?

The patch is to make -webkit-appearance as an alias of -moz-appearance with pref off in stylo.
The pref and the gecko part would be landed in bug 1429700.
Comment on attachment 8942081 [details]
Bug 1429713 - Make -webkit-appearance as an alias of -moz-appearance in stylo.

https://reviewboard.mozilla.org/r/212318/#review219310

::: servo/components/style/properties/longhand/box.mako.rs:585
(Diff revision 1)
>                              -moz-window-button-maximize -moz-window-button-minimize -moz-window-button-restore
>                              -moz-window-frame-bottom -moz-window-frame-left -moz-window-frame-right -moz-window-titlebar
>                              -moz-window-titlebar-maximized
>                           """,
> +                         alias="-webkit-appearance",
> +                         gecko_pref="layout.css.webkit-appearance.enabled",

Ah, this would make the -moz-appearance controlled by the pref.  We'll need a new thing like gecko_alias_pref to make -webkit-appearance be pref controlled.  The support for this new argument can be added in servo/components/style/properties/data.py, on Longhand, and to actually make the CSS parser ignore the alias if the pref isn't set, you'll need to modify the allowed_in function in properties/properties.mako.rs.
Attachment #8942081 - Flags: review?(cam) → review-
Actually, what we want to ship eventually is -webkit-appearance
as the canonical property with -moz-appearance as an alias.
So ideally that's the configuration we want when the pref is true,
and -moz-appearance as the canonical property without an alias when
the pref is false.  But I guess -webkit-appearance as an alias is
good enough approximation for testing purposes for now.
(In reply to Cameron McCormack (:heycam) from comment #3)
> Ah, this would make the -moz-appearance controlled by the pref.  We'll need
> a new thing like gecko_alias_pref to make -webkit-appearance be pref
> controlled.

That doesn't appear to be necessary [any more?]. I didn't figure out the exact mechanics, but the use of parse_property_aliases in Longhand seems to allow what we want:

https://dxr.mozilla.org/mozilla-central/rev/51f2535c797495f1f4e864072c2449b7c28669de/servo/components/style/properties/data.py#68
Assignee: lochang → jwatt
Priority: P3 → P2
Attachment #8942081 - Attachment is obsolete: true
Comment on attachment 8979580 [details] [diff] [review]
add a -webkit-appearance alias for -moz-appearance

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

Add a test for the pref please. r=me with that.
Attachment #8979580 - Flags: review?(emilio) → review+
> Add a test for the pref please. r=me with that.

I'm guessing this does the trick, but r? just to check you didn't have something more in mind.
Attachment #8980516 - Flags: review?(emilio)
Comment on attachment 8980516 [details] [diff] [review]
part 2 - Enable -webkit-appearance pref for WPT webkit-appearance.tentative.html

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

Well, I meant ensuring that we have a test that checks the pref does work, in the sense of, that it's not parsed when the pref is off accidentally.

This change is fine on its own if you want it, but I meant something like either what we do for the moz-document prefs:

  https://searchfox.org/mozilla-central/source/layout/reftests/bugs/reftest.list#2076

Or since in this case reftests are kind of annoying, something that did something on the lines of:

<style id="s"></style>
<script>
var t = async_test("Tests that the webkit-appearance pref works");
pushPrefEnv({ "set": ["layout.css.webkit-appearance.enabled", false] }, t.step_func(function() {
  s.innerText = "div { -webkit-appearance: none }";
  assert_equals(s.sheet.cssRules[0].length, 0);
});
</script>

Or something of that sort.

Maybe also add the relevant entry to property_database.js, in the same way we do in the:

if (IsCSSPropertyPrefEnabled("layout.css.prefixes.webkit")) {

branches?
Attachment #8980516 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> <style id="s"></style>
> <script>
> var t = async_test("Tests that the webkit-appearance pref works");
> pushPrefEnv({ "set": ["layout.css.webkit-appearance.enabled", false] },
> t.step_func(function() {
>   s.innerText = "div { -webkit-appearance: none }";
>   assert_equals(s.sheet.cssRules[0].length, 0);
> });
> </script>
> 
> Or something of that sort.

FWIW it seems changes to the pref are not picked up until the doc is reloaded, so I'll put the pref setting in the mochitest.ini.
Huh, that's really weird...
(In reply to Jonathan Watt [:jwatt] from comment #10)
> FWIW it seems changes to the pref are not picked up until the doc is
> reloaded, so I'll put the pref setting in the mochitest.ini.

Actually, that doesn't work either, since the mochitest harness only allows prefs to be set at the start of mochitest.ini for all the tests it lists, and not on a per-test basis. :/

Instead I've rewritten the test to use an iframe so that I can reload that sub-document after changing the pref.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> something that did something on the lines of:
> 
> <style id="s"></style>
> <script>
> var t = async_test("Tests that the webkit-appearance pref works");
> pushPrefEnv({ "set": ["layout.css.webkit-appearance.enabled", false] },
> t.step_func(function() {
>   s.innerText = "div { -webkit-appearance: none }";
>   assert_equals(s.sheet.cssRules[0].length, 0);
> });
> </script>

Yeah, that's basically what my test does. It checks the computed style with the pref enabled/disabled, checking whether a rule that sets -webkit-appearance results in a suitable computed value.

> Maybe also add the relevant entry to property_database.js, in the same way
> we do in the:
> 
> if (IsCSSPropertyPrefEnabled("layout.css.prefixes.webkit")) {
> 
> branches?

Will do.
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c6f05265942
part 1 - Add a -webkit-appearance alias for -moz-appearance (behind a pref). r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/395339b3bbfc
part 2 - Add basic tests for the layout.css.webkit-appearance.enabled pref. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ad37d3b0b2
part 3 - Update property_database.js for the -webkit-appearance alias. rs=emilio
Blocks: 1368555
Blocks: 1428676
No longer blocks: 1429700
Blocks: 1480073
You need to log in before you can comment on or make changes to this bug.