474356 - Implement device-pixel-ratio

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should expose the ratio of CSS pixels to device pixels as a media query value in nsMediaFeatures. Webkit does, and it's useful.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need this!

Comment 4

[Josh Matthews [:jdm]]

Attachment: Add -moz-device-pixel-ratio media query.

All tests pass; thanks for the help.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Add -moz-device-pixel-ratio media query.

Whoops, qref.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It would probably be good to have parsing tests in test_media_queries.html, even if the serious functionality tests need to be separate.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Add -moz-device-pixel-ratio media query.

Comments addressed, now with more tests.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Add -moz-device-pixel-ratio media query.

Sigh, qref.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW, dbaron is the one who needs to do the real review here.

Comment 12

[Josh Matthews [:jdm]]

Attachment: Add -moz-device-pixel-ratio media query.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

[Josh Matthews [:jdm]]

Attachment: Add -moz-device-pixel-ratio media query.

Comment 15

[Josh Matthews [:jdm]]

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

Carrying over r+.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Let's get this in!

Comment 17

[Jean-Yves Perrier [:teoli]]

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

Comment 18

[Eric Shepherd [:sheppy]]

Thanks, teoli. This woulda been missed without your nice catch, since it wasn't flagged for docs.

Comment 19

[Josh Matthews [:jdm]]

This actually bounced the first time due to a test problem, and I'm planning to land it later today.

Comment 20

[Josh Matthews [:jdm]]

Attachment: Add -moz-device-pixel-ratio media query. a=approval2.0

Comment 21

[Josh Matthews [:jdm]]

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.

Comment 22

[Josh Matthews [:jdm]]

http://hg.mozilla.org/mozilla-central/rev/7060ac6871d1

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great stuff, thanks!

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, you're right.

Comment 26

[fantasai]

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?