Last Comment Bug 813946 - Zooming is enabled on ebay.com (mobile page)
: Zooming is enabled on ebay.com (mobile page)
Status: VERIFIED FIXED
[mentor=kats][lang=js]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 20 Branch
: ARM Android
: -- normal (vote)
: Firefox 20
Assigned To: groodt
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-21 04:55 PST by Cristian Nicolae (:xti)
Modified: 2012-12-06 09:41 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Patch to disable zoom when minScale and maxScale are equal. (1.40 KB, patch)
2012-11-30 14:47 PST, groodt
bugmail.mozilla: feedback+
Details | Diff | Review
Patch for Bug 813946 (1.83 KB, patch)
2012-12-02 11:19 PST, groodt
bugmail.mozilla: review+
Details | Diff | Review

Description Cristian Nicolae (:xti) 2012-11-21 04:55:25 PST
Firefox 20.0a1 (2012-11-20)
Device: Galaxy S2
OS: Android 4.0.3

Steps to reproduce:
1. Go to ebay.com
2. Pinch-to-zoom in/out

Expected result:
Zooming is disabled for mobile pages

Actual result:
Zooming is enabled for ebay.com

Note:
This issue cannot be reproduced on the stock browser.
Comment 1 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-11-21 08:04:40 PST
They have a meta-viewport tag:

width=device-width, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0

this doesn't explicitly set user-scalable=no, so we allow pinching. However they set a min and max scale, so we don't allow actually changing the zoom. This is why our behaviour is a little weird when pinching the page - it'll pinch as long as you have your fingers on but will normalize back to 1.0 when you let go.

I guess we could fix this in browser.js (the getViewportMetadata code) by automatically setting allowZoom = false when minScale and maxScale are both defined and equal to each other.
Comment 2 groodt 2012-11-30 14:47:51 PST
Created attachment 687293 [details] [diff] [review]
Patch to disable zoom when minScale and maxScale are equal.

Intention is to only allow zoom if minScale != maxScale.
Comment 3 groodt 2012-11-30 14:49:36 PST
Tested on my Nexus 7 on the ebay mobile site and it does indeed disable the odd zooming behavior. 

Other sites seem to work correctly.
Comment 4 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-01 00:30:50 PST
Comment on attachment 687293 [details] [diff] [review]
Patch to disable zoom when minScale and maxScale are equal.

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

This looks fine to me. It might be nice to expand that Webkit comment to also mention what we're doing with the new conditional clause. Bonus points if you also point out that NaN != NaN in javascript so if both minScale and maxScale are undefined that clause has no effect.

Please upload an updated patch ready for submission (i.e. includes a commit message with bug number and short description) following the guidelines at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 5 groodt 2012-12-02 11:19:57 PST
Created attachment 687533 [details] [diff] [review]
Patch for Bug 813946
Comment 6 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-03 07:14:19 PST
Comment on attachment 687533 [details] [diff] [review]
Patch for Bug 813946

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

LGTM. Usually when you have a patch you want to be reviewed you should explicitly request review by using the attachment flags (set the review flag to '?' and enter the name of the person you would like to review it).
Comment 7 groodt 2012-12-03 12:26:25 PST
Thanks. I'll do that in future.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-12-03 17:30:08 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce882e41eb1
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-12-04 19:05:26 PST
https://hg.mozilla.org/mozilla-central/rev/6ce882e41eb1
Comment 10 Cristian Nicolae (:xti) 2012-12-06 06:06:03 PST
I cannot zoom the page anymore on the latest Nightly. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-12-06)
Device: Galaxy Tab2 7"
OS: Android 4.0.3
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-12-06 09:41:00 PST
Nice job, groodt! \o/

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