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

VERIFIED FIXED in Firefox 6

Status

defect
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mfinkle, Assigned: vingtetun)

Tracking

Trunk
Firefox 6
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox6 fixed, fennec6+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Blocks: 655762
Summary: Turn off Form Assistant zooming. panning and next/prev on tablets → Turn off Form Assistant zooming, panning and next/prev on tablets
(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)
Depends on: 648026
Posted patch Patch (obsolete) — Splinter Review
Attachment #532162 - Flags: review?(mark.finkle)
Posted patch Patch (obsolete) — Splinter Review
Oups, left some debug code (the previous version was always disabled)
Attachment #532162 - Attachment is obsolete: true
Attachment #532162 - Flags: review?(mark.finkle)
Attachment #532163 - Flags: review?(mark.finkle)
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
Attachment #532163 - Flags: review?(mark.finkle) → review-
Posted patch Patch v0.2Splinter Review
(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)
Attachment #532163 - Attachment is obsolete: true
Attachment #532610 - Flags: review?(mark.finkle)
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.
Attachment #532610 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mozilla-central/rev/a6391aea48e5
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 657614
Blocks: 657836

Comment 8

8 years ago
Can someone having a tablet, please, verify this ?
Verified Ideos s7 - Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110523 Firefox/6.0a1 Fennec/6.0a1 ID:20110523042031
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 6
You need to log in before you can comment on or make changes to this bug.