771997 - Find in Page doesn't work for more than one tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Firefox 16.0a1 (2012-07-08)
Device: Asus EEE Transformer TF101
OS: Android 4.0.3

Steps to reproduce:
1. Open Fennec
2. Go to news.google.com 
3. Open a new tab and go to planet.mozilla.org
4. Go to Menu > Find in Page
5. Perform a search about "m" for example
6. Switch to the page opened at step 2
7. Tap on up/down arrow to find in page the search criteria from step 5

Expected result:
Find in Page still works after step 7.

Actual result:
No results are highlighted at step 7. Find in Page works only for the tab which was active when this feature was enabled.

Comment 1

[Aaron Train [:aaronmt]]

Other browsers handle this by making find-in-page the foreground UI and limiting it to one tab.

Comment 2

[:Margaret Leibovic]

Do users want to perform the same search across multiple tabs? And do they expect the find bar to reappear when they come back to the tab that it was open in before? If the answer to either of those is yes, we should probably just fix the issue as described instead of dismissing it when switching tabs.

Comment 3

[Cristian Nicolae (:xti)]

O Desktop Firefox, the Find in Page bar persists on each tab, so we could do something similar on Android side.

Comment 4

[:Margaret Leibovic]

Attachment: patch

This is similar to what we were talking about doing for text selection. This clears the active find when switching tabs, but it won't close the find bar. If you switch tabs and hit one of the arrows, it will start a new find in that new tab. This matches what desktop does, although it won't persist the find from the previous tab (desktop does that).

Comment 5

[:Margaret Leibovic]

Attachment: (cleanup) Use the browser's built-in fastFind property

We don't need to create our own find instance, since we get one for free with the browser (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#354). I also updated the variable name from _find to _fastFind for consistency with desktop code.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 640804 [details] [diff] [review]
patch


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   uninit: function() {
>     Services.obs.removeObserver(this, "FindInPage:Find", false);
>     Services.obs.removeObserver(this, "FindInPage:Prev", false);
>     Services.obs.removeObserver(this, "FindInPage:Next", false);
>     Services.obs.removeObserver(this, "FindInPage:Closed", false);
>+    Services.obs.removeObserver(this, "Tab:Selected", false);
>   },

Just some cleanup, but removeObserver does not take a third param. You can remove all of those.

"Tab:Selected" is called for adding and closing tabs too, so we should be covered in those cases too.

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/743769226802
https://hg.mozilla.org/integration/mozilla-inbound/rev/29b823147b88

Comment 8

[:Margaret Leibovic]

Comment on attachment 640804 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): issue with find in page, which is new in 15
User impact if declined: find in page gets into a broken state when switching tabs
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk, just cancels find when we switch tabs
String or UUID changes made by this patch: n/a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/743769226802
https://hg.mozilla.org/mozilla-central/rev/29b823147b88

Comment 10

[Cristian Nicolae (:xti)]

Find in Page works now on multi tabs. 
Closing bug as verified fixed on:

Firefox 16.0a1 (2012-07-11)
Device: Galaxy Nexus
OS: Android 4.0.4

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84dd961ac565