590777 - Support low-density and low-resolution (MDPI and LDPI) displays

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Android supports three screen density ranges: HDPI (~240 dpi), MDPI (~160 dpi), and LDPI (~120 dpi):
http://developer.android.com/guide/practices/screens_support.html

Fennec's currently assumes an HDPI display.  We should instead query the OS for the screen density, and adjust certain constants:

* Swipe and pinch gesture thresholds (e.g. SWIPE_MIN_DISTANCE)
* Double-tap radius (kDoubleClickRadius)
* Smart-tap radius (browser.ui.touch.*)
* Favicon size (places.favicons.optimizeToDimension)
* ratio of screen pixels to CSS pixels (zoom.dpiScale)
* ...and probably others

The Android theme (bug 575403) will also need variations for each density.

Comment 1

[Stuart Parmenter]

We should do all of these based on physical units and use the DPI that the platform already knows about.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

marking blocking 2.0+ for a decision on supporting MDPI and LDPI

Comment 3

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

Let's look at getting infra to get this working

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: Viewport metadata cleanup

This is just some preliminary refactoring, to make the next patch easier to read.  This does not contain any major behavior changes.  All existing viewport tests are still green with this patch.

* Move all "dpiScale" math into the parent process.
* Simplify some variable names for readability.
* Allow "0" and "false" for user-scalable, to match WebKit.
* Add tests for the new user-scalable values.

Comment 5

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

Comment on attachment 490756 [details] [diff] [review]
part 1: Viewport metadata cleanup

"unscaled" confused me for a minute. I wasn't sure if it meant "these values are not scaled, but need to be" or "these values are not to be scaled". After reviewing the chrome and content sides of the patch I see now it means "these values are not to be scaled".

Could we change "scaled" to "autoScale" and flip the logic? We already use "autoSize" as a similar hint to the chrome side. Also, can we set "autoScale: true" for the viewport section. I like explicit. You can keep the | "autoScale" in metadata | check in the chrome side for completeness.

r+ but consider the changes

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Viewport scale

This patch replaces the "zoom.dpiScale" pref and its hard-coded default with a value calculated based on on the display density.  The new calculation will match previous Fennec behavior on our ~240dpi target hardware.  It will match iPhone and Android behavior on other popular display types.

There is still a "scaleRatio" pref that can override the automatic calculation.  The tests use this pref to simulate an HDPI device even when running on desktop.  I also added a new test to ensure that other ratios work.

Pushed part 1: http://hg.mozilla.org/mobile-browser/rev/884c5519f311

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Viewport scale (v2)

Fixes an error in the previous version.

Side note: This changes the behavior of Fennec on a typical desktop display - it will act like it is running on an ~100dpi device, instead of on a ~240dpi device like it does now.  Maybe we should have a "Desktop tester tools" extension that ships with our desktop builds, and provides a simple UI for emulating different devices.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: part 3: miscellaneous constants

This replaces the following hard-coded pixel values with calculated ones:

* kDoubleClickRadius
* browser.ui.touch.*
* dragThresholdX/Y

It also adjusts cachePixelX/Y when they are not big enough to cover the whole screen, for example when cachePixelX is at the default value of 580 and the screen is more than 1160 pixels wide.  This prevents us from showing unrendered gray/checkerboard areas on very large displays.

We might want to be even smarter about this, and always keep these values proportional to the window size...

Comment 9

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

Comment on attachment 490929 [details] [diff] [review]
part 2: Viewport scale (v2)


>diff -r 884c5519f311 chrome/content/browser.js

>   // The device-pixel-to-CSS-px ratio used to adjust meta viewport values.
>   // This is higher on higher-dpi displays, so pages stay about the same physical size.
>   getScaleRatio: function getScaleRatio() {
>-    return Services.prefs.getIntPref("zoom.dpiScale") / 100;
>+    let prefValue = Services.prefs.getIntPref("browser.viewport.scaleRatio");
>+    if (prefValue > 0)
>+      return prefValue / 100;
>+
>+    let dpi = Browser.windowUtils.displayDPI;
>+    if (dpi < 200) // Includes desktop displays, and LDPI and MDPI Android devices
>+      return 1;
>+    else if (dpi < 300) // Includes Nokia N900, and HDPI Android devices
>+      return 1.5;
>+

I almost suggested that we memoize this function by converting to a property and only doing the calculation once. But then I saw that the preference would only be read once, and a restart would be needed.

I guess the performance won't be adversely affected

>+    // For very high-density displays like the iPhone 4, calculate an integer ratio.
>+    return Math.floor(dpi/150);

dpi / 150

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: part 4: horrible Maemo displayDPI hack

11 on Maemo always reports 96 dpi, even though the actual display density of the N900 is around 260 dpi.  We can't land parts 2 and 3 without fixing this.  Doug reported this upstream in 2009: https://bugs.maemo.org/show_bug.cgi?id=4825

I can file a followup bug to fix this in the platform; for now this front-end patch lets us run the other patches on Maemo hardware.  Not sure if we'd want to actually take this for b3, or figure out the right platform fix.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 491038 [details] [diff] [review]
part 4: horrible Maemo displayDPI hack

I should add that any Mozilla platform fix will probably be equally hacky, and with more potential side effects.  So we might want to take this mobile-browser hack, because we can control what code is affected by the hard-coded value.

From our point of view, the best way to fix this would be upstream.  However, X11 tends to pretend that all displays are 96dpi for the same reason that Gecko's CSS engine does, for backward compatibility and interop.

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 491038 [details] [diff] [review]
part 4: horrible Maemo displayDPI hack

I filed bug 612978 for fixing GetDPI on Maemo in the platform instead.

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: part 4 of 4: swipe thresholds

Set the swipe gesture thresholds in inches instead of pixels.

This is the last patch I have planned for this bug; all other work is already covered by dependent bugs.

Comment 14

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

Comment on attachment 490994 [details] [diff] [review]
part 3: miscellaneous constants

>diff -r 9e264fb30787 app/mobile.js

>-// Drag thresholds
>-pref("ui.dragThresholdX", 25);
>-pref("ui.dragThresholdY", 25);
>+// threshold in mils (thousands of an inch) before a tap becomes a drag
>+pref("ui.dragThresholdMils", 100);

Why not use the 240px reference DPI here too? It would make things a bit more consistent

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 of 4: Viewport scale (v3)

I fixed a couple of problems with this patch:

1) On pages with autoSize=true, the zoom levels was increased every time the page resized.  Fixed this by applying the scale ratio only when the metadata changes, not when the page resizes.  (This has the side benefit that we fetch the pref and do the calculation less often.)

2) On a page with "width=device-width" but no initial-scale, the scaling resulted in a divide-by-NaN.  I changed the autoSize width calculation to use scaleRatio instead of defaultZoom, and added a test for this case.

Also some minor cleanup to remove an unused member variable in content.js.

Comment 16

[Matt Brubeck (:mbrubeck)]

Push parts 2-4:
http://hg.mozilla.org/mobile-browser/rev/f41ccd9f795e
http://hg.mozilla.org/mobile-browser/rev/be135f4d1a63
http://hg.mozilla.org/mozilla-central/rev/035fcd2ac561

Comment 17

[Catalin Suciu [:csuciu]]

Verified on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1