The default bug view has changed. See this FAQ.

Implement device-pixel-ratio

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: roc, Assigned: jdm)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

We should expose the ratio of CSS pixels to device pixels as a media query value in nsMediaFeatures. Webkit does, and it's useful.
This should be implemented with a -moz- prefix unless it's in the spec.  Or has WebKit shipped an implementation without a prefix?  If so, we may as well copy what they did, as long as we're careful to do the same thing.
I think they used a -webkit prefix.

BTW Hyatt said they'd document it here:
http://webkit.org/specs/MediaQueriesExtensions.html
although it's not there yet.
We need this!
Blocks: 547469
(Assignee)

Comment 4

7 years ago
Created attachment 472831 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.


All tests pass; thanks for the help.
(Assignee)

Comment 5

7 years ago
Created attachment 472833 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.


Whoops, qref.
(Assignee)

Updated

7 years ago
Attachment #472831 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #472833 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Assignee: nobody → josh
Webkit actually exposes a float here instead of a media-query ratio. I think we should probably follow suit. It looks like ratios in the current spec are always actual width-to-height ratios, but this isn't.

It looks like Webkit's 'ratio' is the number of device pixels per CSS pixel.
It would probably be good to have parsing tests in test_media_queries.html, even if the serious functionality tests need to be separate.
(Assignee)

Comment 8

7 years ago
Created attachment 473064 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Comments addressed, now with more tests.
(Assignee)

Updated

7 years ago
Attachment #473064 - Attachment is obsolete: true
(Assignee)

Comment 9

7 years ago
Created attachment 473065 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Sigh, qref.
(Assignee)

Updated

7 years ago
Attachment #473065 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #472833 - Attachment is obsolete: true
Attachment #472833 - Flags: review?(roc)
In the tests, you probably want to check nsIDOMWindowUtils::screenPixelsPerCSSPixel at the start. If it's not 1.0 (e.g. the tests are running zoomed by default, or there's an extra scale factor being applied above fullZoom) we still want the tests to pass. Probably the best thing to do is to document.write out the <style> block with adjusted expected device-pixel-ratio values.

I would have thought that if we set fullZoom to 2, device-pixel-ratio is 2. Indeed,
+  float ratio = float(aPresContext->AppUnitsPerDevPixel()) /
+      aPresContext->AppUnitsPerCSSPixel();
is upside down; it's returning the number of CSS pixels per device pixel. aPresContext->CSSPixelsToDevPixels(1.0) would be the easiest way to write this.
BTW, dbaron is the one who needs to do the real review here.
(Assignee)

Comment 12

7 years ago
Created attachment 474121 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.
(Assignee)

Updated

7 years ago
Attachment #473065 - Attachment is obsolete: true
Attachment #473065 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #474121 - Flags: review?(dbaron)
Comment on attachment 474121 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

>diff -r e024bccd4df5 -r 1caccfebdc75 layout/style/nsCSSStyleSheet.cpp
>+        NS_ASSERTION(mFeature->mValueType || nsMediaFeature::eFloat,
>+                     "bad actual float value");

Drop these two lines.

>+            nsAutoString buffer;
>+            buffer.AppendFloat(expr.mValue.GetFloatValue());
>+            aString.Append(buffer);

Replace these three lines with:
  // Use 'line-height' as a property that takes float values
  // written in the normal way.
  expr.mValue.AppendToString(eCSSProperty_line_height, aString);

>diff -r e024bccd4df5 -r 1caccfebdc75 layout/style/nsMediaFeatures.cpp
>@@ -378,6 +386,13 @@
> 
>     // Mozilla extensions
>     {
>+      &nsGkAtoms::_moz_device_pixel_ratio,
>+      nsMediaFeature::eMinMaxAllowed,
>+      nsMediaFeature::eFloat,
>+      { nsnull },
>+      GetDevicePixelRatio
>+    },

Please indent this the same way as the surrounding lines.

And please add your new GetDevicePixelRatio function between GetGrid and GetSystemMetric to match the ordering here.


>diff -r e024bccd4df5 -r 1caccfebdc75 layout/style/test/test_media_queries.html
>--- a/layout/style/test/test_media_queries.html	Fri Sep 10 10:09:29 2010 -0400
>+++ b/layout/style/test/test_media_queries.html	Fri Sep 10 13:45:24 2010 -0400
>@@ -26,6 +26,13 @@
> 
> var iframe;
> 
>+function getZoomRatio() {
>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+  var utils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                    .getInterface(Components.interfaces.nsIDOMWindowUtils);
>+  return utils ? utils.screenPixelsPerCSSPixel : 1.0;

Don't have a failure case here.  If |utils| is null, the test should throw an exception rather than report incorrect results.

>@@ -329,6 +336,22 @@
>   should_apply("not all and (max-device-aspect-ratio: " + low_dar_2 + ")");
>   expression_should_not_be_parseable("max-device-aspect-ratio");
> 
>+  var real_dpr = 1.0 * getZoomRatio();
>+  var high_dpr = 1.1 * getZoomRatio();
>+  var low_dpr = 0.9 * getZoomRatio();
>+  should_apply("all and (max--moz-device-pixel-ratio: " + real_dpr + ")");
>+  should_apply("all and (min--moz-device-pixel-ratio: " + real_dpr + ")");
>+  should_not_apply("not all and (max--moz-device-pixel-ratio: " + real_dpr + ")");
>+  should_not_apply("not all and (min--moz-device-pixel-ratio: " + real_dpr + ")");

>+  should_not_apply("all and (max--moz-device-pixel-ratio: " + low_dpr + ")");
>+  should_not_apply("all and (min--moz-device-pixel-ratio: " + high_dpr + ")");
>+  should_apply("not all and (max--moz-device-pixel-ratio: " + low_dpr + ")");
>+  should_apply("not all and (min--moz-device-pixel-ratio: " + high_dpr + ")");

You should probably have some should_apply tests for max/high and min/low

>+  should_apply("(-moz-device-pixel-ratio: " + real_dpr + ")");

Should probably test should_not_apply with this against high/low.

>+  should_apply("(-moz-device-pixel-ratio)");


r=dbaron with that
Attachment #474121 - Flags: review?(dbaron) → review+
(Assignee)

Updated

7 years ago
Attachment #474121 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Created attachment 474365 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.
(Assignee)

Comment 15

7 years ago
Comment on attachment 474365 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Carrying over r+.
Attachment #474365 - Flags: review+
Comment on attachment 474365 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.

Let's get this in!
Attachment #474365 - Flags: approval2.0+
As this seems to have been checked-in ( http://hg.mozilla.org/mozilla-central/rev/ec2ffd935bd9 ), I've documented it at:

https://developer.mozilla.org/En/CSS/Media_queries#feature-moz-device-pixel-ratio

and in 

https://developer.mozilla.org/en/Firefox_4_for_developers#Miscellaneous_CSS.c2.a0changes
Thanks, teoli. This woulda been missed without your nice catch, since it wasn't flagged for docs.
(Assignee)

Comment 19

7 years ago
This actually bounced the first time due to a test problem, and I'm planning to land it later today.
(Assignee)

Updated

7 years ago
Attachment #474365 - Attachment is obsolete: true
(Assignee)

Comment 20

7 years ago
Created attachment 474754 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.  a=approval2.0
(Assignee)

Comment 21

7 years ago
Comment on attachment 474754 [details] [diff] [review]
Add -moz-device-pixel-ratio media query.  a=approval2.0

Carrying over r+ and a+.  This is the fixed version that shouldn't bounce this time.
Attachment #474754 - Flags: review+
(Assignee)

Comment 22

7 years ago
http://hg.mozilla.org/mozilla-central/rev/7060ac6871d1
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Great stuff, thanks!
We could actually have avoided adding a new feature here by adding the 'dppx' unit proposed in:
http://dev.w3.org/csswg/css3-images/#resolution-units and just using queries against resolution in dppx units.
Yes, you're right.

Comment 26

5 years ago
Filed bug 741644 on adding dppx. Do we also want a bug on removing this once that's landed or are we planning to keep device-pixel-ratio indefinitely?
Blocks: 1176968
You need to log in before you can comment on or make changes to this bug.