91131 - RFE: Cannot spellcheck selection

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P4)

Description

[bobj]

In Netscape 4.x, running spellcheck would just check any active selection.
Mozilla ignores the selection and spellchecks the entire document.

Comment 1

[rubydoo123]

I swear we had a bug on this, but can't seem to locate it, setting to future.

Comment 2

[jglick]

Shouldn't this be bugscape since its netscape only? Bugscape 8694 was marked as 
a dup of this bug.

Comment 3

[Kathleen :Brade]

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

Comment 4

[kinmoz]

Attachment: Work in progress patch.

Just stashing my work-in-progress here so I don't lose it. This patch actually
works, but needs some work to maintain the original range bounds being checked
so that callers can re-set the selection to the area iterated over when all is
done. It also needs some work on the editor/composer side to make sure that the
range used actually begins/ends on word boundaries.

Comment 5

[kinmoz]

Attachment: Work in progress patch. (01/16/03)

More work in progress. This patch tries to minimize the number of nodes that
can disappear out from under the text services by performing inserts *before*
deleting what is currently selected.

Comment 6

[kinmoz]

Attachment: Work in progress patch (01/23/03)

It was decided to punt on implementing the code to track the range bounds while
editing. We want to use the code that is currently private to the editor
anyways to get a more accurate result. We can add that feature when this
mechanism is exposed by the editor.

This version strips out the work in progress for DOM point tracking, and relies
on DOM gravity to tell us when we need to create a new content iterator. This
patch is fully functional, but spellchecks the selection range as is. Next up
is to add code to adjust the selection range for word boundaries.

Comment 7

[kinmoz]

Attachment: Patch Rev 1

Ok, here's patch rev 1 ... If there is a collapsed selection (caret) when the
spellchecker is invoked, it spellchecks the entire document. If there is an
un-collapsed selection, it expands the selection to include whole words and
then spellchecks just the selection.

Comment 8

[Joe Francis]

Comment on attachment 112929 [details] [diff] [review]
Patch Rev 1

review notes:

nsEditorSpellCheck.cpp:
So we are punting on spellchecking multi-cell table cell selections?  Seems 
like all you would need to do change SetRangebounds to be AddRangeBounds, and
when an internal iter is done you move on to the next one.

nsITextServicesDocument.h
fix comments for SetRangeBounds: it mentions a non-existant param.

nsTextServiceDocument.cpp
not new in patch, but nonetheless i wonder what is up with the
LOCK_DOC/UNLOCK_DOC
macors scrinkled around?  The macros are null.	Is this something on the to-do
list somewhere?  I would recommend using a little stack based class, so that
you
don't have to remember to put UNLOCK_DOC at every return statement. 

I did a build of this but I dont know how to install a spellchecker on 
mozilla for macosx (is there even one?).  
FirstTextNodeInCurrentBlock() can leave the iter unmoved if it doesn't
find a text node, and I'm not sure that it's callers will properly deal 
with this.  I guess this isn't a problem for the spellchecker, given
our discussions.

Fix this comment in FindWordBounds():
	// We've found are start entry, but if we're not looking
	// for end entries, we're done.

Comment 9

[kinmoz]

> nsEditorSpellCheck.cpp:
> So we are punting on spellchecking multi-cell table cell selections?  Seems 
> like all you would need to do change SetRangebounds to be AddRangeBounds, and
> when an internal iter is done you move on to the next one.


Yeah handling multiple ranges would be nice, but I'm hesitant to add support for
it during this round given current time constraints and the fact that it could
add other issues to deal with.

I'm debating whether or not to just rename the method as you suggest, but that
would be misleading since the backend doesn't support multiple ranges now. Also
what would GetRangeBounds() return? An array of ranges?


> nsITextServicesDocument.h
> fix comments for SetRangeBounds: it mentions a non-existant param.


Fixed.


> nsTextServiceDocument.cpp
> not new in patch, but nonetheless i wonder what is up with the
> LOCK_DOC/UNLOCK_DOC
> macors scrinkled around?  The macros are null. Is this something on the to-do
> list somewhere?  I would recommend using a little stack based class, so that
> you don't have to remember to put UNLOCK_DOC at every return statement. 


This was an old coding habit, I was trying to keep in mind minimal "critical"
sections that needed to be protected if we wanted to make this thread safe, but
I think the fact that the textservices document is stateful means it's not ready
to be used by more than one thread simultaneously.


> I did a build of this but I dont know how to install a spellchecker on 
> mozilla for macosx (is there even one?).  


You have to build the commercial tree so that the  spellchecker picks up the 
api changes to the textservices.


> FirstTextNodeInCurrentBlock() can leave the iter unmoved if it doesn't
> find a text node, and I'm not sure that it's callers will properly deal 
> with this.  I guess this isn't a problem for the spellchecker, given
> our discussions.


Actually the iterator will be moved or in the "IsDone" state if it never finds a
text node ... that said, things should work ok.


> Fix this comment in FindWordBounds():
> 	// We've found are start entry, but if we're not looking
> 	// for end entries, we're done.

Fixed.

Comment 10

[kinmoz]

Attachment: Patch Rev 1.1

This should address both jfrancis and sfraser's comments.

Here are the issues I addressed for sfraser:

--- Rename AlignRangeOnWordBounds() to ExpandRangeToWordBoundaries()

--- Rename SetRangeBounds() to SetExtent()

--- Add comment to SetRangeBounds in header file that it says what happens if
it isn't called. And what would happen if there is a null range.

--- Add a comment to AlignRangeOnWordBounds() that says it doesn't change any
internal state.

--- Rename mRangeBounds to mExtent.

--- Add comment for GetRangeBounds() that tells what it will return if
SetRangeBounds() was never called.

--- Add assertion for this case in AlignRangeOnWordBounds():

+    // We should never get here because a first text block
+    // was found above.


--- Avoid extra ClearOffsetTable in both places:


+  result = FindWordBounds(&offsetTable, &blockStr, rngStartNode,
rngStartOffset,
+			   getter_AddRefs(wordStartNode), &wordStartOffset,
+			   getter_AddRefs(wordEndNode), &wordEndOffset);
+
+  if (NS_FAILED(result))
+  {
+    ClearOffsetTable(&offsetTable);
+    return result;
+  }
+
+  rngStartNode = wordStartNode;
+  rngStartOffset = wordStartOffset;
+
+  ClearOffsetTable(&offsetTable);


--- Make First/LastTextNode static if possible.

--- In LastBlock change return of error return of result:


   if (NS_FAILED(result))
   {
     UNLOCK_DOC(this);
-    return result;
+    return NS_ERROR_FAILURE;
   }


--- Reduce this:


  result = mEditor->EndTransaction();

  if (NS_FAILED(result))
  {
    UNLOCK_DOC(this);
    return result;
  }

  UNLOCK_DOC(this);

  return result;

--- Get rid of ensure in First/LastTextNode()

--- In GetWordBreaker use nsString() for arg instead of wbarg.


+    nsAutoString wbarg;
+    result = wbf->GetBreaker(wbarg, aResult);


--- Check if GetWordBreaker does a double add ref.


--- Make this use <= lastIndex:


+  PRInt32 i, lastIndex = aOffsetTable->Count() - 1;
+
+  for (i=0; i < aOffsetTable->Count(); i++)


-- Make FindwordBounds init it's out params.

Comment 11

[kinmoz]

Attachment: Patch Rev 1.2 (Same as Patch Rev 1.1 with Mac/Linux build fix)

This patch is identical to Patch Rev 1.1 except that I had to revert back to
using a temp nsString when calling GetBreaker() to avoid build errors on Mac
and Linux.


+  if (NS_SUCCEEDED(result) && wbf)
+  {
+    nsString wbarg;
+    result = wbf->GetBreaker(wbarg, aResult);
+  }
+

Comment 12

[kinmoz]

Looks like I'll have to make sure that mail compose has a way of turning of
spellcheck to selection when the pref spellcheck on send is set ... just in case
a selection exists at the time of send.

Comment 13

[kinmoz]

Attachment: Modifications to Patch Rev 1.2 above.

In the interest of trying to get some quick reviews, I'm posting this
intermediate diff that	shows just what I changes I made in addition to Patch
Rev 1.2 to keep spellcheck on send working for MailCompose.

This patch adds changes to:

  editor/composer/src/nsEditorSpellCheck.cpp
  editor/idl/nsIEditorSpellCheck.idl
  editor/ui/composer/content/ComposerCommands.js
  editor/ui/dialogs/content/EdSpellCheck.js
  mailnews/compose/resources/content/MsgComposeCommands.js

I didn't make any changes to any of the txtsvc files so reviewers can look at
this diff to review just the new stuff.

I'm posting Patch Rev 1.3 right after this which will include *all* changes.

Comment 14

[kinmoz]

Attachment: Patch Rev 1.3 (txtsvc changes from Patch Rev 1.2 + Mods from attachment 120243 above)

Comment 15

[kinmoz]

Patch Rev 1.3 checked into the TRUNK:

  editor/composer/src/nsEditorSpellCheck.cpp                 revision 1.6
  editor/idl/nsIEditorSpellCheck.idl                         revision 1.11
  editor/txtsvc/public/nsITextServicesDocument.h             revision 1.12
  editor/txtsvc/src/Makefile.in                              revision 1.32
  editor/txtsvc/src/nsTextServicesDocument.cpp               revision 1.45
  editor/txtsvc/src/nsTextServicesDocument.h                 revision 1.16
  editor/ui/composer/content/ComposerCommands.js             revision 1.184
  editor/ui/dialogs/content/EdSpellCheck.js                  revision 1.53
  mailnews/compose/resources/content/MsgComposeCommands.js   revision 1.325