Closed Bug 58305 Opened 24 years ago Closed 20 years ago

Find in page ignores text fields - does not search form textarea

Categories

(SeaMonkey :: UI Design, enhancement, P5)

Product:
SeaMonkey
Component:
UI Design
Type:
enhancement

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.7final

People

(Reporter: selmer, Assigned: rbs)

References

Details

(Keywords: helpwanted, topembed-, verified1.7, Whiteboard: parity-ie | For a workaround, see comment #52)

Attachments

(2 files, 6 obsolete files)

Basic (buggy so beware!) firefox extension for finding in textarea's
19.48 KB, application/x-xpinstall
Details
patch - updated per review comments
30.46 KB, patch
akkzilla
: review+
jst
: superreview+
asa
: approval1.7+
Details | Diff | Splinter Review 
10/26 16 MN6

On any bugzilla page, use ctrl-F to search for any string that's only in a text
input field.  You will not find it.
Noticed something similar on linux for a while now.
It seems one has to check the "Search Backwards" to find words.
In order to find anything i first do a default search - search beeps to me -
then i Search Backwards - and the word is found.
Find in page is supposedly handled by editor. Sending to beppe.
Assignee: ben → beppe
simon, would this be yours?
-> kin. prolly a dup.
Assignee: beppe → kin
This bug does not appear to be a duplicate.  (At least I can't find a dup.)

This is probably related to bug 51550 and bug 10470 in which the find finds
stuff that it shouldn't.  My theory was that it was finding stuff from the html
file whether or not it was visible on the screen.  That might explain why text
areas are ignored, as the text is usually entered after the page is rendered.
However, testing with some pre-filled-in text areas, find still ignores them.

Also, if you hit ctrl-f in a text area it moves the cursor rather than bringing
up a find dialog.  
that is consistent with 4.x, but is a nice enhancement request, setting to 
future
Severity: normal → enhancement
Priority: P3 → P5
Target Milestone: --- → Future
This is an important feature for web applications that use large TEXTAREAs, like 
when viewing NOTES at Yahoo! Calendar.

IE5 has this feature.
Kinda of a dup of bug 76374.

*** Bug 84650 has been marked as a duplicate of this bug. ***
Blocks: 106961
*** Bug 104688 has been marked as a duplicate of this bug. ***
Whiteboard: parity-ie
mass moving open bugs pertaining to find in page/frame to pmac@netscape.com as
qa contact.

to find all bugspam pertaining to this, set your search string to
"AppleSpongeCakeWithCaramelFrosting".
QA Contact: sairuh → pmac
remove self
*** Bug 135095 has been marked as a duplicate of this bug. ***
*** Bug 124989 has been marked as a duplicate of this bug. ***
OS: Windows 98 → All
See also bug 120568, "Find/Replace should be available for text fields in
Navigator".
*** Bug 179858 has been marked as a duplicate of this bug. ***
Has any progress been made or work been done to address this bug?  It's a
definite problem for me, as an internal web application at our company uses
large text fields extensively.  I don't see much other than closing of
duplicates done since 2001.
I'm afraid that I've had to go to IE for this type of application. And since I 
use wikis a fair bit, I've not used mozilla much for a while. I suppose I'm 
just not in the relevant market segment.

Was there a second birthday party for the bug?
QA Contact: pmac → sairuh
Selmer, is this really a P5 Future Enhancement? Sure, it was in October 2000,
but it's 2003!
Also - Should have keywords helpwanted, nsCatFood, nsbeta1 IMO.
Last two comments concur...
Matthew, I don't get to determine the priority or milestone.  It is definitely
an enhancement request.  The owner (kin) should put helpwanted if he wants help.
 I have added nsCatFood though.  Besides voting for this bug, you might look for
other duplicates and make sure they all get duped against this bug.  At some
point, a large number of dups will bring attention as well (there are currently
5 dups.)
Keywords: nsCatFood
topembed nominating
Keywords: topembed
Since we don't have a request for this (yet) from an embeddor or from the
composition point of view I'm going to topembed- this.
Keywords: topembed → topembed-
*** Bug 192645 has been marked as a duplicate of this bug. ***
*** Bug 194129 has been marked as a duplicate of this bug. ***
*** Bug 199894 has been marked as a duplicate of this bug. ***
*** Bug 200495 has been marked as a duplicate of this bug. ***
*** Bug 201179 has been marked as a duplicate of this bug. ***
*** Bug 201206 has been marked as a duplicate of this bug. ***
Teaking summary to help avoid dupes.
Summary: Find in page ignores text fields → Find in page ignores text fields - does not search form textarea
Some hints:
First, textareas are intentionally skipped.  I can't remember why that was,
though I remember we discussed it and weren't sure what the right answer was. 
So the first step in fixing this is to stop skipping them intentionally.  See
the attached patch.

However, applying this patch doesn't actually find matches in textareas.  If
you turn on DEBUG_FIND, you see that it finds the match, but then it fails this
test:
	  if (startParent && endParent && 
	      IsVisibleNode(startParent) && IsVisibleNode(endParent))
which according to the comments means "invisible or bad range".

It turns out that IsVisibleNode is failing because this is failing:
  content->GetDocument(*getter_AddRefs(doc));
(content is the nsIContent that comes from a QI of the #text node inside the
textarea).

Kin -- what's the right test here?
Comment on attachment 120858 [details] [diff] [review]
First step: don't intentionally skip them

Whoops.  I talked to Kin about this, and it turns out that the #text inside a
textarea is only for the initial text; it doesn't change when the textarea is
edited.  So this patch doesn't help -- it's searching the wrong string.

It turns out that there are two ways to do this:

(1) Introduce a new interface that lets you go through the textarea's editor
and get to the actual dom content, then use the normal find code on it.  This
is the preferred solution: it extends to work with midas, and it works with the
existing find APIs.

(2) Get textarea.value as a string, write separate search code to search
through that, then somehow select the relevant part of the textarea using this
interface:
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/html/nsIDOMNSHTMLInputEl
ement.idl#53
Note that this doesn't translate to a range, so it can't work with the existing
find interfaces, but it might work as a hackaround for people who have content
management pages that use textareas.
Attachment #120858 - Attachment is obsolete: true
Blocks: 210168
No longer blocks: 210168
*** Bug 210168 has been marked as a duplicate of this bug. ***
Welp, I'd love to see this one fixed sometime.   It's something that bothers me
in quite a few applications and Open since 2000 -- it's getting old.  Makes me
curious how many others are like this one.
any editor hacking still going on?
chofmann: yes; but unfortunately lower priority than finding another job position
          these days.
This is my number one request. Especially on the Mac this is important, because
the only browser that can do this is IE. There are two problems with IE, though:
(1) it is REALLY REALLY slow, and (2) MS stopped developing it and announced
that there will be no new versions of it. In other words: it is no more.
Why is this bug labelled as an "enhancement"?  The software doesn't do what it
says it will do.  What else could "Find in This Page" mean (see the Edit menu)
but that the command will find the text if it is on the current page?

Also, since we're coming up on the third anniversary of the filing of this bug
(and I doubt it was the first to be filed for this issue, given the early
comments along the lines of "Isn't this a dupe?"), wouldn't it be nice if we
could have the poor thing emerge from its NEW status?  (You know it's a bad sign
when a bug's been around so long that the reporter is marked as "gone"!)

The failure of the software to perform this basic function properly is
crippling.  Any chance the priority could be lifted out of the P5 basement?
Indeed, it's a bug needing fixing, not a desirable enhancement.  Not comfortable
in my new boots to change the P.  Comment #31 suggests it may not be an easy fix.
Severity: enhancement → major
Keywords: helpwanted
It's an enhancement because the mozilla code never did this
now please leave the field alone. the nextg change should be someone
posting a patch or volunteering to fix this bug. if you change this
bug then i will take your action as volunteering to fix this bug.
Severity: major → enhancement
> It's an enhancement because the mozilla code never did this ...

By this line of "reasoning" none of the security flaws which have
been in IE from the start are bugs. :->}

In case you hadn't noticed, the command does *not* say "Find in 
selected portions of this page."  This bug is not about "parity 
with IE" but about the failure of the software to do what it says
it will do.

Have a nice time celebrating the bug's third anniversary!

Well said, couldn't agree more.

> By this line of "reasoning" none of the security flaws which have
> been in IE from the start are bugs. :->}
>
> In case you hadn't noticed, the command does *not* say "Find in 
> selected portions of this page."  This bug is not about "parity 
> with IE" but about the failure of the software to do what it says
> it will do.
> 
> Have a nice time celebrating the bug's third anniversary!
*** Bug 222643 has been marked as a duplicate of this bug. ***
>In case you hadn't noticed, the command does *not* say "Find in 
>selected portions of this page."  This bug is not about "parity 
>with IE" but about the failure of the software to do what it says
>it will do.

I faced problem when using Wikis. So this is a real disadvantage of the 
Firebird browser, you cannot search in edit field, for instance long wiki texts.
Blocks: 164421
The problem is not only for wikis, but also for various other scripts allowing
users to edit files over the web.

This includes osCommerce where you can customize templates (this is how I came
on this bug)

It also includes just about any web-based file manager, such as the ones in
geocities, and the like, and also the ones provided as part of a control panel
on many shared hosting accounts. This means people editing any kind of HTML
files over the web will have a much harder time using Mozilla based browsers
than IE.

I wish I had the knowledge to fix this myself, but I don't, and neither, I
suspect, do many people whom this bug affects. This doesn't mean that it's an
enchancement that should be left for the future. It is a flaw that cripples
usability in a very broad scope of tasks that are common to the web. Whatever
its marked severity and target milestone, this bug needs to be fixed, because it
is one of those little annoyances that tends to drive people away.
*** Bug 228392 has been marked as a duplicate of this bug. ***
*** Bug 232671 has been marked as a duplicate of this bug. ***
My duplicate bug 222643 was marked as resolved. My focus was a little bit 
different. I DON'T want a ordinary full text search to process subwindows as 
well. I want an search option for edit field in the context menu, when the 
focus is on the edit field.

You need it for wiki edit fields.
*** Bug 233841 has been marked as a duplicate of this bug. ***
Blocks: 189039
*** Bug 202074 has been marked as a duplicate of this bug. ***
*** Bug 235598 has been marked as a duplicate of this bug. ***
Any news on this annoying bug, especially in connection with sites such as
Wikipedia where searching within textareas is very important?
Well, it doesn't solve this bug, but maybe the searchtextarea bookmarklet I had
made some time ago is of any use to you.
http://home.hccnet.nl/m.wargers/bookmarklets/searchtextarea.htm
Hardware: PC → All
Whiteboard: parity-ie → parity-ie | For a workaround, see comment #52
(In reply to comment #52)

Thank you Martijn!!!  Your javascript search works flawlessly within TWiki.

If you weren't a dude, I'd propose... I'm so happy.  But as you are, I'll just
offer you my first born in gratitude.  As much as I love Mozilla browsers, I've
been so frustrated by this particular issue for years now.

Maybe we all should pool our resources together and offer the first developer to
fix this problem properly a round trip to Nevada's Bunny Ranch...


This item goes into the "ridiculous" category.

That is, it's ridiculous that the comments go back to the year 2000 yet nothing
has been done about it.

Not only does this make Mozilla look bad, it also makes Wikis look bad. 

P5 priority?  "Enhancement"?  Nay, friends -- this is a critical bug.
Flags: blocking1.7b?
(In reply to comment #54)
> This item goes into the "ridiculous" category.
> 
> That is, it's ridiculous that the comments go back to the year 2000 yet nothing
> has been done about it.

Stop ranting and start coding please; the most ridiculous here is your attitude.
If this bug is 3 years old, it either means that

1. we have MUCH more important bugs on our radar
2. we still don't know how to implement it
3. the bug owner is no longer active on Mozilla 

> Not only does this make Mozilla look bad, it also makes Wikis look bad. 

Again, stop ranting and please contribute code if you really want to see this
done ASAP.

> P5 priority?  "Enhancement"?  Nay, friends -- this is a critical bug.

Certainly not. What means "critical" is defined by
http://bugzilla.mozilla.org/bug_status.html#severity
In other words, RTFM.

I wonder if it would be possible to take comment #52:
http://bugzilla.mozilla.org/show_bug.cgi?id=58305#c52

and use that (or similar) javascript in a popup XUL addition for alternative
replace code?  Could just add an option to the existing "Find" searchbox?

That is could it be added to
chrome://content/inspector/viewers/dom/FindDialog.js  ?   A nasty hack, but it
may be a way to do it.   I'm still learning this, so am not entirely sure....
anyone have thoughts, comments, or testing results on the suggestion above? 

run out of time for this in 1.7b.   might consider for 1.7 final if we think the
work around is a good idea, and has no side affects.
Flags: blocking1.7b?
Flags: blocking1.7b-
Flags: blocking1.7?
>anyone have thoughts, comments, or testing results on the suggestion above? 
Well, my code is a mess, so that would have to be rewritten.
I am not able to get the precise position of the selection when the text wraps
(the hard returns I can count but not the soft ones). So somehow that should be
fixed, otherwise in situations with lots of soft wraps, the selection is not
scrolled to the right position.

Maybe an intermediate solution (which I think is easier to do) is when the focus
is on the textarea and you press ctrl-F, you get a special search and replace
(which would be handy and is different from the regular ctrl-F) for that textarea.

Maybe I have an idea what causes this bug, maybe it is useful:
The find dialog uses range objects to select the text. In a textarea this
doesn't work very well, the text from the textNode inside the textarea element
is selected, but that doesn't become visible on screen. I think that's the
reason why they skip the textarea all together.
Besides, the textnode in the textarea is not updated when you type some text in
it, only textarea.value.
So this bug is about ctrl-f in an text area doesn't search that text area?
(In reply to comment #59)
> So this bug is about ctrl-f in an text area doesn't search that text area?

Yes/no.  It's that using the find function in mozilla doesn't search a text area
period.  Whether you initiated the search from within the text area or from the
whole page.

In contrast I don't know of any other browser that doesn't search text areas
when you search a page.  I know IE does it, I'm pretty sure Opera does it, I
know Safari does it.   Firefox/Seafox don't.
not going to block the release for this feature. A well tested patch with full
reviews and minimal or no impact on localizable strings would be considered.
Flags: blocking1.7? → blocking1.7-
doron: this bug is about the fact that find is based on ranges, as is selection,
but there's no way to get the contents of a textarea as a range so there's no
way to call find on it or to select the result if we did a find (see my comment
31).  Until there's a way to do that, the discussion has concerned alternative
workarounds like accel-F for a special textarea find that doesn't need ranges.
Code just posted to (not-quite-) duplicate Bug 120568.
*** Bug 238461 has been marked as a duplicate of this bug. ***
Well I made an extension for firefox, that popups a findtextareadialog when you
press ctr-f in a textarea.
It is still very basic, I have not changed much from the original
finddialog.xul yet.
I'm still learning all this xul/xbl stuff.
I've found an (ugly, but pretty reliable working) workaround to get to scroll
to the right position of the selection in the textarea.

But would it still be useful to provide an extension, since I see in comment 63
that there is some kind of patch?
The code posted for Bug 120568 is not a patch, it's a temporary non-working
changes made in (hopefully) progress to fixing this bug.  
-> re-assiging to myself.

I am going to take a stab at this but. As this bug is already too long, please
do not post any non-technical comments (or comments about JS extensions, etc)
while I own the bug. I am aiming at the fully-fledged version in the core code
in parity with IE. That is, make the standard Find penetrate in a natural way
into the text area/input control from the C++ side.
Assignee: kinmoz → rbs
Target Milestone: Future → mozilla1.7final
Attached patch proposed patch (obsolete) — — Splinter Review 
- the patch add supports for find in <textarea> and text <input> elements
  @see documentation at the top of nsFind.cpp for how this is done.
  This enables the editor to search & replace in the <HTML> source mode.

- fix an old bug where the mutual exclusion of the selection wasn't respected.
You can see this bug with a current Mozilla build on this bugzilla page for
example:
 1. select something outside a text input field.
 2. select something in a text input field.
    -> bug: the selection isn't cleared outside
       @see documentation in nsTextControlFrame in the patch for the fix.
Comment on attachment 145567 [details] [diff] [review]
proposed patch

Asking akkana for review since this is her code that I have been munging.
Attachment #145567 - Flags: review?(akkzilla)
Attached patch patch - take2 (obsolete) — — Splinter Review 
same functionality as the previous patch, but with some factoring of code in
nsFindContentIterator::First(), ::Last(), ::Reset() to make them more compact.
Attachment #145567 - Attachment is obsolete: true
Attachment #145567 - Flags: review?(akkzilla)
Attachment #145598 - Flags: review?(akkzilla)
Oops... I meant nsFindContentIterator::Next() ::Prev, ::Reset().
Attached patch patch - take 3 (obsolete) — — Splinter Review 
A further simplification. I removed the mIsPositioned variable. It is redundant
with the test of mInnerIterator. If you read the code: a non-null
mInnerIterator means that mOuterIterator is positioned (per the logic in the
patch).
Attachment #145598 - Attachment is obsolete: true
Attachment #145598 - Flags: review?(akkzilla)
Attachment #145649 - Flags: review?(akkzilla)
Attached patch patch - take 4 (obsolete) — — Splinter Review 
Some further simplifications and bullet-proofing... akkana, do well to consider
this one instead.
Attachment #145649 - Attachment is obsolete: true
Attachment #145649 - Flags: review?(akkzilla)
Attachment #145683 - Flags: review?(akkzilla)
Attached patch patch - take 5 (obsolete) — — Splinter Review 
Correct a blunder in the Reset() function.

The patch is nearing completion. I don't anticipate much improvements now other
than corrections that may be discovered. To maximize the chances of getting
approval for 1.7f (which is going to be a _long_ live branch), it will be good
to see some testing by those of you on the CC list who build mozilla and were
interested in this bug. Do well to report your testings to motivate drivers for
approval.
Attachment #145683 - Attachment is obsolete: true
Attachment #145683 - Flags: review?(akkzilla)
Attachment #145689 - Flags: superreview?(sfraser)
Attachment #145689 - Flags: review?(akkzilla)
As far as I can tell/test/think through again, I am now done with this gem,
except this left-over in nsFindContentIterator::Reset()

+  mRange->GetStartContainer(getter_AddRefs(endNode));
+  mRange->GetEndContainer(getter_AddRefs(endNode));

I am simply going to remove the first useless line at checkin (already done in
my tree). The patch is now as compact as I would like, and as a side benefit it
also fixes bug 189039 out-of-the-box.
Status: NEW → ASSIGNED
Comment on attachment 145689 [details] [diff] [review]
patch - take 5

Some initial comments (I haven't tested this yet):

First, thanks!	This looks like exactly the right way to solve the problem. 
It's nicely commented, too, and clear.	Most of my comments that follow are
questions about how it works, not criticism.

There are a few lines going over 80 columns -- I'd prefer that you keep to the
existing 80 column limit if possible.

It doesn't build for me as is:
| nsFind.cpp:56:23: nsIEditor.h: No such file or directory
| nsFind.cpp:57:32: nsIPlaintextEditor.h: No such file or directory

You need to add "editor" to the REQUIRES list (after which the build succeeds).
 Will everybody be okay with making find depend on editor?  I don't have any
problem with it myself.

>Index: embedding/components/find/src/nsFind.cpp
>+// 4) As of consequence of searching through text controls, we can be

Possible typo, "as a consequence of"?  (Your call.)

>+  virtual nsresult Init(nsIDOMRange* aRange);

Maybe a comment here about what aRange is?  The range over which the iterator
will operate, presumably?

In Next() and Prev():

>+  MaybeSetupInnerIterator();  
>+  if (mInnerIterator) {
>+    mOuterIterator->First();
>+    mInnerIterator->First();
>+  }

Why does mOuterIterator go back to First() here?  I'm guessing it's for the
same reason you call mOuterIterator->Init(range); in SetupInnerIterator() --
when you're done with the inner iterator then you set the outer iterator again?

In Reset():

>+  for ( ; startContent; startContent = startContent->GetParent()) {
>+    if (!startContent->IsNativeAnonymous()) {
>+      mStartOuterNode = do_QueryInterface(startContent);
>+      break;
>+    }
>+  }

What does this do?

>+  if (!mFindBackward) {
>+    if (mStartOuterNode != startNode) {
>+      SetupInnerIterator(startContent);
>+    }
>+  }
>+  else if (mEndOuterNode != endNode) {
>+    SetupInnerIterator(endContent);
>+  }

Why not MaybeSetupInnerIterator?  I'm not clear on when you need Maybe and when
you don't.

>+nsresult
>+NS_NewFindContentIterator(nsIContentIterator* aOuterIterator, 
>+                          PRBool aFindBackward,
>+                          nsIContentIterator** aResult)

I'm not clear on why you have to create a content iterator then pass it in to
get a find content iterator.  No way to create one in one step?

[to be continued]
Comment on attachment 145689 [details] [diff] [review]
patch - take 5

>Index: embedding/components/find/src/nsWebBrowserFind.cpp
>===================================================================
[ ... ]

>+// Same as the tail-end of nsEventStateManager::FocusElementButNotDocument.
>+// Used here because nsEventStateManager::MoveFocusToCaret() doesn't
>+// support text input controls.
>+static void
>+FocusElementButNotDocument(nsIDocument* aDocument, nsIContent* aContent)

I'm not very familiar with this code (Aaron, do you know it?)
Can code be shared between nsEventStateManager::FocusElementButNotDocument and
this routine?  Is there any chance that making the ESM support text controls
(either optionally or all the time) would be desirable?

The rest of nsWebBrowserfind.* look fine, at least from a quick reading.

>Index: layout/html/forms/src/nsTextControlFrame.cpp
>===================================================================
>>+#if 0 // moved to SetFocus()
>   // first, tell the caret which selection to use
>   nsCOMPtr<nsISelection> domSel;
>   GetSelection(nsISelectionController::SELECTION_NORMAL, getter_AddRefs(domSel));
>@@ -595,6 +596,7 @@
>   shell->GetCaret(getter_AddRefs(caret));
>   if (!caret) return NS_OK;
>   caret->SetCaretDOMSelection(domSel);
>+#endif

Why leave the dead code there?	Better to remove it entirely.

Aaron, can you comment on the changes to this file?  Is it okay to move this
code from SetCaretEnabled() to SetFocus()?

I'll do some testing of the patch over the next few days. I hope others who are
interested can also help with testing.
> There are a few lines going over 80 columns -- I'd prefer that you keep to the
> existing 80 column limit if possible.

Will do.

> 
> It doesn't build for me as is:
> | nsFind.cpp:56:23: nsIEditor.h: No such file or directory
> | nsFind.cpp:57:32: nsIPlaintextEditor.h: No such file or directory
> 
> You need to add "editor" to the REQUIRES list (after which the build
> succeeds).
>  Will everybody be okay with making find depend on editor?  I don't have any
> problem with it myself.

Apparently, the REQUIRES changes that I made didn't show up on the patch.
Strange that the |Makefile|s are excluded from cvs diff?

I don't have a strong feeling either way regarding the dependency. Some public
methods of nsITextControlFrame actually drag in the editor (ender).

> 
>>Index: embedding/components/find/src/nsFind.cpp
>>+// 4) As of consequence of searching through text controls, we can be
> 
> 
> Possible typo, "as a consequence of"?  (Your call.)

Corrected.

>>+  virtual nsresult Init(nsIDOMRange* aRange);
> 
> Maybe a comment here about what aRange is?  The range over which the iterator
> will operate, presumably?

Yes, that's what it is, but... it operates in a segmented manner (see below).

> In Next() and Prev():
> 
>>+  MaybeSetupInnerIterator();  
>>+  if (mInnerIterator) {
>>+    mOuterIterator->First();
>>+    mInnerIterator->First();
>>+  }
> 
> Why does mOuterIterator go back to First() here?  I'm guessing it's for the
> same reason you call mOuterIterator->Init(range); in SetupInnerIterator() --
> when you're done with the inner iterator then you set the outer iterator
> again?

This goes to the nub of the idea behind the patch, and as I will explain, it is
why it works reliably. When an mInnerIterator is created, the mOuterIterator is
re-inited with the remaining _segment_ of mRange which is yet to be visited
(picture the segment rightward in find-forward and leftward in find-backward).
It is a total re-init. Thus mOuterIterator has to be put at First() or at Last()
accordingly. Its re-init() happens early in SetupInnerIterator, but
First()/Last() happen later when it becomes known that we are in the context of
Next()/Prev(). The nett result of this process is to put mOuterIterator on the
node after (or before) the <textarea>. It is more readable and less error-prone
that trying to loop pass a "<textarea>...big markup here...</textarea>", which
might be the last/first element. Thus we use a robust way to examine the live
text in the editor while skipping the initial text, including edge cases where
only a part of the text control has to be examined.

> In Reset():
> 
>>+  for ( ; startContent; startContent = startContent->GetParent()) {
>>+    if (!startContent->IsNativeAnonymous()) {
>>+      mStartOuterNode = do_QueryInterface(startContent);
>>+      break;
>>+    }
>>+  }
>
> What does this do?

This is what I commented in point 4) in the header. If the current selection is
inside a text control, the starting point is the corresponding (anonymous) text
node. Therefore the for-loop above will walk up, all the way to the
(non-anonymous) <textarea> node itself and use it as the reference outer-point.

With WRAPPING on, find happens from SelEnd to DocEnd and then DocStart to
SelStart. Hence it is possible for the anonymous text node to be either a
starting point or an ending point. That's why Reset() takes no chance and deals
with both situtations. Also, we can hit either point later as we iterate in the
find, and that's why there is a check again later in SetupInnerIterator.

>>+  if (!mFindBackward) {
>>+    if (mStartOuterNode != startNode) {
>>+      SetupInnerIterator(startContent);
>>+    }
>>+  }
>>+  else if (mEndOuterNode != endNode) {
>>+    SetupInnerIterator(endContent);
>>+  }
> 
> Why not MaybeSetupInnerIterator?  I'm not clear on when you need Maybe and
> when you don't.

If there is no anonymous text node (continuing with my clarifications above), we
will get mStartOuterNode == startNode. Otherwise, there _is_
a <textarea> (or text <input>). It is not "maybe" anymore, and we can go
straight to business, without having to depend, at this point, on
mOuterIterator->GetCurrentNode().
 
>>+nsresult
>>+NS_NewFindContentIterator(nsIContentIterator* aOuterIterator, 
>>+                          PRBool aFindBackward,
>>+                          nsIContentIterator** aResult)
> 
> I'm not clear on why you have to create a content iterator then pass it in to
> get a find content iterator.  No way to create one in one step?

Possible. It crossed my mind. But for the sake of readability, and with the
business of segmented ranges, I prefer to have, in effect, three iterators. The
main wrapper for the outside world, then those inner- and outer- helpers that
change dynamically. [If the outer one is the main one, looping pass a <textarea>
would be lot messier.]

Should I |new| the outer-iterator inside or outside? That was another question.
I simply opted to re-use the code with its documentation, and see what reviewers
think. I can move that inside FindContentIterator::Init() if you like.

> Is there any chance that making the ESM support text controls
> (either optionally or all the time) would be desirable?

I actually tried to track the actual caret's DOM selection in the ESM, but
everybody and their friends there assume that it comes from the pres shell. It
was so much hassle and riskier at this 1.7f stage than replicating
FocusElementButNotDocument().

> Why leave the dead code there?	Better to remove it entirely.

Will do.

>Aaron, can you comment on the changes to this file?  Is it okay to 
>move this code from SetCaretEnabled() to SetFocus()?

Actually setting the caret's DOM selection in SetCaretEnabled() was considered a
nasty side-effect in the pres shell.
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsPresShell.cpp#3096
It seems more natural to set it (by default) when the element has focus, while
leaving other non default settings to callers that explicitly want to do it.
rbs: you need to change Makefile.in
- add editor in REQUIRES
- more comments on how it works
- hide the outer-iterator from the outside world
- limit to 80ch
- remove dead code
Attachment #145689 - Attachment is obsolete: true
Attachment #145689 - Flags: superreview?(sfraser)
Attachment #145689 - Flags: review?(akkzilla)
Attachment #145985 - Flags: superreview?(sfraser)
Attachment #145985 - Flags: review?(akkzilla)
Comment on attachment 145985 [details] [diff] [review]
patch - updated per review comments

Sorry, this is a large patch and I don't have time to superreview.
Attachment #145985 - Flags: superreview?(sfraser) → superreview?
Attachment #145985 - Flags: superreview? → superreview?(jst)
Comment on attachment 145985 [details] [diff] [review]
patch - updated per review comments

r=akkana

The changes all look good.  I like hiding the outer iterator from the outside
world in the creator ... seems cleaner.

I haven't done a lot of testing, but this works on textareas forward and
backward, and it still passes all the Find regression tests at
http://www.mozilla.org/quality/browser/front-end/testcases/xpapps-gui/find-on-p
age/find-in-page.html

I saw one weirdness: if you use find-in-page for a string that is found in a
textarea, the string is not highlighted, though it is correctly found.	With
normal find, from the dialog, the string is highlighted; and find-in-page
highlights strings outside of textareas.  Not a big problem -- scrolling
happens correctly and the caret ends up at the right place, which are the
important things.

If JST doesn't have time, Kin would be a good super-reviewer.
Attachment #145985 - Flags: review?(akkzilla) → review+
From comment 82 , that would be bug 134586 I think.
(Great job by the way, rbs)

> From comment 82 , that would be bug 134586 I think.

It looks like that's indeed the problem, and it is Unix-specific. I wasn't even
aware of that difference because the find's selection in the textarea appears
fine on my Win2K box (the correct scrolling is indicative that it is a paint
problem due to that reported Unix paint difference). I wish bug 134586 could be
fixed too because Unix users will miss out on the visual cue that the visible
selection provides for the find.
akkana, martijn (if you have a ready build with the patch), do you mind trying
the following experiment in nsWebBrowserFind::SetSelectionAndScroll (two
changes) and see if it makes any difference?!?

  if (selection) {
    selection->RemoveAllRanges();
-//OLD: commented    selection->AddRange(aRange);
    // Scroll if necessary to make the selection visible:
    selCon->ScrollSelectionIntoView
      (nsISelectionController::SELECTION_NORMAL,
       nsISelectionController::SELECTION_FOCUS_REGION, PR_TRUE);

    if (tcFrame)
+{
      FocusElementButNotDocument(doc, content);
+// NEW: focus _before_ selecting 
      selection->AddRange(aRange);
+}
    else {
+// keep old behavior for normal find
+     selection->AddRange(aRange);

      nsCOMPtr<nsIPresContext> presContext;
      presShell->GetPresContext(getter_AddRefs(presContext));
      PRBool isSelectionWithFocus;
      presContext->EventStateManager()->
        MoveFocusToCaret(PR_TRUE, &isSelectionWithFocus);
    }
  }
Oops... the suggested change is not quite correct. Scroll is misplaced -- it
should happen after adding the range.
Sorry, I'm leaving town today and won't have net access for a week or two ...
maybe martijn can test it.
>I saw one weirdness: if you use find-in-page for a string that is found in a
                                 ^^^^^^^^^^^^
>textarea, the string is not highlighted, though it is correctly found.	With
>normal find, from the dialog, the string is highlighted;
 ^^^^^^^^^^^

It just occurred to me that you may be meaning 'find-as-you-type'... and that
all is well with "normal find" (what we actually know as find-in-page).

If that is so, yes I saw that. The matches from 'find-as-you-type' are not
highlighted. But the scrolling and cursor work fine. It is a really a tagent
from the initial goal. I think it is something that aaronl can look at later. My
patch is clearly doing a selection, but his code seems to clear that in order to
make it green (as find-as-you-type does), but end up not doing it. But even
without the selection, the visible cursor (in the textarea) gives a cue in that
very specific case.
*** Bug 240488 has been marked as a duplicate of this bug. ***
Yes, oops, I meant find-as-you-type, and yes, bug 134586 sounds like it.
Comment on attachment 145985 [details] [diff] [review]
patch - updated per review comments

sr=jst
Attachment #145985 - Flags: superreview?(jst) → superreview+
Comment on attachment 145985 [details] [diff] [review]
patch - updated per review comments

checked into the trunk.

Asking approval for 1.7 as this is a much sought-after feature that stops IE
users from fully migrating to Gecko.
Attachment #145985 - Flags: approval1.7?
This broke the tree due to changes that don't seem to be in the most recent
patch on this bug.  I emailed rbs, but my email bounced:

Final-Recipient: rfc822; <rbs@maths.uq.edu.au>
Action: failed
Status: 5.1.0 MAIL FROM: <dbaron@dbaron.org> 550 REPLY: 550_5.7.1_Access_denied
Diagnostic-Code: smtp; Permanent Failure: Other address status
Last-Attempt-Date: Thu, 15 Apr 2004 20:18:56 -0000
The bustage has nothing to do with REQUIRES as rbs indicated on tinderbox.  The
problem is that there's no |mFindForward| variable.
(And in the future, it would be much easier if you were on irc.mozilla.org
#developers when you break the tree, especially since your email doesn't work.)
Thanks for the follow-up, dbaron, the bustage came from a typo on a last minute
extra. I just checked in a fix. (I don't know why my e-mail bounced, BTW. I
cannot be on IRC. Our local policy denies such access.)
Why is this bug left open if the fix was checked in ?
Marking FIXED. The wait for approval for 1.7f doesn't depend on that indeed.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Looks like bug 120568 is a dupe of this one, isn't it?
Bug 120568 wants Find and Replace, not just Find.
rbs: I've also had mail bounce trying to mail you directly.  The message was:

<rbs@maths.uq.edu.au>: host mailhub2.uq.edu.au[130.102.5.59] said: 550 5.7.1
    Access denied

Perhaps an overly aggressive spam filter?  (Sorry to spam this bug, but
obviously neither of us can mail rbs with the error msg ...)
(In reply to comment #100)
> Bug 120568 wants Find and Replace, not just Find.

It took three years to get find.  Now they want find and replace?  Yeeeesh.  Not
much more to add to get that.  But I see problems.  Basically we have to
distinguish if the element found is within a form.  Not to mention if it's in
the correct form that the user wanted the replace to occurr.  Although... I was
thinking about whether I'd want just the same type of thing myself.   Is Mozilla
a text editor? ;)
*** Bug 241645 has been marked as a duplicate of this bug. ***
Comment on attachment 145985 [details] [diff] [review]
patch - updated per review comments

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #145985 - Flags: approval1.7? → approval1.7+
-> Now FIXED on the 1.7 branch.
Keywords: fixed1.7
*** Bug 243423 has been marked as a duplicate of this bug. ***
Now find-as-you-type seems to be searching in text fields, but the text isn't
highlighted at all.  This is very confusing to the user.
I think there are some other weirdnesses with the way it works with Find As You
Type. For example, when FAYT is finished (via Escape to cancel or timeout), the
focus goes outside the textarea even if that's where the text was found. You
should be in the text area with the text selected at that point.
Please use bug 189039 for the remaining specific issues for find-as-you-type. I
think FAYT needs to take into account the _new_ fact that matches can be inside
textareas. As this bug has shown, things are little bit different with text
boxes. I can assist in bug 189039 if you need clarifications in certain points.

In particular (re: comment 107 and comment 108), to make the selection green (as
FAYT does) or focus, FAYT needs to be careful with two things:
- make sure to sync the primary frame as the text control, as appopriate
- make sure to use the LOCAL selection inside the textarea, as appropiate.
[I looked at the code of FAYT when I was working on this bug and FAYT was only
paying attention at the GLOBAL document selection.]
verified
Status: RESOLVED → VERIFIED
Keywords: fixed1.7 → verified1.7
Product: Core → Mozilla Application Suite
Can we have this reopened for firefox?  Since it's not fixed there?...  

Affects:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a6) Gecko/20041218 Firefox/1.0+

Unless there is something wrong with the build I'm using....
(In reply to comment #111)
> Can we have this reopened for firefox?  

No, since this is not a Firefox bug, bug 252371 is
No longer blocks: 164421
Blocks: 158464
*** Bug 152299 has been marked as a duplicate of this bug. ***
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: