80805 - Composer should use new find component

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Kathleen :Brade]

Composer should use the new find component.
The only library should be removed from the build/tree.

Comment 1

[rubydoo123]

moving to 9.2

Comment 2

[Simon Fraser [no longer active]]

This requires a new interface for Find and Replace, in a similar way to how we do 
find now. I think that's too major for 0.9.2

Comment 3

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 4

[Kathleen :Brade]

reassign to Akkana since she's removing on this or something similar.

Comment 5

[Akkana Peck]

Attachment: Patch: add needed accessors to the <editor> tag's XBL

Comment 6

[Akkana Peck]

Attachment: Patch: new editor replace dialog

This looks exactly like the old editor replace dialog, but IMHO needs cosmetic
work: OK, Cancel and Close are all redundant with each other.  I need a way of
preventing the dialog from showing OK and Cancel.

Comment 7

[Akkana Peck]

These patches are dependant on the new find component introduced by the patch in
bug 97157.

I'd like to get Simon's review on the xbl part of the patch, since he helped on
that.  Note that I added a .editor (vs. .editorShell) accessor -- we're planning
to remove the editor shell, so I didn't want to add any new code that used it,
and this will help other components wean themselves away from it.  The
implementation of the accessor still uses .editorShell, which will be fixed with
the other editor shell work.  Simon, if you can sr this, who else (if anyone)
should I ask for a review?

Charley, can you review the dialog part of the patch, and perhaps suggest a way
that I can get rid of those redundant buttons?

Worth noting is that the new find component is find only, not replace.  The
editor is the only component with enough knowledge to replace safely, so I'm
doing the replace in editor code, not find code.  If there are other components
who want to do a replace, the find component sets the selection around the found
text, so if they want, they can call Range delete and insert methods.  They
won't get the benefit of edit rules, but that's the risk they run by not using
an editor for text replacement.

Comment 8

[Charles Manske]

Akkana: This fix is missing a new EditorReplace.dtd.

Comment 9

[Akkana Peck]

Attachment: New editor replace dialog, including EditorReplace.dtd

Oops!  Thanks for noticing.  Here's a new patch.

Comment 10

[Charles Manske]

Attachment: Updated patch: Fix extra "Ok" button

Comment 11

[Charles Manske]

Comment on attachment 62587 [details] [diff] [review]
Updated patch: Fix extra "Ok" button

r=cmanske

Comment 12

[Charles Manske]

Oh yea, one comment: Don't we need the license stuff at the top of 
EditorReplace.dtd?

Comment 13

[Akkana Peck]

The new find code (97157) went in, but it's preffed off by default because there
wasn't time to test/review it. So this has to wait until new find is switched on.

Comment 14

[Charles Manske]

*** Bug 122717 has been marked as a duplicate of this bug. ***

Comment 15

[Joe Hewitt (gone)]

Comment on attachment 62587 [details] [diff] [review]
Updated patch: Fix extra "Ok" button

In this line, you don't need the oncommand="close()", you get that for free by
specifying dlgtype="cancel"

+      <button dlgtype="cancel" id="close" label="&closeButton.label;"
oncommand="close();"/>


fix that and sr=hewitt

Comment 16

[Akkana Peck]

Attachment: Patch, the next generation

Okay, I checked in the part that was reviewed, except for ComposerCommands.js
since we can't turn the feature on yet.

However, in the meantime, naturally, the patch has broken, probably due to
Mike's editor embedding landing.  In EdReplace.js, 
  var editorXUL = window.opener.document.getElementById("content-frame");
isn't getting anything, so gFindInst is null, so nothing works.

The files like jar.mn and various MANIFESTs also somehow got dropped from the
patch somewhere along the way.	I've made a new patch which includes these
files , and adds the pref in to ComposerCommands.js, but I can't test it
because of the "content-frame" problem.

Charley, I don't suppose you know offhand how I can get the xul <editor>
element now?

Comment 17

[Charles Manske]

Not being able to get the <editor> element by getElementById is news to me!
Mike?

Comment 18

[Brian King [:kinger]]

Shouldn't it just be window.opener.getElementById("content-frame")? If not, try
window.opener.frames[0] or whatever the index is if there is more than one frame
(editor, iframe, browser) in the document you are accessing.
Not sure if editor is in this array.

Comment 19

[Akkana Peck]

If I try that I get: window.opener.getElementById is not a function.  Not sure
about the frames[].  We're asking mjudge now ...

Comment 20

[Akkana Peck]

*** Bug 95461 has been marked as a duplicate of this bug. ***

Comment 21

[Mike Lee]

My addon [http://mozblog.mozdev.org ], make use of frames to setup the window
for editor (with the browser). Its  a (very) bad hack, but it's the only way I
can get
<editor> and <browser> on the same page started without stuffing up browsing
framed pages. There should be a better way to get windows instance. Right now
getElementById live inside document and not window.

Comment 22

[Akkana Peck]

Comment on attachment 62253 [details] [diff] [review]
Patch: add needed accessors to the <editor> tag's XBL

Well, at least part of the problem is that this part of the patch never got
reviewed or checked in!

Comment 23

[Charles Manske]

Comment on attachment 68954 [details] [diff] [review]
Patch, the next generation

r=cmanske

Comment 24

[R.K.Aa.]

on a new non-debug CVS build, Linux, I'm getting several thousand debug lines in
console for a single "find on page". It also changes charset in console on
subsequent finds, so i have to reset all the time with ctrl+v + ctrl+o

Related to this checking?
If so: Can the output be disabled somehow? It's a lot of noise.

Comment 25

[Akkana Peck]

Oh, gack.  DEBUG_FIND is on in nsFind.cpp.  I swear I checked that and turned it
off before checkin!  But I must have turned it back on again.  I'll fix it today
as soon as the tree opens, I promise!

Comment 26

[R.K.Aa.]

the tree is closed untill the freeze, i think. From tinderbox:

"Checkins will be Metered by the Sheriff until the freeze. Contact the Sheriff
to get scheduled"

Comment 27

[Akkana Peck]

The fix went in a few hours ago.  Sorry about that.

Re the composer stuff: the new composer find dialog is in, but switched off by
default.  Since new find in the browser is on by default, I put composer
find/replace on a new pref: user_pref("editor.new_find", true) will turn it on.

It currently sometimes hits a crash during find/replace in tricky pages.  The
crash is in nsDeque code, and timeless has a fix for that in bug 114166, so I've
made this bug dependant on that.

I'm not sure I'm always doing the right thing for replace -- e.g. if you bring
up the find dialog, enter a find string and a replace string, and click
"replace" without first clicking "find", should it find the first instance then
replace it?  Currently it just replaces the current selection, which is wrong.

Also, the dialog is modal, which really sucks (you can't scroll the composer
window or change the selection to make it find from a different place).  The
dialog shouldn't be modal and I may need Charley's help in fixing that.

Other than those issues, it just needs testing before we enable it.

Leaving on 0.9.9 for the moment, though it's likely that fixing the remaining
problems and making it the default will slip to 1.0.

Comment 28

[Charles Manske]

*** Bug 97689 has been marked as a duplicate of this bug. ***

Comment 29

[Brian King [:kinger]]

Akkana, to have the dialog non-modal, launch it with the 'dependent' flag. This
will keep it on top, but allow access to the buffer underneath. Example here:

http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/findUtils.js#61

window.openDialog("chrome://global/content/finddialog.xul", "_blank",
"chrome,resizable=no,dependent=yes", findInst);

Comment 30

[Charles Manske]

Attachment: Patch to use "Close" instead of "Cancel"

Dialogs that change document should use "Close" instead of "Cancel"

Comment 31

[Akkana Peck]

This needs some more code to be written (to get the selection in JS and compare
it against the pattern) so 0.9.9 probably isn't realistic -- bumping to 1.0.

Comment 32

[Akkana Peck]

Attachment: Check the selection before replacing, and find first if necessary

New patch, which in the replace case, checks the selection to see if we're
already pointing at a match, and if we're not, first does a find before
replacing.

To do this, I also added return values to some of the dialogs which weren't
returning any consistent value before.	Charley: does it matter what value is
returned by the onXXX functions called from the buttons in the dialog?	If so,
I'll need to restructure this slightly to make the onXXX functions return the
right value (and use different routines to return true or false for internal
purposes).

I've also removed modal and added dependant=yes as Brian suggested; but it
doesn't work (the dialog doesn't stay on top of the editor window).  That's
still better than it being modal, since we don't want it to prevent the user
from selecting in the editor window, but it sure would be nice if we could keep
the dialog on top.

This could use more testing than I've given it (I haven't yet written a tough
test case for replace) but I think it's pretty close to ready.

Comment 33

[Charles Manske]

The "dependant=yes" seems to work for the Find dialog, doesn't it? Could this be
a Linux-only problem?
Re: return values from onLoad: Be careful! you can use your own return values for 
that method or other "internal" methods, but returning true from a method tied to 
one of the buttons like "Close" will close the dialog.

Comment 34

[Brian King [:kinger]]

dependant=yes should read dependent=yes
      ^                         ^
It's a typo. Fixing that should do it.
Also, using just 'dependent' should work.

Comment 35

[Charles Manske]

Comment on attachment 71407 [details] [diff] [review]
Check the selection before replacing, and find first if necessary

This fix is contingent on fixing the nsIEditor.idl for getting the seleciton.
Just for conistency of style: we usually use the "gDialog" global just for XUL
elements in the dialog. Thus I would have use "gEditor" instead of
gReplaceDialog.editor.

Comment 36

[Kathleen :Brade]

in isSpace, could you add a comment to explain that '/a0' is an nbsp?

Comment 37

[rubydoo123]

removing myself from the cc list

Comment 38

[Akkana Peck]

Attachment: newer patch

Comment 39

[Akkana Peck]

Attachment: newer patch

Comment 40

[Akkana Peck]

Attachment: improved patch

This patch:
1. adds Charley's new UI.
2. Adds the comment Kathy requested.
3. Adds a fix for an infinite-loop problem if you do "replace all" with
wraparound set where the replacement can trigger more matches, e.g. replace a
with aa.
(It also provides an example of how to use nsIFind from JS, which someone asked
about in another bug.)

If you find from the middle of a document with wrap on, it can show two
problems:
1. Sometimes it does one more replace than it should, if the first match is in
the same node as the selection is when you start.  I think the code here is
right and this may be a bug in nsFind, but I have to investigate further.
2. Sometimes it ends up with both a selection AND a blinking caret, not in the
same place.  That shouldn't be possible and suggests an editor bug, and I
wonder if the first problem could possibly be resulting from the second?

I'd like to get a review on this and check it in anyway (since I expect the fix
will be somewhere other than these files).  And consider whether we're ready to
turn the feature on by default, or should we wait until these two problems are
better understood?

Comment 41

[Charles Manske]

Comment on attachment 73269 [details] [diff] [review]
improved patch

r=cmanske
I'd prefer using 
"findInput" and "replaceInput" instead of "findKey" and "replaceKey"

Comment 42

[Akkana Peck]

Attachment: Rename "Key" to "Input", fix enabling

I renamed Key to Input, and noticed that the enabling wasn't working right for
the new button (or, sometimes, for the other replace buttons).	Also, we were
getting the text field's value too often, which is a performance problem. 
Here's a fix (otherwise just like the previous patch).

Comment 43

[Akkana Peck]

Attachment: Same patch with some cleanups

Charley had some minor cleanup he wanted done, so I've done that.

Comment 44

[Akkana Peck]

Attachment: Really same patch with some cleanups

Oops, attached the wrong file (older version).	Here's the real one.

Comment 45

[Charles Manske]

Comment on attachment 73337 [details] [diff] [review]
Really same patch with some cleanups

r=cmanske.
Note that gEditor.select depends on fix in 128903.

Comment 46

[Akkana Peck]

The latest patch has some bogus capitalization changes ResetModificationCount()
to resetModificationCount().  Those are part of another bug, NOT part of this
patch.  (Sorry, juggling too many patches in the same tree.)

Comment 47

[Daniel Veditz [:dveditz]]

Comment on attachment 73337 [details] [diff] [review]
Really same patch with some cleanups

>+// Test to see if a character is whitespace, including a0 ( ).
>+function isSpace(ch)
>+{
>+  return (ch == ' ' || ch == '\t' || ch == '\n' || ch == '\a0');
> }

This seems like a big potential performance issue since for each
char through your loops you've got interpreted function call
overhead plus several interpreted string compares.

If you can re-write this using a pre-compiled regexp you'd
probably get quite a boost. For example, instead of using
  isSpace(x)
you could use
  /\s/.test(x)
which would at least move the compares out of the interpreter
if not the function call. \s includes the 0xa0 char you're concerned
about, plus a couple others.

>+  // Unfortunately, because of whitespace we can't just check
>+  // whether (selStr == specStr), but have to loop ourselves.
>+  // N chars of whitespace in specStr can match any M >= N in selStr.

oh, I see. Drat. If you didn't care that M >= N but instead
considered any batch of whitespace equal to any other you
could do much better. I now see that the actual finding is
done elsewhere in a native component so I guess it's not as
big a deal as I thought. but still, here's what I was thinking:

  specArray = specStr.match(/\S+/g);
  selArray = selStr.match(/\S+/g);
  if ( specArray.length != selArray.length)
    matches = false;
  else 
    for (i=0; i<selArray.length; i++) {
      if (selArray[i] != specArray[i]) {
	matches = false;
	break;
      }
    }

If leading/trailing space is significant you could use
selStr.split(/\s+/) instead (note lower vs uppercase s).

>+      // They're both whitespace, so skip past whitespace in both strings
>+      while (isSpace(specStr.charAt(i)) && i < specLen)
>+        ++i;
>+      while (isSpace(selStr.charAt(j)) && j < selLen)
>+        ++j;

So any given chunk of whitespace doesn't have to match or exceed its
equivalent, it's the total number of whitespace chars if there are
multiple chunks of whitespace. The only enforcement of M>=N seems to
be in the total length of the strings

I'm a little confused that onReplaceAll seems to be doing
something completely different. Why?

Comment 48

[Akkana Peck]

Attachment: Patch to nsFind to make it stop at the right place

I found the problem that was making replace-all sometimes do a few extra
replaces at the end.  This patch (when combined with the JS patch) fixes the
last known bug in replace all, and means that we should be clear to enable the
feature by default.

Comment 49

[Akkana Peck]

Explanation of the nsFind.cpp changes:
- The SkipNode change is actually for bug 129971: the find code was sometimes
imappropriately skipping nodes that came before or after comment nodes.  Since
that current SkipNode code checks parentage, positioning the iterator shouldn't
be necessary any more.
- I noticed that due to a shuffling of routines inside nsFind.cpp, I'd ended up
with a situation where a method that's declared to return nsresult was actually
returning PR_TRUE and PR_FALSE.  Bogus, so I changed them all to NS_OK, making
sure they all called ResetAll() first, and making sure that the range return was
properly initialized to null.
- The end offset wasn't being checked in the case where the end container is a
text node.  The iterator properly stopped at the last node, but we didn't stop
text comparisons when we got to the end offset.  That's why ReplaceAll wasn't
stopping soon enough.  

Comment 50

[Akkana Peck]

dveditz: 
Are you sure that \s andr \S includes the nbsp character a0 as whitespace?  I
thought I'd tried that and established that it didn't, and I know Charley and I
checked his existing isSpace routine and it didn't handle that character. 
Charley just checked Flanagan, "JavaScript, the definitive Guide" (O'Reilly) and
it said "whitespace" = [ \t\n\r\f\v].

If there is a faster JS routine which includes the a0 character, then that would
be a big help.  But checking for a0 is very important here.

Yes, the code would be a lot simpler if we didn't insist that M >= N and just
matched any amount of whitespace, but Kin says that's important (he rejected an
earlier version of nsFind.cpp which made that assumption.

The reason onReplaceAll is so different from the other routines: first,
onReplaceAll doesn't care whether the current selection matches the pattern,
because it's going to go through the whole document.  So that code doesn't have
to be there.  (nsFind does essentially the same thing, matching whitespace and
so forth, in C++ under the hood.)  What ReplaceAll does need to care about is
that it start from the current position, go to the end of the document, then
start from the beginning of the document and end up at the current position. 
nsIWebBrowserFind (which is what the rest of the find/replace dialog code uses)
doesn't have any way of specifying that; it will just keep looping after it gets
back to the starting point until it doesn't find any more matches, and in
certain cases (e.g. replace "a" with "aa") it will just keep finding matches
forever.  The lower level nsIFind interface doesn't have that problem because it
lets us specify start and end ranges explicitly, so it's needed for ReplaceAll
while the rest of the options can use the simpler higher-level interface.

Comment 51

[Charles Manske]

Comment on attachment 74372 [details] [diff] [review]
Patch to nsFind to make it stop at the right place

r=cmanske

Comment 52

[Daniel Veditz [:dveditz]]

CC'ing kin

In js 1.5 \s matches a0, the O'Reilly book covers 1.3 I believe.

try the following URL: javascript:/\s/.test("\xa0")

Unfortunately your algorithm doesn't match the space constraints as kin
explained them in the newsgroup, where "a b   c" is allowed to match
"a   b     c" but not "a   b c". Each group of spaces are dealt with on their own.

Personally I'd bag it and count all spaces equally since it's simpler, but if
you're going to care about spaces I think you have to do it as Kin says nsFind
does it. So here's another attempt:

  specArray = specStr.match(/\S+|\s+/g);
  selArray = selStr.match(/\S+|\s+/g);
  if ( specArray.length != selArray.length)
    matches = false;
  else 
    for (i=0; i<selArray.length; i++)
    {
      if (selArray[i] != specArray[i])
      {
         if ( selArray[i][0].test(/\S/) || specArray[i][0].test(/\S/) )
         {
           // capital \S, not a space chunk -- match fails
           matches = false;
           break;
         }
         else if ( selArray[i].length < specArray[i].length )
         {
           // if it's a space chunk then we only care that sel be
           // at least as long as spec (XXX or did I reverse it?)
           matches = false;
           break;
         }
      }
    }

Comment 53

[Daniel Veditz [:dveditz]]

Comment on attachment 74372 [details] [diff] [review]
Patch to nsFind to make it stop at the right place

sr=dveditz for the nsFind patch

Comment 54

[Akkana Peck]

I tried the suggested code, but while testing it I got:
chrome://editor/content/EdReplace.js line 191: selArray[i][0].test is not a function
Line 191 is the line:
          if ( selArray[i][0].test(/\S/) || specArray[i][0].test(/\S/) )

I'm not clear under what circumstances my version will fail?  I've been testing
on a page that has three spaces, e.g. open the editor and type "Here is a page
with<sp><sp><sp>three contiguous spaces"
(or just make an html file containing that with a " &nbsp; " block in it and
edit that), then bring up the find dialog, search for "th th" (it finds the
three-space block), type a substitute pattern, then try doing replace with these
patterns:
th th    (one space, succeeds)
th   th  (three spaces, succeeds)
th    th (four spaces, fails with "not found")

I believe you that my algorithm is missing something, I'm just not sure where or
what might be a better test?  Each group of spaces should be dealt with
indepdendantly, shouldn't they?

Comment 55

[Daniel Veditz [:dveditz]]

Sorry, I blew it. test() is a RegExp method. You could, for example, do
  /\s/.test(...)

For strings there's match and search which is what I should've used, search
being faster when determining true or false because it doesn't have to copy
stuff to construct an array.

attempt 3 using search

 if ( selArray[i][0].search(/\s/) == -1 || 
      specArray[i][0].search(/\s/) == -1 )
 {
   // (lowercase \s) not a space chunk -- match fails

or using test:

  if ( /\S/.test(selArray[i][0]) || /\S/.test(specArray[i][0]) )
  {
     // (uppercase \S) not a space chunk -- match fails

Not sure which is faster/better, but the search form might be more maintainable
in the future.

Comment 56

[Akkana Peck]

Attachment: Patch with dveditz' changes

Comment 57

[Daniel Veditz [:dveditz]]

Comment on attachment 74444 [details] [diff] [review]
Patch with dveditz' changes

>+            // if it's a space chunk then we only care that sel be
>+            // at least as long as spec (XXX or did I reverse it?)

heh, the idea was for you to double check me and remove the (XXX...)
part of the comment.

sr=dveditz if you clean up that comment problem (and the uppercase one above)

Comment 58

[Charles Manske]

Comment on attachment 74444 [details] [diff] [review]
Patch with dveditz' changes

r=cmanske

Comment 59

[Asa Dotzler [:asa]]

Comment on attachment 74444 [details] [diff] [review]
Patch with dveditz' changes

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 60

[Akkana Peck]

The fix is in and enabled!  All that remains now is to remove the old find code.

That means taking the #ifdef TEXT_SVCS_TEST code out of nsWebBrowserFind,
removing the two prefs from all.js and ComposerCommands.js, and cvs removing the
following files (as well as removing them from appropriate Makefiles):
editor/txtsvc/public/nsIFindAndReplace.idl
editor/txtsvc/nsFindAndReplace.cpp
editor/txtsvc/nsFindAndReplace.h
xpfe/components/find/resources/replacedialog.xul
xpfe/components/find/resources/replacedialog.js
xpfe/components/find/resources/locale/en-US/replacedialog.dtd

Comment 61

[Akkana Peck]

Bug 126312 already covers removing the old find code; I'm going to close this
bug out and let that one cover the removal.

Should I nominate that bug for moz 1.0?

Comment 62

[sujay]

verified.