Closed Bug 474356 Opened 12 years ago Closed 11 years ago

Implement device-pixel-ratio


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: roc, Assigned: jdm)




(1 file, 6 obsolete files)

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:
although it's not there yet.
All tests pass; thanks for the help.
Whoops, qref.
Attachment #472831 - Attachment is obsolete: true
Attachment #472833 - Flags: review?(roc)
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.
Comments addressed, now with more tests.
Attachment #473064 - Attachment is obsolete: true
Sigh, qref.
Attachment #473065 - Flags: review?(roc)
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.
Attachment #473065 - Attachment is obsolete: true
Attachment #473065 - Flags: review?(roc)
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() {
>+  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+
Attachment #474121 - Attachment is obsolete: true
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+
Thanks, teoli. This woulda been missed without your nice catch, since it wasn't flagged for docs.
This actually bounced the first time due to a test problem, and I'm planning to land it later today.
Attachment #474365 - Attachment is obsolete: true
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+
Closed: 11 years ago
Resolution: --- → FIXED
We could actually have avoided adding a new feature here by adding the 'dppx' unit proposed in: and just using queries against resolution in dppx units.
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?
You need to log in before you can comment on or make changes to this bug.