813946 - Zooming is enabled on ebay.com (mobile page)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

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

[Kartikaya Gupta (email:[e-mail])]

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]

Attachment: Patch to disable zoom when minScale and maxScale are equal.

Intention is to only allow zoom if minScale != maxScale.

Comment 3

[groodt]

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

[Kartikaya Gupta (email:[e-mail])]

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]

Attachment: Patch for Bug 813946

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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]

Thanks. I'll do that in future.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce882e41eb1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ce882e41eb1

Comment 10

[Cristian Nicolae (:xti)]

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?)]

Nice job, groodt! \o/