Last Comment Bug 584630 - Find / FAYT is not focusing on correct tab, so returns no or invalid results
: Find / FAYT is not focusing on correct tab, so returns no or invalid results
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Find In Page (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: seamonkey2.1a3
Assigned To: Frank Wein [:mcsmurf]
:
Mentors:
: 583589 (view as bug list)
Depends on:
Blocks: 97023
  Show dependency treegraph
 
Reported: 2010-08-04 23:41 PDT by therube
Modified: 2010-08-24 08:49 PDT (History)
9 users (show)
kairo: in‑testsuite+
neil: blocking‑seamonkey2.0.7-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
a3+


Attachments
Patch (576 bytes, patch)
2010-08-13 12:22 PDT, Frank Wein [:mcsmurf]
bugspam.Callek: review+
Details | Diff | Splinter Review
Idea for a test (1.40 KB, patch)
2010-08-15 02:48 PDT, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
Another idea for a test (1.74 KB, patch)
2010-08-16 15:26 PDT, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
working test (1.91 KB, patch)
2010-08-18 14:35 PDT, Robert Kaiser
bugzilla: review+
Details | Diff | Splinter Review

Description therube 2010-08-04 23:41:50 PDT
User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b3pre) Gecko/20100804 SeaMonkey/2.1a3pre
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b3pre) Gecko/20100804 SeaMonkey/2.1a3pre

 
With the new findbar (find toolbar) Find / FAYT is not focusing on the proper page/tab & so returns no, invalid, or unexpected results.
 

Reproducible: Sometimes

Steps to Reproduce:
 
1. Load http://www.seamonkey-project.org/start/
2. FAYT 'download'
> so, / download
> F3 or Ctrl+G to find next instances
> correctly finds (2); 'Download & Releases', 'You've downloaded (or compiled)'

3. Load, in a tab, http://fileforum.betanews.com/browse/new?days=7
4. Return to the tab, http://www.seamonkey-project.org/start/
5. F3 or Ctrl+G
> no action is observed, nothing found

6. FAYT 'download'
> no action is observed, nothing found
 
Actual Results:  
 
Observe the status line.
Repeatedly hit F3.
You will see various http://fileforum.betanews.com/download/* URLs listed
FAYT is finding results from the http://fileforum.betanews.com/browse/new?days=7 webpage
 

Expected Results:  
 
FAYT should find results from the focused page.
 

 
Starting with above scenario, add another tab with the same SeaMonkey page.
So; SeaMonkey | Betanews | SeaMonkey
F3 on this second SeaMonkey page.
> again correctly finds (2) instances of 'download'

Close that page.
F3 to search for another instance, either page will do.

Find returns, "! Phrase not found".
It does this because it is (attempting to) search on the tab that was closed, that is no longer there.
 
At one point, I believe a keystroke, Enter as it was, must have been "transferred" too, as I got a prompt for a download?
 
> Zart: seems like new fayt doesnt hook to tab/windows switches
> Ratty: Ah focus problems

> Ratty: file followup bugs making it block the original SeaMonkey findbar bug

Unfortunately I don't know what the original bug was, & my searching isn't finding it.
Comment 1 Robert Kaiser 2010-08-05 05:46:12 PDT
Hah, that sounds as it could be at least related to the problem I've seen that I couldn't get Find/FAYT to work on some rather large text documents.
Interestingly, the "highlight" function of the find bar seems to still work in those cases - same in your scenario?
Comment 2 Philip Chee 2010-08-06 08:59:41 PDT
*** Bug 583589 has been marked as a duplicate of this bug. ***
Comment 3 Serge Gautherie (:sgautherie) 2010-08-11 07:41:39 PDT
Is this bug reporting a regression?
(otherwise, it would look like an issue SM 1.5a already had...)
Comment 4 Robert Kaiser 2010-08-11 08:37:27 PDT
Serge, it's surely possible that the findbar landing re-uncovered an issue that's been lingering in tabbrowser all the time.
Comment 5 Frank Wein [:mcsmurf] 2010-08-13 12:22:23 PDT
Created attachment 465777 [details] [diff] [review]
Patch

This seems to do the trick. Got the idea for this from http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/tabbrowser.xml#798
Comment 6 Philip Chee 2010-08-13 13:19:57 PDT
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/tabbrowser.xml#798

Wow. This dates back to the Aviary landing. Hg changeset imported from CVS...
Comment 7 neil@parkwaycc.co.uk 2010-08-13 13:35:35 PDT
Comment on attachment 465777 [details] [diff] [review]
Patch

>+            this._fastFind.setDocShell(this.mCurrentBrowser.docShell);
_fastFind might not be set? (It currently is because the find toolbar causes it to be set, but you never know, we might do it more lazily at some point.)
Comment 8 Philip Chee 2010-08-13 20:04:31 PDT
> _fastFind might not be set? (It currently is because the find toolbar causes it

Using the "fastFind" property (has a getter) instead of "_fastFind" should automatically set and return _fastFind.

> to be set, but you never know, we might do it more lazily at some point.)
As Firefox does ;)
Comment 9 Justin Wood (:Callek) 2010-08-13 22:21:24 PDT
Comment on attachment 465777 [details] [diff] [review]
Patch

Looks good, but as Neil suggests we should be sure to use |this.fastFind| instead of |_fastFind| here [for now].

Firefox at least initiates _fastFind also on addTab. So it can't hurt to be safe on this call.
Comment 10 Justin Wood (:Callek) 2010-08-13 22:21:48 PDT
(In reply to comment #9)
> Comment on attachment 465777 [details] [diff] [review]
> Patch

p.s. a+=me for landing.
Comment 11 Frank Wein [:mcsmurf] 2010-08-14 05:34:51 PDT
http://hg.mozilla.org/comm-central/rev/e05dc1486d50
Comment 12 Serge Gautherie (:sgautherie) 2010-08-14 08:06:28 PDT
I haven't checked myself, but, based on previous comments,
it looks like we may want to check and back-port this to SM 2.0.x.

A test from comment 0 steps would be nice to have...
Comment 13 Robert Kaiser 2010-08-14 10:17:32 PDT
Yes, can we have an automated test for this, please?
Comment 14 neil@parkwaycc.co.uk 2010-08-14 12:16:52 PDT
2.0.x doesn't use _fastFind.
Comment 15 Frank Wein [:mcsmurf] 2010-08-15 02:48:35 PDT
Created attachment 466092 [details] [diff] [review]
Idea for a test

This is a test, but it does not work yet. Not sure why, I guess the initialization of FAYT needs to be changed(?) as calling gBrowser.fastFind.find("random", false) once before the first is(....) function makes both test pass.
Comment 16 Robert Kaiser 2010-08-15 06:27:31 PDT
Does the test in the form where it passes (i.e. with that command added) fail when your patch isn't applied? That should be the most important criteria for the test, after all. :)
Comment 17 Frank Wein [:mcsmurf] 2010-08-15 06:59:17 PDT
Yes, then it fails. BTW: I tested with alert(gBrowser...) and now it seems the problem is timing-related. When I insert alert("foo") before the is(...) and click on the OK button when the alert box appears, both tests pass with the patch applied. Without the patch the second test fails.
Comment 18 Robert Kaiser 2010-08-15 07:05:10 PDT
The usual thing we do for such things is observing some event that gets issued when the initialization is done, in doubt adding an observer notification ourselves in that case so we can observe it in the test. Then, we can make the test do it's stuff when the observer is fired.
See e.g. http://mxr.mozilla.org/comm-central/source/suite/common/downloads/tests/browser/browser_nsISuiteDownloadManagerUI.js observing the "download-manager-ui-done" event we added in the download manager window just for that purpose. Notifying observers is cheap but helpful. :)
Comment 19 Frank Wein [:mcsmurf] 2010-08-16 15:26:12 PDT
Created attachment 466450 [details] [diff] [review]
Another idea for a test

This one does not work either, but I think it's better than the first idea ;-) listens to load event to start the find, but this does seem to be the right event to listen to.
Comment 20 Frank Wein [:mcsmurf] 2010-08-16 15:26:42 PDT
*this does _not_ seem
Comment 21 Robert Kaiser 2010-08-18 14:35:25 PDT
Created attachment 467163 [details] [diff] [review]
working test

This test does actually work - and fails without the patch from this bug.
Comment 22 Robert Kaiser 2010-08-24 08:49:16 PDT
Comment on attachment 467163 [details] [diff] [review]
working test

Pushed this test as http://hg.mozilla.org/comm-central/rev/59416744bb13

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