Closed Bug 584630 Opened 14 years ago Closed 14 years ago

Find / FAYT is not focusing on correct tab, so returns no or invalid results

Categories

(SeaMonkey :: Find In Page, defect)

defect
Not set
major

Tracking

(blocking-seamonkey2.1 a3+)

RESOLVED FIXED
seamonkey2.1a3
Tracking Status
blocking-seamonkey2.1 --- a3+

People

(Reporter: therubex, Assigned: mcsmurf)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 97023
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
blocking-seamonkey2.1: --- → ?
blocking-seamonkey2.1: ? → a3+
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Is this bug reporting a regression?
(otherwise, it would look like an issue SM 1.5a already had...)
Serge, it's surely possible that the findbar landing re-uncovered an issue that's been lingering in tabbrowser all the time.
Attached patch PatchSplinter Review
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
Attachment #465777 - Flags: superreview?(neil)
Attachment #465777 - Flags: review?(neil)
> 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 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.)
> _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 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.
Attachment #465777 - Flags: superreview?(neil)
Attachment #465777 - Flags: review?(neil)
Attachment #465777 - Flags: review+
(In reply to comment #9)
> Comment on attachment 465777 [details] [diff] [review]
> Patch

p.s. a+=me for landing.
http://hg.mozilla.org/comm-central/rev/e05dc1486d50
Assignee: nobody → bugzilla
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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...
Flags: in-testsuite?
Flags: blocking-seamonkey2.0.7?
Target Milestone: --- → seamonkey2.1a3
Yes, can we have an automated test for this, please?
2.0.x doesn't use _fastFind.
Flags: blocking-seamonkey2.0.7? → blocking-seamonkey2.0.7-
Attached patch Idea for a test (obsolete) — Splinter Review
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.
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. :)
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.
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. :)
Attached patch Another idea for a test (obsolete) — Splinter Review
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.
*this does _not_ seem
Attached patch working testSplinter Review
This test does actually work - and fails without the patch from this bug.
Attachment #466092 - Attachment is obsolete: true
Attachment #466450 - Attachment is obsolete: true
Attachment #467163 - Flags: review?(bugzilla)
Attachment #467163 - Flags: review?(bugzilla) → review+
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.