Last Comment Bug 771997 - Find in Page doesn't work for more than one tab
: Find in Page doesn't work for more than one tab
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 16
Assigned To: :Margaret Leibovic
:
:
Mentors:
Depends on:
Blocks: find
  Show dependency treegraph
 
Reported: 2012-07-09 02:09 PDT by Cristian Nicolae (:xti)
Modified: 2016-07-29 14:26 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified


Attachments
patch (1.72 KB, patch)
2012-07-10 14:55 PDT, :Margaret Leibovic
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(cleanup) Use the browser's built-in fastFind property (2.69 KB, patch)
2012-07-10 14:57 PDT, :Margaret Leibovic
mark.finkle: review+
Details | Diff | Splinter Review

Description Cristian Nicolae (:xti) 2012-07-09 02:09:03 PDT
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] 2012-07-09 05:57:56 PDT
Other browsers handle this by making find-in-page the foreground UI and limiting it to one tab.
Comment 2 :Margaret Leibovic 2012-07-09 08:17:25 PDT
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) 2012-07-09 08:19:25 PDT
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 2012-07-10 14:55:05 PDT
Created attachment 640804 [details] [diff] [review]
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 2012-07-10 14:57:41 PDT
Created attachment 640807 [details] [diff] [review]
(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?) 2012-07-10 15:02:14 PDT
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 8 :Margaret Leibovic 2012-07-10 16:00:33 PDT
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 10 Cristian Nicolae (:xti) 2012-07-11 23:57:21 PDT
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

Note You need to log in before you can comment on or make changes to this bug.