126232 - Issues with New Find

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[kinmoz]

Here are some issues I'm seeing with my 02/17/02 Win32 debug build which has the 
latest "new find" code.

To enable the "new find" code, add the following pref to your prefs.js file and 
restart the browser:

    user_pref("browser.new_find", true)

Below are some of the problems I'm seeing. I'll be attatching a test case I put 
together that shows these problems.


Problems searching Forwards:

1. We match across block boundaries (including <p> and table tags).
   Looking for "mozilla" matches: <p>moz</p><p>illa</p>

2. Can't get past textareas, I get a "The text you entered was not found."
   message.


Problems searching backwards:

1. If caret/selection is somewhere the middle of the word "mozilla" and you
   search for "mozilla" it will not hilite that instance, it gets skipped.

2. If you find an instance of a pattern in the search text, and the beginning
   of the text is in an inline, you can't get past it.

3. Can't get out of textareas, selects, lists.

Comment 1

[kinmoz]

Attachment: Single Word Test Case

Load this file and search forwards/backwards for "mozilla".

Note I forgot to mention that the "searching across blocks" problem also
happens when searching backwards.

Comment 2

[Akkana Peck]

Attachment: Fix for the worst two problems

This patch fixes what Kin sees as the worst two problems: improperly matching
across block boundaries (it was correctly detecting the different block parents
but it wasn't ending the match by setting pindex back to zero), and getting
lost in form controls (I skip select, textarea, and input; anything else I
should skip?)

Please don't be alarmed by the line count of this patch -- most of it is just
introducing the new atoms for the three new skipped nodes.  The meat of the
patch is the one place (two new lines) where the new nodes are added to the
list of skipped content, and the two new lines lines when a change in block
parent is detected.

I will work on the other two reverse-find problems next, since those also seem
serious and need fixing.

Comment 3

[Akkana Peck]

Attachment: Fix all but the minor problem

Here's a patch that fixes everything except the first problem Kin listed under
reverse searches, that of not finding a string that spans the selection when we
start out.  It does find it when it wraps around.  Fixing it turns out to be
tricky.  I do agree that finding the string even when it spans the selection is
probably the best behavior, but I would argue that it's not 100% obvious and
that this problem is a minor one that we could live with for a short time and
shouldn't block turning on the feature.

For ease of review: the important differences between this patch and the last
one are that the code on line 776 of the old file, which decremented offset
relative to mIterOffset in the reverse caseso that offset would represent the
index into the string, has been moved; in the new patch, offset always
represents a range offset (pointing between two characters) and the -1 doesn't
happen until we are actually assigning to findex (which is always a string
index).  This simplifies understanding the code.  Then the new clause starting
at line 819, which skips the node if the offset is 0, is what actually fixes
the bug.  The rest of the changes are comments or debugging.

Comment 4

[kinmoz]

Comment on attachment 70143 [details] [diff] [review]
Fix all but the minor problem

sr=[e-mail]

Comment 5

[Akkana Peck]

Fixes for everything but 1r are in.  Leaving bug open for that issue.

Comment 6

[Charles Manske]

Comment on attachment 70143 [details] [diff] [review]
Fix all but the minor problem

r=cmanske

Comment 7

[Akkana Peck]

Bumping the remaining issue off to 1.0.

Comment 8

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 9

[Aleksander Adamowski]

Is the new find code on by default in some later builds? Or does it still
require that pref?

Comment 10

[Akkana Peck]

It's on by default -- the pref and the old find code have been removed.

Comment 11

[Aleksander Adamowski]

Might bug 159418 be related?

Comment 12

[Akkana Peck]

> Might bug 159418 be related?

No, enabling/disabling of menu items is done completely outside of the find code.

Comment 13

[Akkana Peck]

Oh, I should have tested this a long time ago -- turns out this has been fixed
by one of the other find fixes which have gone in since the bug was filed.

Comment 14

[sairuh (rarely reading bugmail)]

rs vrfy --new issues are/should be filed separately.