561455 - Change the ratio of CSS/viewport px to physical pixels on high density screens

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Matt Brubeck (:mbrubeck)]

In Fennec with a zoom level of 100% (e.g. if it sets initial-scale=1.0), 1px in CSS is equal to one physical pixel, and the "device-width" in media queries and viewport metadata is equal to the screen width in physical pixels.

This means that at a given "scale", text and images are drawn much smaller (physically) on a high-density display than on a low-density display of the same physical size.

It also hurts interoperability with Android and iPhone browsers.  The iPhone's screen is about 160 ppi and 320 pixels wide.  Mobile Safari uses a 1:1 ratio of physical pixels to CSS px.  Android behaves the same as the iPhone on devices with the same "mdpi" display density and size, but on "hdpi" screens (240 ppi and 480 pixels wide) it uses a 3:2 ratio of physical to CSS px.

This means that a site with initial-scale=1.0 will appear the same physical size on the iPhone and on Android devices of either pixel density.  But on Fennec on the N900 or on hdpi Android devices it will be about 33% smaller.  Also Android and iPhone report a device-width of 320 on a 3"-4" screen regardless of density, while Fennec reports a device-width of 480px on some of the same high-density screens.

PPK makes a convincing argument that Apple will use a 2:1 ratio of physical pixels to CSS px on iPhone devices with 960x640 (~320ppi) screens:
http://www.quirksmode.org/blog/archives/2010/04/a_pixel_is_not.html

The CSS 2.1 specification says that CSS pixel units should not be the same as device pixels on high-density displays:

"Pixel units are relative to the resolution of the viewing device, i.e., most often a computer display. If the pixel density of the output device is very different from that of a typical computer display, the user agent should rescale pixel values. It is recommended that the pixel unit refer to the whole number of device pixels that best approximates the reference pixel. It is recommended that the reference pixel be the visual angle of one pixel on a device with a pixel density of 96dpi and a distance from the reader of an arm's length. For a nominal arm's length of 28 inches, the visual angle is therefore about 0.0213 degrees."
http://www.w3.org/TR/CSS2/syndata.html#length-units

For Fennec, this basically means that viewport width and scale values should be adjusted by a factor of 1.5 on the N900 and on hdpi Android devices.  I believe this would give a better user experience, and maximum interoperability for mobile web content authors.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: WIP

Does this look about right?  All the meta tag stuff is scaled down by a DPI factor of .64 in preferences (zoom.dpi).  I did this so that 500px width (which IIRC is N900's width in portrait) would become 320px.  Other suggestions are welcome.

This makes Google Reader, Wikipedia, and Facebook all very readable for their touch versions, yay!

Since firstrun and startpage were designed for hdpi viewports, I just hacked the meta viewport tag to make them scale correctly.

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 442248 [details] [diff] [review]
WIP

>+pref("zoom.dpiscale", 64);

Instead of 64/100 this should be 667/1000, since our high-dpi devices are 480 pixels wide, not 500.

(Man, I wish we allowed floating-point preference values.)

>+//    let maxInitialScale = viewportScale || viewportMaxScale;
>+//    if (maxInitialScale && viewportWidth)
>+//      viewportWidth = Math.max(viewportWidth, window.innerWidth / maxInitialScale);

We should get this working again; otherwise lots of iPhone-optimized sites like http://sudoku.rectang.com/iphone/ will look weird in Fennec's landscape mode.

>-  <meta name="viewport" content="width=device-width; initial-scale=1.0" />
>+  <meta name="viewport" content="width=1250; initial-scale=.64" />

This makes these pages too wide in portrait mode.  Once the "maxInitialScale" code above is uncommented, we can fix this by setting something like "width=480; initial-scale=.667" instead.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 442248 [details] [diff] [review]
WIP

>diff --git a/app/mobile.js b/app/mobile.js

> pref("zoom.minPercent", 20);
> pref("zoom.maxPercent", 400);
> pref("toolkit.zoomManager.zoomValues", ".2,.3,.5,.67,.8,.9,1,1.1,1.2,1.33,1.5,1.7,2,2.4,3,4");
>+pref("zoom.dpiscale", 64);

"zoom.dpiScale" matches the other zoom prefs

This patch needs tests and probably affects the existing tests.

(plus all of Matt's comments)

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm not 100% sold on this idea either. Scaling up the viewport can make websites look worse.

Comment 5

[Matt Brubeck (:mbrubeck)]

If we've done it right, this change will affect only websites that set viewport scale explicitly - and it will make them the same approximate physical size on all Fennec, Android, and Mobile Safari devices.

Right now a content author who makes a touch-optimized interface will see it rendered at one size on Android and Mobile Safari, and a much smaller (often harder to use) size on Fennec.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: WIP v2

Code review comments addressed.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: WIP v2.1

Same as before but firstrun is updated too.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 442504 [details] [diff] [review]
WIP v2.1

>     // If initial scale is 1.0 and width is not set, assume width=device-width
>     if (viewportScale == 1.0 && !viewportWidthStr)
>       viewportWidthStr = "device-width";

We need to do this comparison *before* we modify viewportScale.

This is looking pretty good.  But yeah, major test breakage.

The patch doesn't affect WAP/WML/XHTMLMobile sites like http://m.facebook.com/.  We should probably treat all those "handheld" page types as though they had initial-scale=1.0, width=device-width and then send them through the same set of calculations as HTML pages.

Comment 9

[Benjamin Stover (:stechz)]

Attachment: WIP v3

Fixes bitrot and initial scale bug and adds a descriptive comment of what's going on.

Still need to fix tests and apply this to handheld sites as mentioned above.

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 442523 [details] [diff] [review]
WIP v3

>+    viewportScale *= dpiScale;
>+    viewportMinScale *= dpiScale;
>+    viewportMaxScale *= dpiScale;
>+    viewportWidth /= dpiScale;
>+    viewportHeight /= dpiScale;
>+
>     viewportWidth  = Util.clamp(viewportWidth,  kViewportMinWidth,  kViewportMaxWidth);
>     viewportHeight = Util.clamp(viewportHeight, kViewportMinHeight, kViewportMaxHeight);

We should apply all the "clamp" calls before the dpiScale adjustments, so the limits have a consistent affect on devices with different DPI (and consistent with Mobile Safari, whose limits we used).

Comment 11

[Benjamin Stover (:stechz)]

I swear I moved those lines above.  Added to laundry list of things to do for this patch.

Comment 12

[Benjamin Stover (:stechz)]

Attachment: WIP v4

All comments addressed.  Tests have been updated, and they now make assumptions about browser width (800px) and DPI scale (1.5).  Also, the expected data for the tests is now put in the HTML files.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 442852 [details] [diff] [review]
WIP v4

This looks awesome.  I'm pretty sure it's all correct except for one problem with handheld/mobile doctypes (below).  The tests look good, and it works perfectly with the sites I found in the wild - especially iPhone-optimized sites like http://sudoku.rectang.com/iphone/.

>-      return { reason: "doctype", result: true, scale: 1.0 };
>+      return { reason: "doctype", result: true, scale: dpiScale, width: window.innerWidth / dpiScale };

We don't actually do anything with the "width" field when reason =~ /doctype|handheld/, so this change doesn't work.  If you load http://m.facebook.com/ it does not get set to the device width.  We could instead update the styles for .browser-handheld, or  get rid of .browser-handheld and use a single code path for /doctype|handheld|viewport/.

We need a testcase for the handheld/doctype case too.

By the way, we don't use the result:true anywhere, do we?

>+    viewportScale *= dpiScale;
>+    viewportMinScale *= dpiScale;
>+    viewportMaxScale *= dpiScale;

I figured out that these lines were breaking my brain, because we're using the same "scale" variables to talk about one ratio, and then a different ratio later on in the same function.  My thinking was much clearer once I started using different names for these two concepts:

    let defaultZoom = viewportScale    * dpiScale;
    let minZoom     = viewportMinScale * dpiScale;
    let maxZoom     = viewportMaxScale * dpiScale;

I'll upload a patch to show what I mean.

>+++ b/chrome/content/aboutHome.xhtml
>+  <meta name="viewport" content="width=720; initial-scale=.667; initial-scale=.667" />
>+++ b/chrome/content/firstrun/firstrun.xhtml
>+  <meta name="viewport" content="width=720; initial-scale=.667; maximum-scale=.667; user-scalable=0;"/>

These should both be "width=480" to work in portrait mode.  See Bug 563136 for a convenient new way to test this. :)

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: additional patch: new variable names

This patch depends on Ben's.  It doesn't change any results, just introduces some new variable names for extra clarity (I hope).  All tests still pass.

Comment 15

[Matt Brubeck (:mbrubeck)]

>+++ b/chrome/content/aboutHome.xhtml
>+  <meta name="viewport" content="width=720; initial-scale=.667; initial-scale=.667" />

Two more things:
* There are two initial-scales here; I think you mean maximum-scale?
* This ends up with zoomLevel=1.005.  If you use .6667 instead then you'll get zoomLevel=1.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: updated patch

Taking this bug at Ben's request.  Updated patch:

* Add new variable names, as above.
* Fix width of handheld/wap/wml/mobile doctype pages, and add a test case.
* Fix width and scale of about:home and about:firstrun.
* Remove unused code.

Comment 17

[Benjamin Stover (:stechz)]

No test 09.html?

Comment 18

[Matt Brubeck (:mbrubeck)]

(In reply to comment #17)
> No test 09.html?

It's there - it's the last hunk in the patch.  The Bugzilla "diff view" labels it weirdly.

Comment 19

[Benjamin Stover (:stechz)]

Comment on attachment 443132 [details] [diff] [review]
updated patch

Oops, missed it.  Looks good to me, the variable renames do help a lot.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 443132 [details] [diff] [review]
updated patch


>+    // Device size in CSS pixels:
>+    let deviceWidth  = window.innerWidth  / dpiScale;
>+    let deviceHeight = window.innerHeight / dpiScale;

nit: We don't use pretty alignment

Nice cleanup. This is starting to make more sense too. Yay for more tests!

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/51ab43c68413

baking on m-b before pushing to m-1.1

Comment 22

[Dan Fabulich]

FYI, the Android team has implemented a custom meta viewport property to handle this case called "target-densityDpi."  It's totally undocumented; you can read more about it in the check-in comment: https://android.git.kernel.org/?p=platform/external/webkit.git;a=commit;h=f10585d69aaccf4c1b021df143ee0f08e338cf31

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-1.1:
http://hg.mozilla.org/releases/mobile-1.1/rev/99a5b8889040