Closed
Bug 271299
Opened 20 years ago
Closed 19 years ago
Help Viewer search results should be updated with every keystroke (live search)
Categories
(SeaMonkey :: Help Viewer, enhancement)
SeaMonkey
Help Viewer
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.8beta2
People
(Reporter: bugzillamozilla, Assigned: steffen.wilberg)
References
Details
Attachments
(1 file, 2 obsolete files)
5.35 KB,
patch
|
Details | Diff | Splinter Review |
There's a good reason why live search is used by both Find-As-You-Type and the new Firefox Find toolbar - responsiveness. This method simply provides significantly better feedback, especially for users who are not sure about the correct search string. All they have to do is start typing and keep an eye on the results list. This functionality is nicely demonstrated in Windows .hlp files (not .chm ones). Just click the Find tab and start typing. Note that you may first need to create an index by pressing Next a few times. This part we don't want to replicate. Prog.
Reporter | ||
Updated•20 years ago
|
Product: Documentation → Firefox
Assignee | ||
Updated•20 years ago
|
Assignee: documentation → jwalden+fxhelp
QA Contact: daniel.bugmail → firefox.help
Assignee | ||
Comment 1•20 years ago
|
||
The textbox logic is taken from the searchbox in the Bookmarks Sidebar. The search takes place after the user entered something and either waited 500ms or hit enter. If he removes the search string, the results are emptied as well. Firefox also needs the patch from bug 258997 in order to be able to respond to single letter searches.
Assignee: jwalden+fxhelp → steffen.wilberg
Status: NEW → ASSIGNED
Attachment #171861 -
Flags: review?(mconnor)
Comment 2•20 years ago
|
||
Comment on attachment 171861 [details] [diff] [review] patch > <hbox align="center"> >+ <textbox id="findText" flex="1" You can take out the hbox and flex="1" >+ if (!findText.value) { >+ searchTree.builder.rebuild(); >+ return; >+ } >+ > // split search string into separate terms and compile into regexp's > RE = findText.value.split(/\s+/); > for (var i=0; i < RE.length; ++i) { > if (RE[i] == "") > continue; > > RE[i] = new RegExp(RE[i], "i"); > } When I looked at this code I spotted an existing bug. Try typing just spaces into the search, and check the console. Fortunately the new code can fairly easily deal with this, like so: RE = findText.value.match(/\S+/); if (!RE) { searchTree.builder.rebuild(); return; }
Comment 3•20 years ago
|
||
Whoops, left a /g off that match :-[
Assignee | ||
Comment 4•20 years ago
|
||
Thanks, Neil! How about this?
Attachment #171861 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #171861 -
Flags: review?(mconnor)
Comment 5•20 years ago
|
||
Comment on attachment 171864 [details] [diff] [review] patch v1.1 >+ // if the search string is empty or contains only whitespace, purge the results tree and return >+ RE = findText.value.match(/\S+/g); >+ if (!RE) { >+ searchTree.builder.rebuild(); >+ return; >+ } >+ >+ // compile the search string, which has already been split up above, into regexp's > for (var i=0; i < RE.length; ++i) { > if (RE[i] == "") > continue; I'm having this can of worms feeling, but I'd say this test is no longer necessary now.
Assignee | ||
Comment 6•20 years ago
|
||
> I'm having this can of worms feeling, but I'd say this test is no longer necessary now. Which test? This one? > >+ if (!RE) { > >+ searchTree.builder.rebuild(); > >+ return; > >+ } That's necessary to empty the results list if the search string is empty, and return early because we don't need to search. And if we don't catch an empty RE, we get an "RE has no properties" error in the following 'for' line.
Comment 7•20 years ago
|
||
When I said "I'd say this test is no longer necessary" I meant "it looks as if this existing test has been superceded by your new test".
I do like this idea, would it be possible to be done on the suite's help too?
Assignee | ||
Comment 9•20 years ago
|
||
Oh, *that* test! :) That's indeed redundant now. That was filtering spaces at the end of the search string. But now we're removing those with '.match(/\S+/g);'.
Attachment #171864 -
Attachment is obsolete: true
Attachment #171899 -
Flags: review?(mconnor)
Comment 10•19 years ago
|
||
Comment on attachment 171899 [details] [diff] [review] patch v1.2 Steffen, if you could pick another toolkit reviewer (note that despot has a separate list for /toolkit now, see owners.html) that'd be great, since I'm not going to get to this for at least a few weeks.
Assignee | ||
Updated•19 years ago
|
Attachment #171899 -
Flags: review?(mconnor) → review?(jan)
Updated•19 years ago
|
Attachment #171899 -
Flags: review?(jan) → review+
Assignee | ||
Comment 11•19 years ago
|
||
Checked in. Note that Firefox still needs the fix from bug 258997 in order to be able to respond to single letter searches. Re comment 8: It should be easy to port this to Seamonkey's Help Viewer. I don't have the resources (time, Seamonkey tree) to do that though.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1
Reporter | ||
Comment 12•19 years ago
|
||
Verified with Gecko/20050213/Firefox/1.0+ Thanks for fixing this, Steffen! Prog.
Status: RESOLVED → VERIFIED
Updated•19 years ago
|
Flags: review+
Product: Firefox → Toolkit
Target Milestone: Firefox1.1 → ---
Assignee | ||
Comment 13•19 years ago
|
||
Targetting bugs which were targetted to Firefox1.1 before the move to mozilla1.8beta2.
Target Milestone: --- → mozilla1.8beta2
Updated•18 years ago
|
Flags: in-testsuite?
Updated•8 years ago
|
Product: Toolkit → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•