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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement, P2)

Description

[Louis Chang [:louis]]

Make -webkit-appearance as an alias of -moz-appearance under preference "layout.css.webkit-appearance.enabled" default to false in stylo.

Comment 1

[Louis Chang [:louis]]

Attachment: Bug 1429713 - Make -webkit-appearance as an alias of -moz-appearance in stylo.

Review commit: https://reviewboard.mozilla.org/r/212318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212318/

Comment 2

[Louis Chang [:louis]]

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 3

[Cameron McCormack (:heycam)]

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.

Comment 4

[Mats Palmgren (inactive)]

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.

Comment 5

[Jonathan Watt [:jwatt]]

(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

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: add a -webkit-appearance alias for -moz-appearance

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Enable -webkit-appearance pref for WPT webkit-appearance.tentative.html

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

Comment 9

[Emilio Cobos Álvarez (: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?

Comment 10

[Jonathan Watt [:jwatt]]

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Huh, that's really weird...

Comment 12

[Jonathan Watt [:jwatt]]

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

Comment 13

[Jonathan Watt [:jwatt]]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7c6f05265942
https://hg.mozilla.org/mozilla-central/rev/395339b3bbfc
https://hg.mozilla.org/mozilla-central/rev/d9ad37d3b0b2