241053 - find in textarea misses some matches

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[rbs]

This is due to a glitch in nsFindContentIterator::Reset().
If you have been following bug 58305, you are probably aware of my wrestling
with the edge cases of that function. I will attach a patch to correct the problem.

Comment 1

[rbs]

Attachment: testcase - find for 'x' misses the match in the textarea

Comment 2

[rbs]

Attachment: fix

Some clean-ups:
- remove some further dead code that I discovered later in nsWebBrowserFind.cpp

- don't bother re-creating mOuterIterator when not necessary in Init()
- move the gimmicks of mOuterIterator->First() [or mOuterIterator->Last] just
after |SetupInnerIterator| inside |MaybeSetupInnerIterator| and |Reset|. This
way, those gimmicks don't have to be spread at caller sites.

The actual fix to this bug (in |Reset|): 
- the direct call to |SetupInnerIterator| in Reset() was based on an if-clause
that misses the case where the boundary point _is_ a text control. The clause
was only capturing the case where the point is inside the text control. The fix
 is to continue to try to create the inner-iterator if it wasn't created
already.

Comment 3

[rbs]

Comment on attachment 146570 [details] [diff] [review]
fix

Should drivers approve bug 58305 for 1.7f, this fix up will be necessary as
well.

sicking, do you mind reviewing this? akkana did the earlier review, but she is
on vacation at present.

jst, this is a small continuation of the earlier bug.

Thanks.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 146570 [details] [diff] [review]
fix

sr=jst

Comment 5

[rbs]

For my sanity sake, I am also going to stabilize PositionAt() as indicated
below. I have discovered that if you do PositionAt(PARIS), the iterator code
will put its current node there before returning failure. And so you can't do
much else with it, unless you stash the current node away first.

nsresult
nsFindContentIterator::PositionAt(nsIContent* aCurNode)
{
  nsIContent* oldNode = mOuterIterator->GetCurrentNode();
  nsresult rv = mOuterIterator->PositionAt(aCurNode);
  if (NS_SUCCEEDED(rv)) {
    MaybeSetupInnerIterator();
  }
  else {
    mOuterIterator->PositionAt(oldNode);
    if (mInnerIterator)
      rv = mInnerIterator->PositionAt(aCurNode);
  }
  return rv;
}

Comment 6

[rbs]

Comment on attachment 146570 [details] [diff] [review]
fix

Trying akkana since sicking is apparently unwilling or unable to attend to
this.

akkana, based on your earlier review, this is a simple follow-up that shouldn't
take much to look at.

Comment 7

[Akkana Peck]

Comment on attachment 146570 [details] [diff] [review]
fix

r=akkana

Comment 8

[rbs]

Comment on attachment 146570 [details] [diff] [review]
fix

checked in the trunk.

drivers, this is a follow-up to bug 58305 for which the approval is also
pending.

Comment 9

[rbs]

-> FIXED on the 1.8a trunk.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 146570 [details] [diff] [review]
fix

a=asa (on behalf of drivers) for checkin to 1.7

Comment 11

[rbs]

-> Now FIXED on the 1.7 branch.