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

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

Trunk
All
Android
Dependency tree / graph

Details

(Whiteboard: [has-patch])

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
tracking-fennec: --- → ?

Comment 1

8 years ago
We should do all of these based on physical units and use the DPI that the platform already knows about.
Depends on: 590817
marking blocking 2.0+ for a decision on supporting MDPI and LDPI
tracking-fennec: ? → 2.0+
(Assignee)

Updated

8 years ago
Depends on: 602322
(Assignee)

Updated

8 years ago
Depends on: 602647

Updated

8 years ago
Depends on: 605024
Let's look at getting infra to get this working
tracking-fennec: 2.0+ → 2.0b3+
Assignee: nobody → mark.finkle
(Assignee)

Updated

8 years ago
Assignee: mark.finkle → mbrubeck
(Assignee)

Comment 4

8 years ago
Created attachment 490756 [details] [diff] [review]
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.
Attachment #490756 - Flags: review?(mark.finkle)
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
Attachment #490756 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 6

8 years ago
Created attachment 490921 [details] [diff] [review]
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
Attachment #490921 - Flags: review?(mark.finkle)
(Assignee)

Comment 7

8 years ago
Created attachment 490929 [details] [diff] [review]
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.
Attachment #490921 - Attachment is obsolete: true
Attachment #490929 - Flags: review?(mark.finkle)
Attachment #490921 - Flags: review?(mark.finkle)
(Assignee)

Comment 8

8 years ago
Created attachment 490994 [details] [diff] [review]
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...
Attachment #490994 - Flags: review?(mark.finkle)
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
Attachment #490929 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 10

8 years ago
Created attachment 491038 [details] [diff] [review]
part 4: horrible Maemo displayDPI hack

X11 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.
Attachment #491038 - Flags: review?(mark.finkle)
(Assignee)

Comment 11

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #491038 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Depends on: 612978
(Assignee)

Comment 12

8 years ago
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.
Attachment #491038 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
Created attachment 491296 [details] [diff] [review]
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.
Attachment #491296 - Flags: review?(blassey.bugs)
(Assignee)

Updated

8 years ago
Whiteboard: [has-patch]
Attachment #491296 - Flags: review?(blassey.bugs) → review+
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
Attachment #490994 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 15

8 years ago
Created attachment 491341 [details] [diff] [review]
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.
Attachment #490929 - Attachment is obsolete: true
Attachment #491341 - Flags: review?(mark.finkle)
Attachment #491341 - Attachment is patch: true
Attachment #491341 - Attachment mime type: application/octet-stream → text/plain
Attachment #491341 - Flags: review?(mark.finkle) → review+
Verified on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.