Karl Dubost💡 :karlcow Reporter
	Description • 9 years ago

-webkit-min-device-pixel-ratio creates Web Compatibility issues. 
See for https://webcompat.com/issues/1056
        https://webcompat.com/issues/1034

Related to the Bug 1170774 effort.

	Daniel Holbert [:dholbert] Assignee
	Comment 1 • 9 years ago

Two notes here:
 1) This isn't a CSS property -- it's a type of media query (so, a CSS feature, but not a property)
 2) This can't be a simple alias - the units & scale are different, as noted at http://www.w3.org/blog/CSS/2012/06/14/unprefix-webkit-device-pixel-ratio/

--> clarifying bug summary.

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

Taking.

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

(In reply to Daniel Holbert [:dholbert] from comment #1)
>  2) This can't be a simple alias - the units & scale are different, as noted
> at http://www.w3.org/blog/CSS/2012/06/14/unprefix-webkit-device-pixel-ratio/

Actually, we do already support "-moz-device-pixel-ratio" which seems to have the same units. Though the syntax is a bit different -- quoting our tests, we have e.g.:
  min--moz-device-pixel-ratio: <min value>
...and...
  -moz-device-pixel-ratio: <exact value>

...whereas the webkit syntax is:
  -webkit-min-device-pixel-ratio: <min value>

So they put the "min" on the right side of the prefix, whereas we've got it on the left in the "moz" form.

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

Here's a simple testcase for this family of media queries. (-webkit-min-device-pixel-ratio, -webkit-max-device-pixel-ratio, -webkit-device-pixel-ratio)

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

This first patch refactors the media-query parsing logic to use a nsDependentString to hold the name of our media-query feature, and Rebind() to move the start of that string to strip off "min-" & "max-" prefixes.

(This will be useful later, in part 3, when we'll have to also consider the "-webkit-" prefix which comes before "min" and "max".)

This new code is similar to the way we parse prefixes in gradient expressions, too (both vendor prefixes and the "repeating-" prefix), FWIW -- e.g. -moz-repeating-linear-gradient, vs. repeating-linear-gradient vs. linear-gradient:
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=84c5d7320a5c&mark=7586-7600#7586

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

This patch adds a new field to our nsMediaFeature struct, which can contain flags that represent requirements that calling code must check before treating a given nsMediaFeature as being valid.

(At this point in the patch queue, there are no such flags. The next patch will add one, for the "-webkit-" prefix.)

	Daniel Holbert [:dholbert] Assignee
	Comment 7 • 9 years ago

(Note that eNoRequirements=0 isn't a flag -- it's just an alias for 0 aka "no flags set", to make the value for this field more readable in the struct definitions in nsMediaFeatures.cpp.)

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

This part adds a new nsMediaFeatures entry for "device-pixel-ratio" (the unprefixed name), with a new requirement-flag "eHasWebkitPrefix", which is only satisfied if nsCSSParser strips "-webkit-" off the beginning of the feature-name.

I also put this behind the pref "layout.css.prefixes.webkit" (via its bool cache variable sWebkitPrefixedAliasesEnabled).

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

Comment on attachment 8685108 [details] [diff] [review]
part 3: Add support for -webkit-device-pixel-ratio, along with its min/max variants (behind a pref)

Actually, part 3 isn't quite complete -- it's missing some serialization code (to correctly produce "-webkit-{min-|max-}device-pixel-ratio" in a serialized string when the developer queries styleNode.sheet.media.mediaText).

Canceling review on part 3 for the moment.

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 9 years ago

OK, I've added the serialization code in this updated version of "part 3". (Just a new clause in nsMediaQuery::AppendToString, in CSSStyleSheet.cpp.)

This makes us pass expanded mochitests (which are coming in my next patch here).

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 9 years ago

Here are the mochitests. I'm just copying our existing tests for "-moz-device-pixel-ratio".

Specifically:
* In test_media_queries.html, I refactored the existing test code into a helper-function, for easier sharing.
* I also added some checks to be sure that unprefixed "device-pixel-ratio" expressions are rejected.
* And I used "hg cp" to copy + modify our standalone test for the "-moz" version, test_moz_device_pixel_ratio.html.  (It's not actually a very robust test -- it doesn't seem to test min/max -- but I copied it for thoroughness, and I'll lean on test_media_queries.html for min/max testing.)

Feel free to skim/rubberstamp this; it's test-only and pretty mechanical.

	Daniel Holbert [:dholbert] Assignee
	Comment 12 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4a7fc18e73

	Cameron McCormack (:heycam)
	Comment 13 • 9 years ago

Comment on attachment 8685148 [details] [diff] [review]
part 4: tests

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

::: layout/style/test/mochitest.ini
@@ +199,5 @@
>  [test_media_queries_dynamic.html]
>  [test_media_queries_dynamic_xbl.html]
>  [test_media_query_list.html]
>  [test_moz_device_pixel_ratio.html]
> +[test_webkit_device_pixel_ratio.html]

Nit: I guess we should keep the list of tests sorted by name, rather than have groups like this.

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 9 years ago

Oh, good point; I'll fix that before landing. Thanks!

	Pulsebot
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b44182492569
https://hg.mozilla.org/integration/mozilla-inbound/rev/d848bc063207
https://hg.mozilla.org/integration/mozilla-inbound/rev/3850cd85d8ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f644f1c41e

	Carsten Book [:Tomcat]
	Comment 16 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b44182492569
https://hg.mozilla.org/mozilla-central/rev/d848bc063207
https://hg.mozilla.org/mozilla-central/rev/3850cd85d8ed
https://hg.mozilla.org/mozilla-central/rev/48f644f1c41e

	Mike Taylor [:miketaylr]
	Comment 17 • 9 years ago

Spec text: https://compat.spec.whatwg.org/#css-media-queries-webkit-device-pixel-ratio

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

Note that this feature was later preffed off by default in bug 1237720.

	Sebastian Zartner [:sebo]
	Comment 19 • 8 years ago

Mentioned at https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries#-moz-device-pixel-ratio and https://developer.mozilla.org/en-US/Firefox/Releases/45#CSS.

Daniel, can you do me the favor again and check if everything is ok?

Sebastian

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

I'm not sure it makes sense for us to declare support for -webkit-min-device-pixel-ratio & -webkit-max-device-pixel-ratio in the Compatibility Table there, given that the feature is disabled by default.

As I understand it, the Compatibility Table is intended to serve a similar purpose to caniuse.com -- and as such, it's misleading to have a table that looks like this & declares Firefox 45 as having support (with a footnote that says the opposite):

> Feature                                                                  Firefox
> -webkit-min-device-pixel-ratio, -webkit-max-device-pixel-ratio           45

For that reason, and also since we don't bother to have "moz-device-pixel-ratio" in the compat table (and that is something we support though discourage/deprecate), I wonder if we should just remove webkit-max-device-pixel-ratio from the table as well, and rely on your <note> further up in the document to mention our (preffed-off) support for this webkit media-query?

	Sebastian Zartner [:sebo]
	Comment 21 • 8 years ago

Redirecting the question to teoli. He may know better what to do here.

Sebastian

	Jean-Yves Perrier [:teoli]
	Comment 22 • 8 years ago

As long as it is not activated by default I think we should:
- remove the note
- keept the entry and the note in the compat table but marked it as not supported.

We can revisit this the day it is activated by default.

	Sebastian Zartner [:sebo]
	Comment 23 • 8 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #22)
> As long as it is not activated by default I think we should:
> - remove the note

Just to be clear, you mean the note at https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries#-moz-device-pixel-ratio, right?

> - keept the entry and the note in the compat table but marked it as not
> supported.

Ok, I can do that, though I'd like to get some feedback on general rules[1] for this first.

Btw. I agree with Daniel that it may be misleading to claim support in 45, but clarify in the footnote that it's behind a pref that is only enabled by default starting from 49. Though, as far as I know, that's what we always did so far (and what I did).
Unfortunately there is no clear rule for that in the contributor documentation, therefore I'm asking about this in the discussion group[1].

Sebastian

[1] https://groups.google.com/forum/#!topic/mozilla.dev.mdc/B--xY8My3_4

	Jean-Yves Perrier [:teoli]
	Comment 24 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #23)
> (In reply to Jean-Yves Perrier [:teoli] from comment #22)
> > As long as it is not activated by default I think we should:
> > - remove the note
> 
> Just to be clear, you mean the note at
> https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/
> Using_media_queries#-moz-device-pixel-ratio, right?
Yes, this one.

> Btw. I agree with Daniel that it may be misleading to claim support in 45,
> but clarify in the footnote that it's behind a pref that is only enabled by
> default starting from 49. Though, as far as I know, that's what we always
> did so far (and what I did).
We never have been consistent there; but I think there was consensus on the thread.

	Sebastian Zartner [:sebo]
	Comment 25 • 8 years ago

Thanks for the feedback! Removed the notes and adjusted the compatibility information.

Sebastian

	Karl Dubost💡 :karlcow Reporter
	Comment 26 • 8 years ago

See also https://webcompat.com/issues/3584
Another webcompat issue related to this.

	Karl Dubost💡 :karlcow Reporter
	Comment 27 • 7 years ago

See https://webcompat.com/issues/5617 for Quora bug

	Karl Dubost💡 :karlcow Reporter
	Comment 28 • 7 years ago

see https://webcompat.com/issues/5444 for Etsy

	Thomas Wisniewski
	Comment 29 • 7 years ago

This just hit the iTunes store as well: https://webcompat.com/issues/9086

	Karl Dubost💡 :karlcow Reporter
	Comment 30 • 6 years ago

On https://www.samehadaku.tv/
They use it as a technique to select WebKit/Blink browsers:

```css
@media screen and (-webkit-min-device-pixel-ratio: 0) {
  #mobile-search .search-field {
    font-size: 16px;
  }
}

```
See https://webcompat.com/issues/11985

	Karl Dubost💡 :karlcow Reporter
	Comment 31 • 6 years ago

This created a regression on the site 
http://conferenciaweb.w3c.br
https://webcompat.com/issues/18653

We will contact them. Just putting it here for information.