Give "-webkit-min-device-pixel-ratio"/"-webkit-max-device-pixel-ratio" media query its own pref, & disable it

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
(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
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"
(In reply to Daniel Holbert [:dholbert] from comment #2)
> but that's problematic due to 1208744.

"due to bug 1208744"
Posted patch fix v1 (obsolete) — Splinter Review
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)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8705333 - Flags: review?(cam)
(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)
Posted patch fix v2Splinter Review
(minor tweak -- just added a leading "-" to "-webkit" in a comment in the tests prefs file.)
Attachment #8705333 - Attachment is obsolete: true
Attachment #8705333 - Flags: review?(cam)
Attachment #8705334 - Flags: review?(cam)
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?
Attachment #8705334 - Flags: review?(cam) → review+
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

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be92e1923730
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
No longer blocks: 1338424
You need to log in before you can comment on or make changes to this bug.