Closed Bug 1014708 Opened 5 years ago Closed 5 years ago

Disable zoom for find in page

Categories

(Firefox for Android :: General, defect)

30 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox29 --- unaffected
firefox30 + verified
firefox31 + verified
firefox32 + verified
fennec 30+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
Attachment #8427182 - Flags: review?(mark.finkle)
Attachment #8427182 - Flags: review?(mark.finkle) → review+
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
Attachment #8427182 - Flags: approval-mozilla-beta?
Attachment #8427182 - Flags: approval-mozilla-aurora?
(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!
Flags: needinfo?(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.
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.
Attachment #8427945 - Flags: review?(gbrown)
Oops didn't qrefresh.
Attachment #8427945 - Attachment is obsolete: true
Attachment #8427945 - Flags: review?(gbrown)
Attachment #8427949 - Flags: review?(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".
Attachment #8427949 - Flags: review?(gbrown) → review-
(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!
Pushed to try for good measure: https://tbpl.mozilla.org/?tree=Try&rev=2f5a7ce4500d
Attachment #8427949 - Attachment is obsolete: true
Attachment #8427967 - Flags: review?(gbrown)
Comment on attachment 8427967 [details] [diff] [review]
Revert robocop changes and disable testFindInPage (v2)

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

Thanks!
Attachment #8427967 - Flags: review?(gbrown) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/97c6e373c441
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Attachment #8427182 - Flags: approval-mozilla-beta?
Attachment #8427182 - Flags: approval-mozilla-beta+
Attachment #8427182 - Flags: approval-mozilla-aurora?
Attachment #8427182 - Flags: approval-mozilla-aurora+
Please verify the disabling of this on nightly, aurora and beta.
Flags: needinfo?(fennec)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(fennec)
You need to log in before you can comment on or make changes to this bug.