1014708 - Disable zoom for find in page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Bug 958111 added zoom for find in page, but we need to refine the behavior (bug 1014113). Until we do that, we should disable this. I'll write a patch.

Comment 1

[:Margaret Leibovic]

Attachment: Disable zoom for find in page

I think we should land this on m-c and uplift to aurora and beta. Then we can undo it as part of the fix for bug 1014113.

Comment 2

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/cff9a2cee111

Comment 3

[:Margaret Leibovic]

Comment on attachment 8427182 [details] [diff] [review]
Disable zoom for find in page

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1014113 
User impact if declined: page zooms to much when using find in page
Testing completed (on m-c, etc.): tested locally, just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, disables new behavior that was added in bug 1014113 
String or IDL/UUID changes made by this patch: none

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This broke robocop-1: https://tbpl.mozilla.org/php/getParsedLog.php?id=40233929&tree=Fx-Team

Backed out in https://hg.mozilla.org/integration/fx-team/rev/21c3f77e224b

Comment 5

[:Margaret Leibovic]

(In reply to Wes Kocher (:KWierso) from comment #4)
> This broke robocop-1:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=40233929&tree=Fx-Team
> 
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/21c3f77e224b

Grr, I looked at the test changes from bug 958111 before landing this, but I only saw the part we disabled, not this change:
https://hg.mozilla.org/mozilla-central/diff/91176f7dd8e2/mobile/android/base/tests/testFindInPage.java

I will update and push to try. Sorry for causing a backout!

Comment 6

[:Margaret Leibovic]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=a6575bc6d423

Comment 7

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #6)
> Try push:
> https://tbpl.mozilla.org/?tree=Try&rev=a6575bc6d423

Boo, this is still failing, but only on 2.2. This test seems pretty flakey (it's caused problems in the past and it relies on multiple sleep calls), so I think we should just disable it until we can make it better.

I was actually able to reproduce this locally on my N4, and it looks like the test page actually renders with smaller than it used to, so when the page pans on a match, it doesn't actually pan far enough to move to a different spot.

I feel like there has to be a better way to test if the page panned than checking the color of pixels on screen. Maybe this test was written before we had more JS testing capabilities, but it seems like this check would be better done in JS.

Comment 8

[:Margaret Leibovic]

Attachment: Revert robocop changes and disable testFindInPage

This patch reverts the testFindInPage changes made in bug 958111, but it also disables the test. I filed bug 1015395 about improving this test so that we can re-enable it. Maybe Robin would want to do that as part of his work on bug 1014113.

Comment 9

[:Margaret Leibovic]

Attachment: Revert robocop changes and disable testFindInPage

Oops didn't qrefresh.

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 8427949 [details] [diff] [review]
Revert robocop changes and disable testFindInPage

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

::: mobile/android/base/tests/robocop.ini
@@ +40,2 @@
>  # disabled on Android 2.3; bug 975155
>  skip-if = android_version == "10"

Notice the skip-if -- it previously applied to testFindInPage; now it skips testFilterOpenTab!

Let's remove this skip-if and the "disabled on Android 2.3" comment, and leave a comment "see bug 1015395, bug 975155".

Comment 11

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #10)
> Comment on attachment 8427949 [details] [diff] [review]
> Revert robocop changes and disable testFindInPage
> 
> Review of attachment 8427949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/robocop.ini
> @@ +40,2 @@
> >  # disabled on Android 2.3; bug 975155
> >  skip-if = android_version == "10"
> 
> Notice the skip-if -- it previously applied to testFindInPage; now it skips
> testFilterOpenTab!
> 
> Let's remove this skip-if and the "disabled on Android 2.3" comment, and
> leave a comment "see bug 1015395, bug 975155".

Oh, I assumed the skip-if applied to the line that followed, not the line before. That's confusing!

Comment 12

[:Margaret Leibovic]

Attachment: Revert robocop changes and disable testFindInPage (v2)

Pushed to try for good measure: https://tbpl.mozilla.org/?tree=Try&rev=2f5a7ce4500d

Comment 13

[Geoff Brown [:gbrown]]

Comment on attachment 8427967 [details] [diff] [review]
Revert robocop changes and disable testFindInPage (v2)

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

Thanks!

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by: will swing back around to get these uplifts approved when it's landed (and stuck) on central but before next week's mobile beta.

Comment 15

[:Margaret Leibovic]

Green on try, so landing on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/97c6e373c441
https://hg.mozilla.org/integration/fx-team/rev/9d54afd94ff3

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97c6e373c441

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/372bd0f41776
https://hg.mozilla.org/releases/mozilla-aurora/rev/2799a3739894

https://hg.mozilla.org/releases/mozilla-beta/rev/d2394f3e3825
https://hg.mozilla.org/releases/mozilla-beta/rev/cca418979066

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d2394f3e3825
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/cca418979066

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

Please verify the disabling of this on nightly, aurora and beta.

Comment 20

[Catalin Suciu [:csuciu]]

Verified that 'zoom for find in page' is disabled on Nightly 32.0a1 (2014-05-28), Aurora 31.0a2 (2014-05-28) and Firefox 30 Beta 8