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)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: bugzillamozilla, Assigned: steffen.wilberg)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Product: Documentation → Firefox
Assignee: documentation → jwalden+fxhelp
QA Contact: daniel.bugmail → firefox.help
Attached patch patch (obsolete) — Splinter Review
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 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;
}
Whoops, left a /g off that match :-[
Attached patch patch v1.1 (obsolete) — Splinter Review
Thanks, Neil! How about this?
Attachment #171861 - Attachment is obsolete: true
Attachment #171861 - Flags: review?(mconnor)
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.
> 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.
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?
Attached patch patch v1.2Splinter Review
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 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.
Attachment #171899 - Flags: review?(mconnor) → review?(jan)
Attachment #171899 - Flags: review?(jan) → review+
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
Verified with Gecko/20050213/Firefox/1.0+

Thanks for fixing this, Steffen!

Prog.
Status: RESOLVED → VERIFIED
Flags: review+
Product: Firefox → Toolkit
Target Milestone: Firefox1.1 → ---
Targetting bugs which were targetted to Firefox1.1 before the move to
mozilla1.8beta2.
Target Milestone: --- → mozilla1.8beta2
Blocks: 295904
Flags: in-testsuite?
Product: Toolkit → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: