Last Comment Bug 656373 - Turn off Form Assistant zooming, panning and next/prev on tablets
: Turn off Form Assistant zooming, panning and next/prev on tablets
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: Firefox 6
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
Mentors:
Depends on: 648026 657614
Blocks: 655762 657836
  Show dependency treegraph
 
Reported: 2011-05-11 10:44 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2011-08-08 11:36 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (4.60 KB, patch)
2011-05-13 03:33 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Review
Patch (4.60 KB, patch)
2011-05-13 03:35 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
mark.finkle: review-
Details | Diff | Review
Patch v0.2 (5.49 KB, patch)
2011-05-16 04:58 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
mark.finkle: review+
Details | Diff | Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-05-11 10:44:34 PDT
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) 2011-05-12 07:54:23 PDT
(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) 2011-05-13 03:33:15 PDT
Created attachment 532162 [details] [diff] [review]
Patch
Comment 3 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-13 03:35:43 PDT
Created attachment 532163 [details] [diff] [review]
Patch

Oups, left some debug code (the previous version was always disabled)
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-13 21:59:40 PDT
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) 2011-05-16 04:58:21 PDT
Created attachment 532610 [details] [diff] [review]
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?) 2011-05-16 08:18:53 PDT
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) 2011-05-17 02:59:43 PDT
http://hg.mozilla.org/mozilla-central/rev/a6391aea48e5
Comment 8 Anna (Waverley) 2011-05-23 05:17:58 PDT
Can someone having a tablet, please, verify this ?
Comment 9 Kevin Brosnan [:kbrosnan] 2011-05-23 15:16:27 PDT
Verified Ideos s7 - Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110523 Firefox/6.0a1 Fennec/6.0a1 ID:20110523042031

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