Emilio Cobos Álvarez (:emilio) Assignee
	Description • 7 years ago

See also bug 1396048.

We've been without them for a while in stylo and no bug reports so far.

Code-search in github also returns no results for it, and it looks like a fingerprinting vector (see bug 541386).

We should consider unshipping it from content pages after 57.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 1 • 7 years ago

I sent https://groups.google.com/forum/#!topic/mozilla.dev.platform/RVttfrQkXLU, let's use that thread if discussion is needed.

	Xidorn Quan [:xidorn] UTC+11
	Comment 7 • 7 years ago

I don't think your intent to unship email in comment 1 covers patches after the first one. Could you send another intent to unship mentioning all of them?

	Xidorn Quan [:xidorn] UTC+11
	Comment 8 • 7 years ago

Oh, actually, I guess that one probably intended to cover all the media queries although didn't name all of them explicitly.

-moz-is-glyph isn't a system metric anyway...

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 9 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> Oh, actually, I guess that one probably intended to cover all the media
> queries although didn't name all of them explicitly.

Right, I posted separate patches for the ones that don't explicitly use `GetSystemMetric`, but I think they all fall in the same category.

> -moz-is-glyph isn't a system metric anyway...

That's true, but there was a comment (falsely) claiming it was internal, so I just assumed this was overlooked and fixed it. Let me know if you really want another email.

	Xidorn Quan [:xidorn] UTC+11
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8914736 [details]
Bug 1396066: Restrict :-moz-system-metric to chrome and ua sheets.

https://reviewboard.mozilla.org/r/186018/#review191252

	Xidorn Quan [:xidorn] UTC+11
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8914737 [details]
Bug 1396066: Restrict system-metric media features to UA and chrome sheets only.

https://reviewboard.mozilla.org/r/186020/#review191254

r=me with the issues addressed.

::: layout/style/nsCSSParser.cpp:3505
(Diff revision 1)
>    // case insensitive from CSS - must be lower cased
>    nsContentUtils::ASCIIToLower(mToken.mIdent);
>    nsDependentString featureString(mToken.mIdent, 0);
>    uint8_t satisfiedReqFlags = 0;
>  
> +  if (EnabledState() & (CSSEnabledState::eInUASheets | CSSEnabledState::eInChrome)) {

This line is too long. Please have it less than 80 chars. Probably add a line break after `|`.

::: layout/style/nsMediaFeatures.cpp:740
(Diff revision 1)
>    {
>      &nsGkAtoms::_moz_touch_enabled,
>      nsMediaFeature::eMinMaxNotAllowed,
>      nsMediaFeature::eBoolInteger,
> -    nsMediaFeature::eNoRequirements,
> +    nsMediaFeature::eUserAgentAndChromeOnly,
>      { &nsGkAtoms::touch_enabled },
>      GetSystemMetric
>    },

I'm a bit concerned about this one. From search result, it seems to be widely used as a way to detect touch screen, and is mentioned even in published book.

We should probably add a telemetry before removing this, or at least, we should not unship this until we implement the standard equivalent in bug 1035774.

::: layout/style/nsMediaFeatures.cpp:760
(Diff revision 1)
>    },
>    {
>      &nsGkAtoms::_moz_windows_theme,
>      nsMediaFeature::eMinMaxNotAllowed,
>      nsMediaFeature::eIdent,
> +    // TODO(emilio): Unship.

You may not want to remove the comment when you haven't touched their restriction? It probably doesn't matter a lot, though.

::: layout/style/nsMediaFeatures.cpp:784
(Diff revision 1)
>    {
>      &nsGkAtoms::_moz_physical_home_button,
>      nsMediaFeature::eMinMaxNotAllowed,
>      nsMediaFeature::eBoolInteger,
> -    nsMediaFeature::eNoRequirements,
> +    nsMediaFeature::eUserAgentAndChromeOnly,
>      { &nsGkAtoms::physical_home_button },
>      GetSystemMetric
>    },

This is something we can probably remove directly. It is likely a leftover from B2G. I see there is not even any internal code using it.

::: layout/style/test/test_media_queries.html
(Diff revision 1)
> -  expression_should_be_parseable("-moz-scrollbar-start-backward");
> -  expression_should_be_parseable("-moz-scrollbar-start-forward");
> -  expression_should_be_parseable("-moz-scrollbar-end-backward");
> -  expression_should_be_parseable("-moz-scrollbar-end-forward");
> -  expression_should_be_parseable("-moz-scrollbar-thumb-proportional");
> -  expression_should_be_parseable("-moz-overlay-scrollbars");
> -  expression_should_be_parseable("-moz-windows-default-theme");
> -  expression_should_be_parseable("-moz-mac-graphite-theme");
> -  expression_should_be_parseable("-moz-mac-yosemite-theme");
> -  expression_should_be_parseable("-moz-windows-accent-color-in-titlebar");
> -  expression_should_be_parseable("-moz-windows-compositor");
> -  expression_should_be_parseable("-moz-windows-classic");
> -  expression_should_be_parseable("-moz-windows-glass");
> -  expression_should_be_parseable("-moz-touch-enabled");
> -  expression_should_be_parseable("-moz-swipe-animation-enabled");
> -
> -  expression_should_be_parseable("-moz-scrollbar-start-backward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-start-forward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-end-backward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-end-forward: 0");
> -  expression_should_be_parseable("-moz-scrollbar-thumb-proportional: 0");
> -  expression_should_be_parseable("-moz-overlay-scrollbars: 0");
> -  expression_should_be_parseable("-moz-windows-default-theme: 0");
> -  expression_should_be_parseable("-moz-mac-graphite-theme: 0");
> -  expression_should_be_parseable("-moz-mac-yosemite-theme: 0");
> -  expression_should_be_parseable("-moz-windows-accent-color-in-titlebar: 0");
> -  expression_should_be_parseable("-moz-windows-compositor: 0");
> -  expression_should_be_parseable("-moz-windows-classic: 0");
> -  expression_should_be_parseable("-moz-windows-glass: 0");
> -  expression_should_be_parseable("-moz-touch-enabled: 0");
> -  expression_should_be_parseable("-moz-swipe-animation-enabled: 0");
> -
> -  expression_should_be_parseable("-moz-scrollbar-start-backward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-start-forward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-end-backward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-end-forward: 1");
> -  expression_should_be_parseable("-moz-scrollbar-thumb-proportional: 1");
> -  expression_should_be_parseable("-moz-overlay-scrollbars: 1");
> -  expression_should_be_parseable("-moz-windows-default-theme: 1");
> -  expression_should_be_parseable("-moz-mac-graphite-theme: 1");
> -  expression_should_be_parseable("-moz-mac-yosemite-theme: 1");
> -  expression_should_be_parseable("-moz-windows-accent-color-in-titlebar: 1");
> -  expression_should_be_parseable("-moz-windows-compositor: 1");
> -  expression_should_be_parseable("-moz-windows-classic: 1");
> -  expression_should_be_parseable("-moz-windows-glass: 1");
> -  expression_should_be_parseable("-moz-touch-enabled: 1");
> -  expression_should_be_parseable("-moz-swipe-animation-enabled: 1");

I would probably prefer we keep these tests bug mark them `expression_should_not_be_parseable`. Although I guess removing is also fine...

	Xidorn Quan [:xidorn] UTC+11
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8914738 [details]
Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only.

https://reviewboard.mozilla.org/r/186022/#review191260

Mostly looks good. Cancel r? for test change.

::: layout/style/test/chrome/bug418986-2.js
(Diff revision 1)
> -// These media queries return value 0 or 1 when the pref is off.
> -// When the pref is on, they should not match.
> -var suppressed_toggles = [
> -  "-moz-mac-graphite-theme",
> -  // Not available on most OSs.
> -//  "-moz-maemo-classic",
> -  "-moz-scrollbar-end-backward",
> -  "-moz-scrollbar-end-forward",
> -  "-moz-scrollbar-start-backward",
> -  "-moz-scrollbar-start-forward",
> -  "-moz-scrollbar-thumb-proportional",
> -  "-moz-touch-enabled",
> -  "-moz-windows-compositor",
> -  "-moz-windows-default-theme",
> -  "-moz-windows-glass",
> -];
> -
> -// Possible values for '-moz-os-version'
> -var windows_versions = [
> -  "windows-win7",
> -  "windows-win8",
> -  "windows-win10",
> -];
> -
> -// Possible values for '-moz-windows-theme'
> -var windows_themes = [
> -  "aero",
> -  "aero-lite",
> -  "luna-blue",
> -  "luna-olive",
> -  "luna-silver",
> -  "royale",
> -  "generic",
> -  "zune"
> -];

Majority of the test change should be in the previous part?

I would prefer we keep each part self-contained so that we can backout any specific part if necessary.

Also if we are not removing `-moz-touch-enabled`, we probably need to keep most of the test code below.

Actually I would prefer we don't remove the test code, but treat them as if they are always resisted so that we don't regress. Probably marking them not parseable in the media query test is enough, though...

::: layout/style/test/test_media_queries.html
(Diff revision 1)
> -  expression_should_be_parseable("-moz-windows-theme: aero");
> -  expression_should_be_parseable("-moz-windows-theme: aero-lite");
> -  expression_should_be_parseable("-moz-windows-theme: luna-blue");
> -  expression_should_be_parseable("-moz-windows-theme: luna-olive");
> -  expression_should_be_parseable("-moz-windows-theme: luna-silver");
> -  expression_should_be_parseable("-moz-windows-theme: royale");
> -  expression_should_be_parseable("-moz-windows-theme: generic");
> -  expression_should_be_parseable("-moz-windows-theme: zune");
> -  expression_should_be_parseable("-moz-windows-theme: garbage");

As before, I would prefer we do `not_be_parseable`.

	Xidorn Quan [:xidorn] UTC+11
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8914739 [details]
Bug 1396066: Remove -moz-physical-home-button.

https://reviewboard.mozilla.org/r/186024/#review191262

::: layout/style/test/test_media_queries.html
(Diff revision 1)
> -  expression_should_be_parseable("-moz-os-version: windows-win7");
> -  expression_should_be_parseable("-moz-os-version: windows-win8");
> -  expression_should_be_parseable("-moz-os-version: windows-win10");

`not_be_parseable`

	Kohei Yoshino [:kohei]
	Comment 14 • 7 years ago

Is this disabling the equivalent non-standard media features [1] as well, or just :-moz-system-metric pseudo-classes?

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/Mozilla_Extensions#Media_features

	Xidorn Quan [:xidorn] UTC+11
	Comment 15 • 7 years ago

(In reply to Kohei Yoshino [:kohei] from comment #14)
> Is this disabling the equivalent non-standard media features [1] as well, or
> just :-moz-system-metric pseudo-classes?
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/CSS/
> Mozilla_Extensions#Media_features

Both of them, although I'm arguing that we should probably not unship -moz-touch-enabled before bug 1035774.

The last patch also unships :-moz-is-glyph in addition to all the system metrics stuff, which I guess is probably fine given there is unlikely any reason people want to use it anywhere.

	Xidorn Quan [:xidorn] UTC+11
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8914740 [details]
Bug 1396066: Restrict -moz-is-glyph to UA and chrome only.

https://reviewboard.mozilla.org/r/186026/#review191266

It is probably fine that we don't test anything around `:-moz-is-glyph`... if it doesn't work properly, it should be easily caught by refests.

	Kohei Yoshino [:kohei]
	Comment 17 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> Both of them, although I'm arguing that we should probably not unship
> -moz-touch-enabled before bug 1035774.

Thanks for your clarification. I was actually thinking about it.

	Xidorn Quan [:xidorn] UTC+11
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8914738 [details]
Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only.

https://reviewboard.mozilla.org/r/186022/#review191260

> Majority of the test change should be in the previous part?
> 
> I would prefer we keep each part self-contained so that we can backout any specific part if necessary.
> 
> Also if we are not removing `-moz-touch-enabled`, we probably need to keep most of the test code below.
> 
> Actually I would prefer we don't remove the test code, but treat them as if they are always resisted so that we don't regress. Probably marking them not parseable in the media query test is enough, though...

I think keeping them in both this test and media queries test is probably more preferable, since if we need to re-enable anything for webcompat, we wouldn't forget that it should still be surpressed when resisting fingerprinting.

	Code Review Bot [:reviewbot]
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8914737 [details]
Bug 1396066: Restrict system-metric media features to UA and chrome sheets only.

https://reviewboard.mozilla.org/r/186020/#review191418


C/C++ static analysis didn't find any defects in this patch. Hooray!

	Xidorn Quan [:xidorn] UTC+11
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8914738 [details]
Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only.

https://reviewboard.mozilla.org/r/186022/#review191640

	Pulsebot
	Comment 31 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5b9aed44133
Restrict :-moz-system-metric to chrome and ua sheets. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/87b44ea10407
Restrict system-metric media features to UA and chrome sheets only. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b9d8e4588584
Restrict -moz-windows-theme and -moz-os-version to UA and chrome only. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/19c9ea492f5e
Restrict -moz-is-glyph to UA and chrome only. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/5b1997aeaead
Remove -moz-physical-home-button. r=xidorn

	Mozilla Autoland
	Comment 32 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 74a0c5bdb21c -d a701f7c8334f: rebasing 424496:74a0c5bdb21c "Bug 1396066: Restrict :-moz-system-metric to chrome and ua sheets. r=xidorn"
note: rebase of 424496:74a0c5bdb21c created no changes to commit
rebasing 424497:7d8e0e0215f2 "Bug 1396066: Restrict system-metric media features to UA and chrome sheets only. r=xidorn"
merging layout/style/nsMediaFeatures.cpp
merging layout/style/test/chrome/bug418986-2.js
merging layout/style/test/test_media_queries.html
warning: conflicts while merging layout/style/nsMediaFeatures.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 33 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/635746c88630
Fix bug 418986 tests. r=me
https://hg.mozilla.org/integration/autoland/rev/ed95eea7105c
Tentatively fix browser_parsable_css.js. r=me

	Kohei Yoshino [:kohei]
	Comment 34 • 7 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/non-standard-system-metric-pseudo-classes-and-media-features-have-been-removed/

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 7 years ago

So autocomplete-item.css worries me.  Why is the test failing for that?  It should be loaded from a chrome:// URI, I would think...

	Pulsebot
	Comment 36 • 7 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a22efa8df6b
followup.  Fix the expected message to be the right non-ASCII thing, admit that devtools don't use UA-only media features.

	Pulsebot
	Comment 37 • 7 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00
another followup.  Load input.css from a chrome uri, so it can use :-moz-system-metric.  This requires exposing the reftest chrome package to content, but that should be OK.  r=bzbarsky

	Xidorn Quan [:xidorn] UTC+11
	Comment 38 • 7 years ago

windows/autocomplete-item.css contains -moz-windows-default-theme, so that should probably be available on chrome (it is even possible we need to keep exposing it to content, since it seems to be in an XBL.)

That should definitely not be whitelisted.

The whitelist for svg.css also looks too loose. It should probably be specific to only the violating ones that are ua-only.

	Xidorn Quan [:xidorn] UTC+11
	Comment 39 • 7 years ago

(In reply to Pulsebot from comment #37)
> Pushed by bzbarsky@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00
> another followup.  Load input.css from a chrome uri, so it can use
> :-moz-system-metric.  This requires exposing the reftest chrome package to
> content, but that should be OK.  r=bzbarsky

I suspect that it wouldn't work. IIRC availability of chrome-only features depends on document url rather than stylesheet url?

	Xidorn Quan [:xidorn] UTC+11
	Comment 40 • 7 years ago

Just figured out that there are several media features that we still need to expose because chrome code uses them in matchMedia. Also there are some features that can be removed completely, because they are no longer used.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 41 • 7 years ago

Yeah, that still doesn't explain why https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00 doesn't work, but anyway, Xidorn and I discussed and we're going to back this out for now, and reland more incrementally.

	Pulsebot
	Comment 42 • 7 years ago

Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4279ffa1e0da
Backed out 9 changesets for Windows reftest failures

	Pulsebot
	Comment 43 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c404f982003
Add a mechanism to make media features chrome / UA-only. r=xidorn

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 44 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2c404f982003

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 45 • 7 years ago

Bug 1410074 takes care of the last ones, yay!

	Chris Mills [:cmills]
	Comment 46 • 7 years ago

I've documented this; first of all take a look at the Fx 58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#CSS_2

All of the pages I've touched are listed there, and they match the pages listed in Kohei's compat article. On each of the pages I've added a note stating that they are no longer available to web content in Firefox 58.

I've also updated the Mozilla CSS extensions page, moving the affected pseudo-classes to this section:

https://developer.mozilla.org/en-US/docs/Web/CSS/Mozilla_Extensions#Mozilla-only_properties_and_pseudo-classes_(avoid_using_on_websites)

Let me know if that look OK.

	Alex Vallat
	Comment 47 • 7 years ago

Is it intentional that these should be unavailable to userChrome.css too? Not being able to define styles per-os and per-theme can be quite limiting.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 48 • 7 years ago

(In reply to Alex Vallat from comment #47)
> Is it intentional that these should be unavailable to userChrome.css too?
> Not being able to define styles per-os and per-theme can be quite limiting.

I think you should be able to still style per theme with :-moz-lwtheme-* pseudoclasses, but... Per [1], it looks these are loaded with file:// URIs, so they're not loaded as "chrome" stylesheets. Also, there's nothing in the CSS system that makes these available to user sheets...

I can see no reason why they shouldn't though, mind filing a bug and needinfo me?

[1]: https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/layout/style/nsLayoutStylesheetCache.cpp#443

	Alex Vallat
	Comment 49 • 7 years ago

Sorry, I meant windows theme, like -moz-windows-classic, not -moz-lwtheme themes.

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1418963 as requested.