414076 - [FIX]Find previous does not wrap (reports "Phrase not found")

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Elmar Ludwig]

Attachment: testcase (see comment #0)

The "Previous" button on the find toolbar sometimes stops at the first match on the page and does not wrap. Steps to reproduce:

1. Load the attached testcase
2. Open find toolbar and search for "elmar" (without the quotes)
3. Press "Next" a few times, observe that wrap around works
4. Press "Prev" a few times, find stops at first match and reports
   "Phrase not found"

Searching for "rss" in the testcase wraps correctly in both directions.

Note that the structure of the HTML in the testcase is slightly broken: there is a DIV before the HTML element. This is the same as in the original page from which this testcase was derived. Fixing the HTML structure makes find previous work correctly.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012404 Minefield/3.0b3pre ID:2008012404

Comment 1

[Elmar Ludwig]

This works fine in Firefox 2.0.0.11, so this is a regression.

I haven't tried to identify the regression range yet, but a quick test shows that the problem was already present in Gecko/20070518 Minefield/3.0a5pre.

Comment 2

[Elmar Ludwig]

Regression range:

2006-10-20-04 (works)
2006-10-21-04 (fails)

Checkins between 2006-10-20 04:00 and 2006-10-21 04:59:

http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2006-10-20+04%3A00%3A00&maxdate=2006-10-21+04%3A59%3A59

This points to bug 357445 as the only likely candidate.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Possibly related to bug 336279 and/or bug 379280.

Comment 4

[Mike Schroepfer]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022204 Minefield/3.0b4pre ID:2008022204

WFM on above nightly.  Can you test again?

Comment 5

[Elmar Ludwig]

(In reply to comment #4)
> WFM on above nightly.  Can you test again?

It still does not work for me in the attached testcase here with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022204 Minefield/3.0b4pre ID:2008022204.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

The issue was that the change made in bug 357445 to the ContentIsInTraversalRange call in nsContentIterator::PositionAt was wrong: it used parent nodes that had nothing to do with the offsets that were being passed in.  To fix that, it was easiest to change ContentToParentOffset to return nsIContent, and simplify it drastically in the process.  That covers the PositionAt changes.

That left precious few callers of GetNumChildren, so I nixed the few remaining ones, and in the process got rid of the GetChildAt callers too.  That's the Init() changes.  If you prefer, I could leave those out while still fixing this bug, but they're quite safe and straightforward, and make the code clearer, imo. Not to mention the cruft-method removal...

I'd love it if someone can come up with a mochitest for this.  Should be pretty doable, since the find apis are scriptable (after all, the findbar uses them!).  I just don't know them off the top of my head, so it would take me a while to write the test.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and that patch fixes bug 379280 too.  Makes sense given the debugging Alex did there.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 309592 [details] [diff] [review]
Fix

Yay! Thanks!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed.  I'd still love a hand on those tests.  Otherwise I'll do them myself sometime in May or June...

Comment 10

[Ronny Perinke]

Comment on attachment 309592 [details] [diff] [review]
Fix


-      numChildren = GetNumChildren(firstNode);
-
-      if (numChildren)
+      if (firstNode->GetChildCount())
         firstOffset = numChildren;
       else
-        ContentToParentOffset(mFirst, getter_AddRefs(firstNode), &firstOffset);
+        firstNode = ContentToParentOffset(mFirst, &firstOffset);

numChildren is not initialized

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 310608 [details] [diff] [review]
follow-up to fix use of uninitialized variable

Ah, indeed.  Thanks for catching this!

Comment 12

[Boris Zbarsky [:bzbarsky]]

I checked in the followup patch.  Thank you again for fixing that.

Comment 13

[Ronny Perinke]

(In reply to comment #12)
> I checked in the followup patch.  Thank you again for fixing that.
> 

You're welcome!