656373 - Turn off Form Assistant zooming, panning and next/prev on tablets

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Due to the large screen sizes, we do not need (or want) to zoom/pan to form fields. We also do not need the next/prev buttons to appear. Tablet keyboards have a "Tab" button to make moving between fields easier.

We still want the combobox UI and the form suggestion bubble.

We should try to trigger this based on screen size. We are using >800 px in a different tablet UI bug.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #0)
> We still want the combobox UI and the form suggestion bubble.

If you want the form suggestion bubble this is another good reason to move it out of FormHelperUI (bug 648026)

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Oups, left some debug code (the previous version was always disabled)

Comment 4

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

Comment on attachment 532163 [details] [diff] [review]
Patch


>+    // Dynamically enabled/disabled the form helper if needed
>+    let mode = Services.prefs.getIntPref("formhelper.mode");
>+    let state = (mode == 2) ? (window.innerWidth <= 480) : !!mode;

I think we want to use a physical length. We are using 124 mm in a different place as a tablet trigger.

See http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/input.js#571  for getting the DPI

if my math is right, this should work:
      let dpmm = DPI / 25.4;
      let state = (mode == 2 ? ((window.innerWidth / dpmm) <= 124) : !!mode);

>+    Services.prefs.setBoolPref("formhelper.enabled", state);

Instead of using "formhelper.enabled" can we just move this into the "enabled" getter? and make it memoized

>       case "FormAssist:Hide":
>-        this.enabled ? this.hide()
>-                     : SelectHelperUI.hide();
>+        if (this.enabled)
>+          this.hide();
>+        else {
>+          SelectHelperUI.hide();
>+          ContentPopupHelper.popup = null;
>+        }

Use { } around the "if" part

r- for the nits

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

(In reply to comment #4)
> Instead of using "formhelper.enabled" can we just move this into the
> "enabled" getter? and make it memoized

forms.js use formhelper.enabled too but can't access window.innerWidth (since it use a fake viewport)

Comment 6

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

Comment on attachment 532610 [details] [diff] [review]
Patch v0.2

>diff --git a/mobile/chrome/content/Util.js b/mobile/chrome/content/Util.js

>     return (!appInfo || appInfo.getService(Ci.nsIXULRuntime).processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT);
>   },
> 
>+  isTabletSized: function isTablet() {

I kinda prefer "isTablet"

>diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js

>+    // Dynamically enabled/disabled the form helper if needed depending on
>+    // the size of the screen
>+    let mode = Services.prefs.getIntPref("formhelper.mode");
>+
>+    // See the tablet_panel_minwidth from mobile/themes/core/defines.inc
>+    let tablet_panel_minwidth = 124;
>+    let dpmm = Util.getWindowUtils(window).displayDPI / 25.4;

You can remove this code, right? You're using Utils.isTablet()

r+ with the nits fixed

We could use a test for this. Running it on phones would at least let us know the FormHelper is active for small screens.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/a6391aea48e5

Comment 8

[Anna (Waverley)]

Can someone having a tablet, please, verify this ?

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

Verified Ideos s7 - Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110523 Firefox/6.0a1 Fennec/6.0a1 ID:20110523042031