374873 - [FIX]Find returns to top immediately after first instance in form field

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[Neil P. Pena]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070321 Minefield/3.0a3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070321 Minefield/3.0a3pre

When doing a find on a page containing form fields, if the text the user is searching for is found inside one of the fields, when Enter or Next is pressed again, the search immediately begins from the top of the page, even though there are more instances later in the page.

Reproducible: Always

Steps to Reproduce:
1. Load the URL shown above
2. Search for the text "Firefox", which occurs multiple times in the article

Actual Results:  
The first two instances of "Firefox" are found--one in the header "Editing Mozilla Firefox" and the first instance in the first line of the input box, "{{redirect|Firefox}}".  Even though there are many more instances of the word "Firefox" later in that same input box and later in the page outside the input box, they are not found, and searching begins again from the top of the page, with the page header.

Expected Results:  
All remaining instances in the page should have been found before starting again from the top.

Comment 1

[Neil P. Pena]

Attachment: Testcase for Bug 374873

Comment 2

[Martijn Wargers (dead)]

This regressed between 2007-02-28 and 2007-03-02:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-28+04&maxdate=2007-03-02+09&cvsroot=%2Fcvsroot
There are a lot of check-ins in that area, but it seems to me that this is a regression from bug 372086, somehow.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Seems possible...  I have no idea how search goes about figuring out where to go next.  I'll try to look, but I have several dozen crash and security bugs that are higher-priority, so if someone who knows something about search (or is willing to learn) looks first, so much the better.

Comment 4

[Martijn Wargers (dead)]

I have checked with my debug build that the patch for bug 372086 indeed has caused this.

Maybe this gives an indication of what's going on?
http://lxr.mozilla.org/mozilla/source/embedding/components/find/src/nsFind.cpp#95
Especially this line:
"
This means that we can be given an initial search
// range that stretches across the anonymous DOM and the normal DOM.
"

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, that's exactly the problem.  This code is using ranges to keep track of part of the flattened tree (including anonymous content), and when we changed things so you can't do that anymore, it didn't deal very well...

Patch coming up that seems to fix things for me.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And this is a core Find issue.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

There are a few things going on here.

First of all, when we initialize mIterator we may want it to iterate between endpoints one of which is anonymous and one of which is not.  That's not expressible with a range, so just keep track of the endpoint nodes and offsets directly.  This is the bulk of the patch -- it involves changing InitIterator, adding another Init() method to the find iterator, changing the members, and replacing all uses of mRange with the new members

The other issue I ran into was that this code actually depended on a bug in the text node implementation of IsNativeAnonymous().... a bug that we fixed sometime in the 1.9 cycle.  Before the fix, a text child of a native anonymous element claimed to be native anonymous; now it (correctly) only claims it if it's native anonymous with respect to its parent.  Which means that to check whether a node is really "native anonymous" with respect to the toplevel document you really need to walk the bindingParent chain.  That's the second change in this patch.  I went for a minimally-invasive change, because I wasn't sure how this code wanted to handle XBL...

With those two changes, the testcase and site both work for me.

Comment 9

[rbs]

The following bit of the patch doesn't look correct. What you want to emulate is
   range = mRange; // to clone/copy everything
   range.some_fields = new_values // while other cloned values are preserved.
As it stands with the patch, some fields are left uninitialized.

(to check this code-path, IIRC, try ffwd and fbwd in a document with: a textarea as first element, some text/other things, and a textarea as last element).

     // make sure to place the outer-iterator outside
     // the text control so that we don't go there again.
     nsresult res;
-    mRange->CloneRange(getter_AddRefs(range));
+    // XXXbz what about out-of-memory here?
+    range = do_CreateInstance(kRangeCID);
     if (!mFindBackward) { // find forward
       // cut the outer-iterator after the current node
-      res = range->SetStartAfter(outerNode);
+      res = range->SetEnd(mEndNode, mEndOffset);
+      res |= range->SetStartAfter(outerNode);
     }
     else { // find backward
       // cut the outer-iterator before the current node
-      res = range->SetEndBefore(outerNode);
+      res = range->SetStart(mStartNode, mStartOffset);
+      res |= range->SetEndBefore(outerNode);
     }

Comment 10

[rbs]

While re-reading the code for the review, I saw that you can take a nice bite at the OOM issue by swapping things around, specifically:

377   nsCOMPtr<nsIDOMElement> rootElement;
378   editor->GetRootElement(getter_AddRefs(rootElement));
379   nsCOMPtr<nsIContent> rootContent(do_QueryInterface(rootElement));
380 
381   // now create the inner-iterator
382   mInnerIterator = do_CreateInstance(kCPreContentIteratorCID);
383 
384   if (mInnerIterator) {
385     nsCOMPtr<nsIDOMNode> node(do_QueryInterface(rootContent));
386     nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID));
387     range->SelectNodeContents(node);

TO:

377   nsCOMPtr<nsIDOMElement> rootElement;
378   editor->GetRootElement(getter_AddRefs(rootElement));
379   nsCOMPtr<nsIContent> rootContent(do_QueryInterface(rootElement));
      nsCOMPtr<nsIDOMNode> node(do_QueryInterface(rootContent));

      nsCOMPtr<nsIDOMRange> range(do_CreateInstance(kRangeCID));
      if (!range)
        return;

      range->SelectNodeContents(node);

And continue with the create mInnerIterator as is, and re-use range to init
mOuterIterator without further worries about the OOM issue.

[You could swap far back, BTW, but it boils down to creating the iterator only if you have a range, rather than the other way around as it is presently done.]

Comment 11

[rbs]

Comment on attachment 261644 [details] [diff] [review]
Proposed fix

r+sr=rbs

Eh, the patch does what I mentioned... What was I reading before?! (Nevermind, it was 3am here.) Only take care of the OOM and it is just fine.

Comment 12

[Boris Zbarsky [:bzbarsky]]

We can't re-use |range| for both iterators -- iterators don't copy the range they're inited with.  So we need two different range objects.  That said, I'll just go ahead and use two different variables for that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: With commens addressed

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in.