Last Comment Bug 779527 - Fennec reports wrong screen resolution/density and pixel ratio
: Fennec reports wrong screen resolution/density and pixel ratio
Status: RESOLVED FIXED
[leave open]
: regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 16 Branch
: All Android
: -- normal (vote)
: ---
Assigned To: Matt Brubeck (:mbrubeck)
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 776903 780999 784802 785285 (view as bug list)
Depends on: 803207
Blocks: 564815 reader 771390 776903 794056 840916
  Show dependency treegraph
 
Reported: 2012-08-01 09:05 PDT by Lucas Rocha (:lucasr)
Modified: 2016-07-29 14:29 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed
verified
16+


Attachments
band-aid (1.29 KB, patch)
2012-08-23 13:32 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
band-aid (v2) (2.75 KB, patch)
2012-08-28 15:06 PDT, Matt Brubeck (:mbrubeck)
dbaron: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
mbrubeck: checkin+
Details | Diff | Splinter Review
WIP (2.75 KB, patch)
2012-08-28 15:49 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
WIP 2 (5.53 KB, patch)
2012-08-31 13:34 PDT, Matt Brubeck (:mbrubeck)
dbaron: feedback-
Details | Diff | Splinter Review

Description Lucas Rocha (:lucasr) 2012-08-01 09:05:36 PDT
Because of that, media queries with min-resolution and -moz-device-pixel-ratio just don't work. This is happening on current Aurora and Nightly. I've created two test pages to help us finding out when the regression happened. The page should show the correct Android density on screen (MDPI, HDPI, and XHDPI).

Test min-resolution media query
http://lucasr.org/res.html

Test -moz-device-pixel-ratio media query
http://lucasr.org/pix.html
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-01 12:48:31 PDT
Adding dbaron
Comment 3 Lucas Rocha (:lucasr) 2012-08-02 02:08:21 PDT
This seems to be a very serious regression btw. Pixel ratio is always 1.0 even on HDPI and XHDPI Android devices. Pretty much breaks any media queries using screen density conditions.
Comment 4 Lucas Rocha (:lucasr) 2012-08-03 12:40:25 PDT
*** Bug 776903 has been marked as a duplicate of this bug. ***
Comment 5 David Baron :dbaron: ⌚️UTC-10 2012-08-03 12:50:51 PDT
Bug 771390 should have changed resolution, but they shouldn't have changed anything about -moz-device-pixel-ratio.  They should have made resolution work the same way device-pixel-ratio does, except for the factor of 96.

Are you sure device-pixel-ratio changed in that range?  If so, which changeset changed it?
Comment 6 David Baron :dbaron: ⌚️UTC-10 2012-08-03 14:34:53 PDT
And I'm not sure when we'd ever have returned non-integer values for device-pixel-ratio.  I thought it should always be an integer.
Comment 7 Lucas Rocha (:lucasr) 2012-08-06 07:30:49 PDT
(In reply to David Baron [:dbaron] from comment #6)
> And I'm not sure when we'd ever have returned non-integer values for
> device-pixel-ratio.  I thought it should always be an integer.

Not if we want to be compatible with -webkit-device-pixel-ratio. For example, here's what Android does:

  http://developer.android.com/guide/webapps/targeting.html#DensityCSS

Even if it was integer-only, I'm seeing -moz-device-pixel-ratio == 1 on an XHDPI device where it should be 2.
Comment 8 Lucas Rocha (:lucasr) 2012-08-06 07:32:17 PDT
(In reply to David Baron [:dbaron] from comment #5)
> Bug 771390 should have changed resolution, but they shouldn't have changed
> anything about -moz-device-pixel-ratio.  They should have made resolution
> work the same way device-pixel-ratio does, except for the factor of 96.
> 
> Are you sure device-pixel-ratio changed in that range?  If so, which
> changeset changed it?

So, you mean we should use things like (-min-resolution: 2) meaning the device has screen density (2 * 96dpi)?
Comment 9 Arthur Stolyar [:nekr] 2012-08-08 07:25:45 PDT
On xhdpi devices length of independent by width is 360. So chrome scales viewport into ~340 pixels and has ~2.14 dpr. Opera scales to 320 and has ~2.25 dpr. If you want has exactly 2 into dpr (or resolution: 2dppx) you should scale page to 360 pixels when developer adds meta tag or style with viewport with zoom value -- 1.
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-08 07:36:56 PDT
*** Bug 780999 has been marked as a duplicate of this bug. ***
Comment 11 Lucas Rocha (:lucasr) 2012-08-08 07:56:30 PDT
(In reply to Arthur from comment #9)
> On xhdpi devices length of independent by width is 360. So chrome scales
> viewport into ~340 pixels and has ~2.14 dpr. Opera scales to 320 and has
> ~2.25 dpr. If you want has exactly 2 into dpr (or resolution: 2dppx) you
> should scale page to 360 pixels when developer adds meta tag or style with
> viewport with zoom value -- 1.

I'm using (min--moz-device-pixel-ratio: 2.0) and still doesn't work. Using (min-resolution: 2dppx) doesn't work either (on XHDPI devices).
Comment 12 Arthur Stolyar [:nekr] 2012-08-08 07:58:36 PDT
Yep, it seems latest versions has bug here. But Firefox Stable still matches 320+ dpi and 2dppx.
Comment 13 Matt Brubeck (:mbrubeck) 2012-08-09 12:05:51 PDT
On my HDPI Android device, I get the following values in GetResolution:

a: AppUnitsPerPhysicalInch = 14400
b: AppUnitsPerCSSInch = 5760
c: AppUnitsPerDevPixel = 60

GetResolution used to return a/c = 240, but the patch from bug 771390 changed it to return b/c = 96.

AppUnitsPerCSSInch is always defined as (96 * AppUnitsPerCSSPixel) = (96 * 60).

AppUnitsPerDevPixel is set to mAppUnitsPerDevNotScaledPixel / mPixelScale, which is currently set to (60 / 1.0) = 60.  Is this the value that we ultimately need to change, to more accurately reflect how we do layout and zooming on Android?  There are currently some prefs or widget methods that can change this, but they seem designed for use with FullZoom and have side effects that we don't want...
Comment 14 David Baron :dbaron: ⌚️UTC-10 2012-08-09 12:15:29 PDT
It's also possible that we just need to teach some of these media queries to better reflect the state of the device rather than the state of the fake viewport in which we're doing mobile rendering.  The question is which ones and how -- since that does break the viewport + pan + zoom model.
Comment 15 Matt Brubeck (:mbrubeck) 2012-08-09 16:00:53 PDT
(In reply to David Baron [:dbaron] from comment #14)
> It's also possible that we just need to teach some of these media queries to
> better reflect the state of the device rather than the state of the fake
> viewport in which we're doing mobile rendering.  The question is which ones
> and how -- since that does break the viewport + pan + zoom model.

I think the "resolution" media query should definitely reflect the state of the physical device; this would match the existing behavior of Fennec and other browsers, and the spec says it should be "the resolution of the output device."
Comment 16 Matt Brubeck (:mbrubeck) 2012-08-16 14:04:04 PDT
dbaron, should we back out bug 771390 (at least on Aurora) to fix this regression?  Is there a simple fix to make "resolution" always report the device resolution, as suggested in comment 14?
Comment 17 Matt Brubeck (:mbrubeck) 2012-08-21 08:33:23 PDT
I think the correct dppx value on Android (i.e. the one that matches other mobile browsers with panning/zooming independant of the CSS viewport) is the value returned by ViewportHandler.getScaleRatio in the Android front-end:
https://hg.mozilla.org/mozilla-central/file/f9a8fdb08193/mobile/android/chrome/content/browser.js#l4579

Perhaps the best solution is to finish moving this code into Gecko (bug 716575) and then make the resolution media query aware of it.  I can work on this, but I'll need some advice from dbaron or other layout/content experts to figure out the right way to do it.  We still need a lower-risk fix for Fx16 (and probably Fx17), e.g. backing out bug 771390.
Comment 18 Matt Brubeck (:mbrubeck) 2012-08-22 11:57:12 PDT
(In reply to David Baron [:dbaron] from comment #5)
> Bug 771390 should have changed resolution, but they shouldn't have changed
> anything about -moz-device-pixel-ratio.  They should have made resolution
> work the same way device-pixel-ratio does, except for the factor of 96.

It looks like this is accurate.

* Before bug 771390 was fixed, resolution had a correct value on Android, while device-pixel-ratio was broken on Android and always had a value of 1.

* After bug 771390 was fixed, device-pixel-ratio is unchanged (still broken), and resolution now always has a value of 96dpi (1dppx), which matches the broken device-pixel-ratio behavior.

This is regressing in-content UI in Firefox 16 that uses the "resolution" media query (bug 776903).
Comment 19 Aaron Train [:aaronmt] 2012-08-22 14:20:05 PDT
*** Bug 784802 has been marked as a duplicate of this bug. ***
Comment 20 Matt Brubeck (:mbrubeck) 2012-08-23 13:32:57 PDT
Created attachment 654770 [details] [diff] [review]
band-aid

Here's a band-aid patch that might be suitable for Fx16 and 17.  Basically this reverts to the old behavior on Android, while keeping the bug 771390 fix on desktop platforms.

Longer-term, I think the real fix for overridden viewports will be to change both resolution and device-pixel-ratio aware of the nsContentUtils::GetDevicePixelRatio code in the patches for bug 716575.
Comment 21 Aaron Train [:aaronmt] 2012-08-23 19:40:55 PDT
*** Bug 785285 has been marked as a duplicate of this bug. ***
Comment 22 Matt Brubeck (:mbrubeck) 2012-08-28 15:06:25 PDT
Created attachment 656225 [details] [diff] [review]
band-aid (v2)

Also revert the test changes from bug 771390 because they will fail on HDPI Android devices, and I think they are incorrect in general per comment 7.  Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=66c98e4fe62f
Comment 23 Matt Brubeck (:mbrubeck) 2012-08-28 15:49:20 PDT
Created attachment 656237 [details] [diff] [review]
WIP

This is a sketch of what a "real" fix would look like.  This brings Android (and any other platform using viewport overrides) back in line with the spec changes implemented in bug 771390.  It depends on the patches in bug 716575 which aren't fully reviewed yet.
Comment 24 Matt Brubeck (:mbrubeck) 2012-08-28 18:57:12 PDT
Comment on attachment 656225 [details] [diff] [review]
band-aid (v2)

It looks like this broke Android reftests on Try.  Investigating.
Comment 25 Matt Brubeck (:mbrubeck) 2012-08-28 21:23:30 PDT
Comment on attachment 656225 [details] [diff] [review]
band-aid (v2)

False alarm; the Android R1 oranges were from a bad patch that I pushed on top of.  Here's a Try run with green R1 tests: https://tbpl.mozilla.org/?tree=Try&rev=0a8aee25e69e
Comment 26 David Baron :dbaron: ⌚️UTC-10 2012-08-31 13:19:37 PDT
Comment on attachment 656225 [details] [diff] [review]
band-aid (v2)

r=dbaron as a temporary fix
Comment 27 David Baron :dbaron: ⌚️UTC-10 2012-08-31 13:20:56 PDT
Comment on attachment 656237 [details] [diff] [review]
WIP

This patch is exactly the same as the other one.  Did you mean to attach WIP for the permanent fix?
Comment 28 Matt Brubeck (:mbrubeck) 2012-08-31 13:32:45 PDT
Comment on attachment 656225 [details] [diff] [review]
band-aid (v2)

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d3b0866dd9
Comment 29 Matt Brubeck (:mbrubeck) 2012-08-31 13:34:50 PDT
Created attachment 657427 [details] [diff] [review]
WIP 2

Sorry, here's the correct WIP patch.
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-08-31 18:42:24 PDT
https://hg.mozilla.org/mozilla-central/rev/e7d3b0866dd9
Comment 31 Matt Brubeck (:mbrubeck) 2012-09-02 19:34:20 PDT
Comment on attachment 656225 [details] [diff] [review]
band-aid (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 771390

User impact if declined: "resolution" media query gives incorrect result on high-DPI Android devices.  (This is a web-facing regression that breaks existing web content, as well as some of Fennec's own in-content UI.)

Testing completed (on m-c, etc.): On m-c since 8/31.  The code this touches has good automated test coverage.

Risk to taking this patch (and alternatives if risky): This is a fairly low-risk patch that should have no effect on desktop Firefox.  On mobile Firefox it just reverts the change made by bug 771390.

String or UUID changes made by this patch: None.
Comment 32 Alex Keybl [:akeybl] 2012-09-04 06:34:10 PDT
Comment on attachment 656225 [details] [diff] [review]
band-aid (v2)

[Triage Comment]
Fix for a new FF16 issue, returning us to a known good state.
Comment 33 Matt Brubeck (:mbrubeck) 2012-09-04 08:14:07 PDT
Landed on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/ae36fddd9e3f

Will land on Aurora when the Aurora tree re-opens.
Comment 34 Aaron Train [:aaronmt] 2012-09-04 09:59:28 PDT
Thus far glanced over the reader-mode toolbar on the Galaxy Nexus and the drawables are much crisper now.

Comment #0's URLs:

Galaxy Nexus's reported min-resolution media query: XHDPI
Galaxy Nexus's reported -moz-device-pixel-ratio media query: MDPI
Comment 35 Matt Brubeck (:mbrubeck) 2012-09-04 11:08:00 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/ecd964c8edec
Comment 36 David Baron :dbaron: ⌚️UTC-10 2012-09-04 14:22:28 PDT
Comment on attachment 657427 [details] [diff] [review]
WIP 2

So it would be helpful to have comments here explaining what you intend resolution and device-pixel-ratio to mean when the viewport is overridden.  Presumably they ought to describe the behavior you get with width=device-width or similar patterns, no?

I'm puzzled by the code, though -- it seems like you're doing different things for resolution and device-pixel-ratio -- for one you're multiplying by GetDevicePixelRatio, and for the other you're replacing the other value with it.  I'd expect the same answer for both (not sure which).

Otherwise the approach seems good, though.

Also, you can probably re-remove the appUnitsPerInch variable in GetResolution.
Comment 37 Matt Brubeck (:mbrubeck) 2012-09-25 08:14:54 PDT
I think the right way to solve this may involve changing nsIWidget::GetDefaultScale as discussed in bug 716575 comment 33 and 34.
Comment 38 Mark Finkle (:mfinkle) (use needinfo?) 2012-10-18 10:35:59 PDT
Matt - Can we file a new bug on the remaining work?
Comment 39 Matt Brubeck (:mbrubeck) 2012-10-18 11:35:43 PDT
Filed follow-up bug 803207.

Note You need to log in before you can comment on or make changes to this bug.