Support -webkit-min-device-pixel-ratio in CSS media queries (nonstandard version of "min-resolution")

RESOLVED FIXED in Firefox 45

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: karlcow, Assigned: dholbert)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla45
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 affected, firefox45 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

2 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.
(Reporter)

Updated

2 years ago
Blocks: 1170774
(Reporter)

Updated

2 years ago
No longer blocks: 1170774
Blocks: 1170789
(Assignee)

Comment 1

2 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.
Summary: Alias -webkit-min-device-pixel-ratio into a moz equivalent property → Support -webkit-min-device-pixel-ratio media query feature (nonstandard version of "min-resolution")
(Assignee)

Updated

2 years ago
Summary: Support -webkit-min-device-pixel-ratio media query feature (nonstandard version of "min-resolution") → Support -webkit-min-device-pixel-ratio in CSS media queries (nonstandard version of "min-resolution")
(Assignee)

Comment 2

2 years ago
Taking.
Assignee: nobody → dholbert
(Assignee)

Comment 3

2 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.
(Assignee)

Updated

2 years ago
Depends on: 474356
Version: 41 Branch → Trunk
(Assignee)

Comment 4

2 years ago
Created attachment 8685062 [details]
testcase 1 (valid for devices with 1.0 pixel ratio)

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)
(Assignee)

Comment 5

2 years ago
Created attachment 8685082 [details] [diff] [review]
part 1: use nsDependentString & Rebind() to strip min/max prefixes off of media query

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
Attachment #8685082 - Flags: review?(cam)
(Assignee)

Comment 6

2 years ago
Created attachment 8685099 [details] [diff] [review]
part 2: Add "requirement flags" field to media query features, and logic for ignoring the feature if requirements aren't met

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.)
Attachment #8685099 - Flags: review?(cam)
(Assignee)

Comment 7

2 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.)
(Assignee)

Comment 8

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

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).
Attachment #8685108 - Flags: review?(cam)
(Assignee)

Comment 9

2 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.
Attachment #8685108 - Flags: review?(cam)
(Assignee)

Comment 10

2 years ago
Created attachment 8685141 [details] [diff] [review]
part 3 v2: Add support for -webkit-device-pixel-ratio, along with its min/max variants (behind a pref)

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).
Attachment #8685108 - Attachment is obsolete: true
Attachment #8685141 - Flags: review?(cam)
(Assignee)

Comment 11

2 years ago
Created attachment 8685148 [details] [diff] [review]
part 4: tests

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.
Attachment #8685148 - Flags: review?(cam)
(Assignee)

Comment 12

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4a7fc18e73
Attachment #8685082 - Flags: review?(cam) → review+
Attachment #8685099 - Flags: review?(cam) → review+
Attachment #8685141 - Flags: review?(cam) → review+
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.
Attachment #8685148 - Flags: review?(cam) → review+
(Assignee)

Comment 14

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

Comment 15

2 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

Comment 16

2 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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
Flags: in-testsuite+
Spec text: https://compat.spec.whatwg.org/#css-media-queries-webkit-device-pixel-ratio
(Assignee)

Updated

2 years ago
Depends on: 1237101
(Assignee)

Updated

2 years ago
Depends on: 1237720
(Assignee)

Comment 18

2 years ago
Note that this feature was later preffed off by default in bug 1237720.
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
Flags: needinfo?(dholbert)
Keywords: dev-doc-needed
(Assignee)

Comment 20

11 months 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?
Flags: needinfo?(dholbert) → needinfo?(sebastianzartner)
Redirecting the question to teoli. He may know better what to do here.

Sebastian
Flags: needinfo?(sebastianzartner) → needinfo?(jypenator)
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.
Flags: needinfo?(jypenator) → needinfo?(sebastianzartner)
(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
Flags: needinfo?(sebastianzartner) → needinfo?(jypenator)
(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.
Flags: needinfo?(jypenator)
Thanks for the feedback! Removed the notes and adjusted the compatibility information.

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Comment 26

9 months ago
See also https://webcompat.com/issues/3584
Another webcompat issue related to this.
(Reporter)

Comment 27

4 months ago
See https://webcompat.com/issues/5617 for Quora bug
(Reporter)

Comment 28

4 months ago
see https://webcompat.com/issues/5444 for Etsy
You need to log in before you can comment on or make changes to this bug.