Closed Bug 830760 Opened 8 years ago Closed 8 years ago

Disable zoom into fields on pages with metaviewport and on tablets

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
fennec 21+ ---

People

(Reporter: pwd.mozilla, Assigned: wesj)

References

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130114 Firefox/21.0
Build ID: 20130114031033

Steps to reproduce:

Attempting to enter search term on Google hides the search button due to unnecessarily zooming in.
Blocks: 725018
OS: Windows 7 → Android
Hardware: x86 → ARM
Agreed it's kind of silly.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Google's should just disable zooming on their mobile sites. But I think we'll probably just disable this on sites with a metaviewport, as well as on tablets. Its main use is when using desktop sites on phones.
Shouldn't the onus be on us? I mean we've put out numerous releases that didn't have the zoom feature and the internet continued to work. We've now added a feature which turns the existing Firefox user experience on its head, for the worse, without a user preference and we're kind of expecting the internet to suddenly conform.

Are we confident that our Evangelism department has the resources to get sites to disable zooming? Or even in a timely manner? Perhaps we should add a preference and should the majority of users opt-in to the feature, we enable it by default. Bug 725018 attempts to fix something that isn't broken and by creating an opt-in preference (perhaps under the umbrella of accessibility) we can at least allow the Evangelism Team time to work and not disrupt the user experience of the majority of Firefox users that have been more than fine without the zooming?
(In reply to Paul [sabret00the] from comment #3)
> Shouldn't the onus be on us?

I did put up a solution for us. We'll implement it. We can also do some math to be smarter about when we zoom, but for a second round I'd like to try this first. I'm stealing this bug for it. If we get close to Aurora/Beta/Release and decide this isn't ready for that train, its simple to turn off. I also don't mind adding a (hidden?) pref as well. You should file a bug and we can discuss that there.

This is nightly. Expect features to land that "turn the existing Firefox user experience on its head". Expect that at times you'll find it for the worse. Feedback on those features is helpful and useful (that's why we land them). Thanks!
Summary: Google user experience is broken as a result of Bug 725018 → Disable zoom into fields on pages with metaviewport and on tablets
tracking-fennec: ? → 21+
Attached patch PatchSplinter Review
Assignee: nobody → wjohnston
Attachment #705004 - Flags: review?(bugmail.mozilla)
Comment on attachment 705004 [details] [diff] [review]
Patch

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

r=me with nit addressed.

::: mobile/android/chrome/content/browser.js
@@ +5414,5 @@
>      // http://developer.apple.com/safari/library/documentation/AppleApplications/Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html
>  
>      // Note: These values will be NaN if parseFloat or parseInt doesn't find a number.
>      // Remember that NaN is contagious: Math.max(1, NaN) == Math.min(1, NaN) == NaN.
> +    let hasViewport = true;

I'd prefer if this were renamed to hasMetaViewport or something else less ambiguous.
Attachment #705004 - Flags: review?(bugmail.mozilla) → review+
Forgot to update the test. backed out for now. will fix tomorrow:

https://hg.mozilla.org/integration/mozilla-inbound/rev/391856672bb5
Duplicate of this bug: 839620
Ping?
Attached patch Follow up patch (obsolete) — Splinter Review
So the old patch disabled this entirely for tablets and metaviewport pages, but we actually want to maintain the scrollTo behaviour, and just remove the zooming. This applied on top of the first patch to do that.

I also fixed a bug dealing with scrolling to the element in situations where we disabled zooming. We would use cssPageLeft which is typically zero, rather than cssX (i.e. don't allow horizontal scrolling).

Also fixed up the tests to test both situations (tapping a field on pages with and without a viewport).
Attachment #711989 - Flags: review?(bugmail.mozilla)
Comment on attachment 711989 [details] [diff] [review]
Follow up patch

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

The change itself looks fine, but the test looks wrong.

::: mobile/android/base/tests/testVkbOverlap.java.in
@@ +74,5 @@
> +            // if zooming is allowed, the number of green pixels visible should have increased substatially
> +            if (shouldZoom)
> +                mAsserter.ok(newCount > greenPixelCount, "testVkbOverlap", "Found " + newCount + " green pixels after tapping; expected " + greenPixelCount);
> +            else
> +                mAsserter.ok(newCount >= (greenPixelCount * 0.9), "testVkbOverlap", "Found " + newCount + " green pixels after tapping; expected " + greenPixelCount);

This test seems wrong to me. If we're not zooming, then the green pixel count shouldn't go up. It should be newcount == greenPixelCount, or maybe something more fuzzy like (Math.abs(greenPixelCount - newCount) / greenPixelCount < 0.1). The way it is currently written it will still pass if we zoom.
Attachment #711989 - Flags: review?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> This test seems wrong to me. If we're not zooming, then the green pixel
> count shouldn't go up. It should be newcount == greenPixelCount, or maybe
> something more fuzzy like (Math.abs(greenPixelCount - newCount) /
> greenPixelCount < 0.1). The way it is currently written it will still pass
> if we zoom.
Ahh. Good point. I was using the condition we used before we added the zooming feature [1]. Fixing...

http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/tests/testVkbOverlap.java.in#67
This alters the test to just make sure the number of green pixels is within 10% of the original value.
Attachment #711989 - Attachment is obsolete: true
Attachment #712682 - Flags: review?(bugmail.mozilla)
Comment on attachment 712682 [details] [diff] [review]
Follow up patch v2

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

r=me with comment addressed.

::: mobile/android/base/tests/testVkbOverlap.java.in
@@ +73,5 @@
>              int newCount = countGreenPixels(painted);
> +
> +            // if zooming is allowed, the number of green pixels visible should have increased substatially
> +            if (shouldZoom)
> +                mAsserter.ok(newCount > greenPixelCount, "testVkbOverlap", "Found " + newCount + " green pixels after tapping; expected " + greenPixelCount);

Since the else clause is fuzzy, it makes sense to change this condition to "newCount > greenPixelCount * 1.5" or something along those lines. Otherwise there's a range of values for newCount where both tests will pass and if we fall into that case the test will be undetectably useless.

Also, nit: please add braces around the single line if/else bodies.
Attachment #712682 - Flags: review?(bugmail.mozilla) → review+
Wrong bug number in commit message. backedout and relanded to fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/24090bdf0393 - backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9261d6e9efe - reland
https://hg.mozilla.org/mozilla-central/rev/e9261d6e9efe
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Duplicate of this bug: 839199
Verified fixed on:
-build:  Firefox for Android 21.0a1 (2013-02-18)
-device: Samsung Galaxy Nexus
-OS: Android 4.2.1
Status: RESOLVED → VERIFIED
Also verified fixed on tablet:
-build: Firefox for Android 21.0a1 (2013-02-18)
-device: Samsung Galaxy Tab 10.1
-OS: Android 3.1
You need to log in before you can comment on or make changes to this bug.