Closed Bug 1116048 Opened 9 years ago Closed 9 years ago

The 'browser.ui.zoom.force-user-scalable' pref is not toggleable

Categories

(Firefox for Android Graveyard :: General, defect)

36 Branch
All
Android
defect
Not set
normal

Tracking

(firefox34 unaffected, firefox35 wontfix, firefox36 verified, firefox37 verified, fennec36+)

VERIFIED FIXED
Firefox 37
Tracking Status
firefox34 --- unaffected
firefox35 --- wontfix
firefox36 --- verified
firefox37 --- verified
fennec 36+ ---

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(1 file, 1 obsolete file)

STR:
1. Start Fennec, load about:config
2. Type "force-" in the search box. This should make the browser.ui.zoom.force-user-scalable pref appear at the top of the list
3. Tap on the pref to give it focus
4. Tap on it again to toggle it (or hit the toggle button if you prefer)

ER:
Pref toggles

AR:
Pref toggles, then immediately toggles back. i.e. there's no way to actually change the value of the pref.

Regression in Fennec 35 from what I can tell using the builds I have installed on my device.
tracking-fennec: --- → ?
It looks like what's happening is the onSingleTapUp [1] fires with double tap disallowed (so waitForDoubleTap() returns false). This flips the pref and changes the zoom constraints so that now double tap is allowed. This means when onSingleTapConfirmed [2] runs waitForDoubleTap() returns true and another Gesture:SingleTap is dispatched.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java?rev=8081294c87ee#1362
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java?rev=8081294c87ee#1375
Attached patch Patch (obsolete) — Splinter Review
Kind of hacky but I couldn't think of anything better.
Attachment #8541996 - Flags: review?(bnicholson)
Comment on attachment 8541996 [details] [diff] [review]
Patch

Review of attachment 8541996 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, but I think we can make this less hacky. How about the following:
1) Rename mMediumPress to mWaitForDoubleTap.
2) Set mWaitForDoubleTap = mTarget.getZoomConstraints().getAllowDoubleTapZoom() at [1].
3) Set mWaitForDoubleTap to false at [2].
4) Remove waitForDoubleTap(), and check the boolean directly instead (which won't change when the pref changes).

Makes sense in my head, but feel free to reflag r? if this won't work for some reason.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java?rev=8081294c87ee#1336
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java?rev=8081294c87ee#1348
Attachment #8541996 - Flags: review?(bnicholson) → review-
That is much cleaner, thanks!
Attached patch Patch v2Splinter Review
Assignee: nobody → bugmail.mozilla
Attachment #8541996 - Attachment is obsolete: true
Attachment #8542173 - Flags: review?(bnicholson)
Comment on attachment 8542173 [details] [diff] [review]
Patch v2

Review of attachment 8542173 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +122,5 @@
>      /* The per-frame zoom delta for the currently-running AUTONAV animation. */
>      private float mAutonavZoomDelta;
>      /* The user selected panning mode */
>      private AxisLockMode mMode;
> +    /* Whether or to wait for a double-tap before dispatching a single-tap */

Nit: "whether or not" or just "whether".
Attachment #8542173 - Flags: review?(bnicholson) → review+
Comment on attachment 8542173 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: unsure, something in Fx35
[User impact if declined]: unable to toggle the browser.ui.zoom.force-user-scalable pref from false to true using about:config in Fennec. (that pref specifically is the only one affected)
[Describe test coverage new/current, TBPL]: tested locally
[Risks and why]: should be pretty low risk; relatively small patch in fennec-only code
[String/UUID change made/needed]: none

Even though this is a regression in 35 it's pretty late in the beta cycle and I don't think it's worth uplifting. I don't think that many people flip this pref that it will be an issue. People who have already flipped the pref from false to true shouldn't be affected unless they flip it back to false and then try to flip it back to true again.
Attachment #8542173 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/da105bbc729c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Attachment #8542173 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tapping on the "toggle" button, the pref will change into true and will remain so. Tapping once again it will become false, so:
Verified as fixed on:
Device: Alcatel One Touch
OS: Android 4.1.2
Build: Firefox for Android 37.0a1 (2015-01-05) and Firefox for Android 36.0a2 (2014-01-05)
Status: RESOLVED → VERIFIED
tracking-fennec: ? → 36+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.