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)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
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)
19.48 KB,
application/x-xpinstall
|
Details | |
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.
Comment 2•24 years ago
|
||
Find in page is supposedly handled by editor. Sending to beppe.
Assignee: ben → beppe
Comment 3•24 years ago
|
||
simon, would this be yours?
Comment 5•24 years ago
|
||
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.
Comment 6•24 years ago
|
||
that is consistent with 4.x, but is a nice enhancement request, setting to future
Severity: normal → enhancement
Priority: P3 → P5
Target Milestone: --- → Future
Comment 7•24 years ago
|
||
This is an important feature for web applications that use large TEXTAREAs, like when viewing NOTES at Yahoo! Calendar. IE5 has this feature.
Comment 10•23 years ago
|
||
*** Bug 104688 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Whiteboard: parity-ie
Comment 11•23 years ago
|
||
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
Comment 12•23 years ago
|
||
remove self
Comment 13•22 years ago
|
||
*** Bug 135095 has been marked as a duplicate of this bug. ***
Comment 14•22 years ago
|
||
*** Bug 124989 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
OS: Windows 98 → All
Comment 15•22 years ago
|
||
See also bug 120568, "Find/Replace should be available for text fields in Navigator".
Comment 16•22 years ago
|
||
*** Bug 179858 has been marked as a duplicate of this bug. ***
Comment 17•22 years ago
|
||
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.
Comment 18•22 years ago
|
||
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?
Updated•22 years ago
|
QA Contact: pmac → sairuh
Comment 19•22 years ago
|
||
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...
Reporter | ||
Comment 20•22 years ago
|
||
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
Comment 22•22 years ago
|
||
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.
Comment 23•22 years ago
|
||
*** Bug 192645 has been marked as a duplicate of this bug. ***
Comment 24•22 years ago
|
||
*** Bug 194129 has been marked as a duplicate of this bug. ***
Comment 25•21 years ago
|
||
*** Bug 199894 has been marked as a duplicate of this bug. ***
Comment 26•21 years ago
|
||
*** Bug 200495 has been marked as a duplicate of this bug. ***
Comment 27•21 years ago
|
||
*** Bug 201179 has been marked as a duplicate of this bug. ***
Comment 28•21 years ago
|
||
*** Bug 201206 has been marked as a duplicate of this bug. ***
Comment 29•21 years ago
|
||
Teaking summary to help avoid dupes.
Summary: Find in page ignores text fields → Find in page ignores text fields - does not search form textarea
Comment 30•21 years ago
|
||
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 31•21 years ago
|
||
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
Comment 32•21 years ago
|
||
*** Bug 210168 has been marked as a duplicate of this bug. ***
Comment 33•21 years ago
|
||
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.
Comment 34•21 years ago
|
||
any editor hacking still going on?
chofmann: yes; but unfortunately lower priority than finding another job position these days.
Comment 36•21 years ago
|
||
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.
Comment 37•21 years ago
|
||
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?
Comment 38•21 years ago
|
||
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
Comment 39•21 years ago
|
||
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
Comment 40•21 years ago
|
||
> 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!
Comment 41•21 years ago
|
||
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!
Comment 42•21 years ago
|
||
*** Bug 222643 has been marked as a duplicate of this bug. ***
Comment 43•21 years ago
|
||
>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.
Comment 44•21 years ago
|
||
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.
Comment 45•21 years ago
|
||
*** Bug 228392 has been marked as a duplicate of this bug. ***
Comment 46•21 years ago
|
||
*** Bug 232671 has been marked as a duplicate of this bug. ***
Comment 47•21 years ago
|
||
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.
Comment 48•21 years ago
|
||
*** Bug 233841 has been marked as a duplicate of this bug. ***
Comment 49•21 years ago
|
||
*** Bug 202074 has been marked as a duplicate of this bug. ***
Comment 50•21 years ago
|
||
*** Bug 235598 has been marked as a duplicate of this bug. ***
Comment 51•21 years ago
|
||
Any news on this annoying bug, especially in connection with sites such as Wikipedia where searching within textareas is very important?
Comment 52•21 years ago
|
||
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
Updated•21 years ago
|
Hardware: PC → All
Whiteboard: parity-ie → parity-ie | For a workaround, see comment #52
Comment 53•21 years ago
|
||
(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...
Comment 54•21 years ago
|
||
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.
Updated•21 years ago
|
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.
Comment 56•21 years ago
|
||
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....
Comment 57•20 years ago
|
||
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?
Comment 58•20 years ago
|
||
>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.
Comment 59•20 years ago
|
||
So this bug is about ctrl-f in an text area doesn't search that text area?
Comment 60•20 years ago
|
||
(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.
Comment 61•20 years ago
|
||
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-
Comment 62•20 years ago
|
||
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.
Comment 63•20 years ago
|
||
Code just posted to (not-quite-) duplicate Bug 120568.
Comment 64•20 years ago
|
||
*** Bug 238461 has been marked as a duplicate of this bug. ***
Comment 65•20 years ago
|
||
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?
Comment 66•20 years ago
|
||
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.
Assignee | ||
Comment 67•20 years ago
|
||
-> 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
Assignee | ||
Comment 68•20 years ago
|
||
- 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.
Assignee | ||
Comment 69•20 years ago
|
||
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)
Assignee | ||
Comment 70•20 years ago
|
||
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)
Assignee | ||
Comment 71•20 years ago
|
||
Oops... I meant nsFindContentIterator::Next() ::Prev, ::Reset().
Assignee | ||
Comment 72•20 years ago
|
||
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)
Assignee | ||
Comment 73•20 years ago
|
||
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)
Assignee | ||
Comment 74•20 years ago
|
||
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)
Assignee | ||
Comment 75•20 years ago
|
||
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 76•20 years ago
|
||
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 77•20 years ago
|
||
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.
Assignee | ||
Comment 78•20 years ago
|
||
> 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.
Comment 79•20 years ago
|
||
rbs: you need to change Makefile.in
Assignee | ||
Comment 80•20 years ago
|
||
- 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 81•20 years ago
|
||
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 82•20 years ago
|
||
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+
Comment 83•20 years ago
|
||
From comment 82 , that would be bug 134586 I think. (Great job by the way, rbs)
Assignee | ||
Comment 84•20 years ago
|
||
> 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.
Assignee | ||
Comment 85•20 years ago
|
||
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); } }
Assignee | ||
Comment 86•20 years ago
|
||
Oops... the suggested change is not quite correct. Scroll is misplaced -- it should happen after adding the range.
Comment 87•20 years ago
|
||
Sorry, I'm leaving town today and won't have net access for a week or two ... maybe martijn can test it.
Assignee | ||
Comment 88•20 years ago
|
||
>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.
Assignee | ||
Comment 89•20 years ago
|
||
*** Bug 240488 has been marked as a duplicate of this bug. ***
Comment 90•20 years ago
|
||
Yes, oops, I meant find-as-you-type, and yes, bug 134586 sounds like it.
Comment 91•20 years ago
|
||
Comment on attachment 145985 [details] [diff] [review] patch - updated per review comments sr=jst
Attachment #145985 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 92•20 years ago
|
||
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.)
Assignee | ||
Comment 96•20 years ago
|
||
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 ?
Assignee | ||
Comment 98•20 years ago
|
||
Marking FIXED. The wait for approval for 1.7f doesn't depend on that indeed.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 99•20 years ago
|
||
Looks like bug 120568 is a dupe of this one, isn't it?
Comment 100•20 years ago
|
||
Bug 120568 wants Find and Replace, not just Find.
Comment 101•20 years ago
|
||
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 ...)
Comment 102•20 years ago
|
||
(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? ;)
Comment 103•20 years ago
|
||
*** Bug 241645 has been marked as a duplicate of this bug. ***
Comment 104•20 years ago
|
||
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+
Assignee | ||
Comment 105•20 years ago
|
||
-> Now FIXED on the 1.7 branch.
Comment 106•20 years ago
|
||
*** Bug 243423 has been marked as a duplicate of this bug. ***
Comment 107•20 years ago
|
||
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.
Comment 108•20 years ago
|
||
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.
Assignee | ||
Comment 109•20 years ago
|
||
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.]
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
Comment 111•20 years ago
|
||
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....
Comment 112•20 years ago
|
||
(In reply to comment #111) > Can we have this reopened for firefox? No, since this is not a Firefox bug, bug 252371 is
Comment 113•19 years ago
|
||
*** Bug 152299 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•