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)
SeaMonkey
Find In Page
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)
576 bytes,
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
1.91 KB,
patch
|
mcsmurf
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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?
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•14 years ago
|
blocking-seamonkey2.1: --- → ?
blocking-seamonkey2.1: ? → a3+
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment 3•14 years ago
|
||
Is this bug reporting a regression?
(otherwise, it would look like an issue SM 1.5a already had...)
Comment 4•14 years ago
|
||
Serge, it's surely possible that the findbar landing re-uncovered an issue that's been lingering in tabbrowser all the time.
Assignee | ||
Comment 5•14 years ago
|
||
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
Updated•14 years ago
|
Attachment #465777 -
Flags: superreview?(neil)
Attachment #465777 -
Flags: review?(neil)
Comment 6•14 years ago
|
||
> 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•14 years ago
|
||
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•14 years ago
|
||
> _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•14 years ago
|
||
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+
Comment 10•14 years ago
|
||
(In reply to comment #9)
> Comment on attachment 465777 [details] [diff] [review]
> Patch
p.s. a+=me for landing.
Assignee | ||
Comment 11•14 years ago
|
||
Assignee: nobody → bugzilla
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 12•14 years ago
|
||
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
Comment 13•14 years ago
|
||
Yes, can we have an automated test for this, please?
Comment 14•14 years ago
|
||
2.0.x doesn't use _fastFind.
Flags: blocking-seamonkey2.0.7? → blocking-seamonkey2.0.7-
Assignee | ||
Comment 15•14 years ago
|
||
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•14 years ago
|
||
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. :)
Assignee | ||
Comment 17•14 years ago
|
||
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•14 years ago
|
||
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. :)
Assignee | ||
Comment 19•14 years ago
|
||
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.
Assignee | ||
Comment 20•14 years ago
|
||
*this does _not_ seem
Comment 21•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #467163 -
Flags: review?(bugzilla) → review+
Comment 22•14 years ago
|
||
Comment on attachment 467163 [details] [diff] [review]
working test
Pushed this test as http://hg.mozilla.org/comm-central/rev/59416744bb13
Updated•14 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•