Closed Bug 712386 Opened 8 years ago Closed 8 years ago

Flinging after zooming allows unbounded zooming

Categories

(Firefox for Android :: General, defect, P2)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
fennec 11+ ---

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(1 file, 1 obsolete file)

STR:
1. Open some page and wait for it to load
2. Put both fingers down on the screen and pinch zoom in (make content larger)
3. Continue doing step 2 until the zoom is greater than 4.0 (when this happens lifting both fingers will result in the zoom snapping back to 4.0)
4. Do another pinch zoom to drive the zoom above 4.0, then lift one finger and fling the page.

Expected results: After the fling stops, or while the fling is happening, or at some point during this sequence of events, the page reduces the zoom level back to 4.0
Observed results: Page zoom remains above 4.0. Step 4 can be repeated over and over to drive the zoom as high as desired.

A variation of this is to just keep one finger down at all times, and use the other finger to do multiple pinches, one after another. This also allows zooming in as much as desired, instead of clamping it to 4.0.
Assignee: nobody → bugmail.mozilla
Priority: -- → P2
Attached patch Have a hard limit on zoom (obsolete) — Splinter Review
Attachment #583885 - Flags: review?(chrislord.net)
Comment on attachment 583885 [details] [diff] [review]
Have a hard limit on zoom

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

I guess you meant to use ALLOWED_OVERZOOM in the calculation of the zoom past MAX_ZOOM. r+ with this rectified.

::: mobile/android/base/ui/PanZoomController.java
@@ +102,5 @@
>      private static final float MIN_SCROLLABLE_DISTANCE = 0.5f;
>      // The maximum amount we allow you to zoom into a page
>      private static final float MAX_ZOOM = 4.0f;
> +    // The amount we allow (temporarily) zooming up to over MAX_ZOOM. There will be resistance here.
> +    private static final float ALLOWED_OVERZOOM = 2.0f;

This is never used. Either it should be removed, or it should be used in the calculations below.

@@ +943,5 @@
> +                // apply resistance when zooming past MAX_ZOOM,
> +                // such that it asymptotically reaches MAX_ZOOM + 1.0
> +                // but never exceeds that
> +                float excessZoom = newZoomFactor - MAX_ZOOM;
> +                excessZoom = 1.0f - (float)Math.exp(-excessZoom);

I guess this should actually reach MAX_ZOOM + ALLOWED_OVERZOOM and the excessZoom equation should be modified appropriately.
Attachment #583885 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #2)
> I guess you meant to use ALLOWED_OVERZOOM in the calculation of the zoom
> past MAX_ZOOM. r+ with this rectified.

Good catch. I had initially set up ALLOWED_OVERZOOM and was scaling up excessZoom by that before adding it to MAX_ZOOM, but realized that the behaviour with that didn't feel quite right. The problem is that the spanRatio is multiplied on top of the last zoom factor (which already includes some resistance), so the function ends up being not what I expected.

Long story short, it worked better without the ALLOWED_OVERZOOM so I just took that out. Uploading updated patch and carrying r+, will land when tree is open.
Attachment #583885 - Attachment is obsolete: true
Attachment #583907 - Flags: review+
Hardware: All → ARM
Landed on m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f9960e286a
Whiteboard: [inbound], [leave open after inbound merge]
Target Milestone: --- → Firefox 12
Attachment #583907 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58836d3fa78

Backed out on suspicion of causing Android m1 and m2 orange.
Whiteboard: [inbound], [leave open after inbound merge] → [leave open after inbound merge]
Comment on attachment 583907 [details] [diff] [review]
Have a hard limit on zoom (v2)

Investigating oranges
Attachment #583907 - Flags: approval-mozilla-aurora?
Re-landed on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/18eead5419ff
Try run ran green here: https://tbpl.mozilla.org/?tree=Try&rev=d5f66b34017d

Random oranges appear to be caused by the patch for bug 712761.
Whiteboard: [leave open after inbound merge] → [leave open after inbound merge], [inbound]
https://hg.mozilla.org/mozilla-central/rev/18eead5419ff
Whiteboard: [leave open after inbound merge], [inbound]
Attachment #583907 - Flags: approval-mozilla-aurora?
Comment on attachment 583907 [details] [diff] [review]
Have a hard limit on zoom (v2)

[triage comment]
Approved for aurora. Polish issue, mobile only.

Please remember to land the patch as it landed on m-c (looks like there was an additional change needed for the r+).
Attachment #583907 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/2f0e91546c19
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 12 → Firefox 11
tracking-fennec: --- → 11+
Verified fixed on Firefox 11.0a2 (2012-01-10)
                          12.0a1 (2012-01-10)
Device: Samsung Galaxy SII (Android 2.3)
Status: RESOLVED → VERIFIED
Fixing flags I messed up when I landed this to aurora.
Target Milestone: Firefox 11 → Firefox 12
You need to log in before you can comment on or make changes to this bug.