Daniel Holbert [:dholbert] Assignee
	Description • 8 years ago

I think we should put "-webkit-min-device-pixel-ratio"/"-webkit-max-device-pixel-ratio" behind its own pref, so we can turn it off by default (while leaving other webkit prefix support enabled), because it breaks Google Docs, as described in bug 1237101.

Summary of bug 1237101:
 * Google Docs has a chunk of CSS inside of a "-webkit-min-device-pixel-ratio" media query, and this chunk relies on support for an unrelated not-yet-standardized (and not-available-in-gecko) feature -- "content" property on arbitrary elements. (bug 215083)
 * That feature ("content") is non-trivial to implement, and not something that we should block our webkit prefix support on.
 * So, given the current CSS used by Google Docs -- and its popularity -- it would be irresponsible of us to ship support for "-webkit-min-device-pixel-ratio" to users.
 * We can & should strive to get Google to fix their CSS so that it doesn't depend on this non-standard feature. But in the meantime, we should plan for the possibility that this will take some time.
 * So: in the meantime, we should put "-webkit-min-device-pixel-ratio" behind its own pref, so that we can turn it off by default, but still allow it to be enabled to e.g. test this Google Docs bug.

Filing this bug on adding such a pref.

	Mike Taylor [:miketaylr]
	Comment 1 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #0)
> I think we should put
> "-webkit-min-device-pixel-ratio"/"-webkit-max-device-pixel-ratio" behind its
> own pref, so we can turn it off by default 

+1

	Daniel Holbert [:dholbert] Assignee
	Comment 2 • 8 years ago

I was initially going to name the new pref "layout.css.prefixes.webkit-device-pixel-ratio", but that's problematic due to 1208744.  (Live changes to "layout.css.prefixes.webkit-device-pixel-ratio" will sneakily flip nsCSSParser.cpp's static bool for "layout.css.prefixes.webkit", without actually changing the pref itself. That is annoying and bad and undesirable.)

To avoid that, this new pref cannot have "layout.css.prefixes.webkit" as a prefix (or else changes to it will muck with internal state that tracks layout.css.prefixes.webkit).

So, my tentative new pref name is now "layout.css.prefixes.device-pixel-ratio-webkit"

	Daniel Holbert [:dholbert] Assignee
	Comment 3 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #2)
> but that's problematic due to 1208744.

"due to bug 1208744"

	Daniel Holbert [:dholbert] Assignee
	Comment 4 • 8 years ago

I verified the following locally, in a profile with layout.css.devPixelsPerPx set to 2 (to simulate HiDPI):
 (1) Google docs toolbar icons are broken in a stock debug build.

 (2) The icons are fixed, in a debug build with this patch applied.

 (3) I can re-break Google Docs by manually togging the new pref to false. (i.e. the pref does succesfully control support for -webkit-min-device-pixel-ratio)

 (4) After (3), I can then re-fix Google Docs by setting layout.css.prefixes.webkit to false & reloading (i.e.: the "master" webkit-prefix pref still "wins" at disabling webkit-prefix support across-the-board, regardless of this new pref's value)

	Daniel Holbert [:dholbert] Assignee
	Comment 5 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #4)
>  (3) I can re-break Google Docs by manually togging the new pref to false.
> (i.e. the pref does succesfully control support for
> -webkit-min-device-pixel-ratio)


Sorry, typo -- should've said "toggling the new pref to *true*" (enabling support for -webkit-min-device-pixel-ratio)

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 8 years ago

(minor tweak -- just added a leading "-" to "-webkit" in a comment in the tests prefs file.)

	Cameron McCormack (:heycam)
	Comment 7 • 8 years ago

Comment on attachment 8705334 [details] [diff] [review]
fix v2

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

::: layout/style/nsCSSParser.cpp
@@ +3480,5 @@
>    uint8_t satisfiedReqFlags = 0;
>  
>    // Strip off "-webkit-" prefix from featureString:
>    if (sWebkitPrefixedAliasesEnabled &&
> +      sWebkitDevicePixelRatioEnabled &&

If we ever start supporting another -webkit- prefixed media feature, then the layout.css.prefixes.device-pixel-ratio-webkit prefix will affect that too.  Is that likely?  Can we add a check (or an assertion) that this only affects -webkit-(...)-device-pixel-ratio?

	Daniel Holbert [:dholbert] Assignee
	Comment 8 • 8 years ago

Good point. I added an assertion that will fail as soon as we add support for -webkit prefixed media-query feature, which should prevent that (or let us know to refactor this logic as-needed).

Try run, which looks good (modulo intermittents):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=697b8822a058&selectedJob=15224806

	Pulsebot
	Comment 9 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/be92e1923730

	Phil Ringnalda (:philor)
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/be92e1923730