RFE: Cannot spellcheck selection

RESOLVED FIXED in mozilla1.4beta

Status

()

Core
Editor
P4
enhancement
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: bobj, Assigned: kinmoz)

Tracking

Trunk
mozilla1.4beta
x86
Windows 95
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spellcheck])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

17 years ago
I swear we had a bug on this, but can't seem to locate it, setting to future.
Assignee: beppe → kin
Severity: normal → enhancement
Priority: -- → P4
Summary: Cannot spellcheck selection → RFE: Cannot spellcheck selection
Whiteboard: [spell check]
Target Milestone: --- → Future

Updated

17 years ago
Whiteboard: [spell check] → [spellcheck]

Comment 2

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

Comment 3

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

Updated

16 years ago
Blocks: 119232
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.3beta
(Assignee)

Comment 4

15 years ago
Created attachment 111583 [details] [diff] [review]
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.
(Assignee)

Comment 5

15 years ago
Created attachment 112184 [details] [diff] [review]
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.
Attachment #111583 - Attachment is obsolete: true
(Assignee)

Comment 6

15 years ago
Created attachment 112458 [details] [diff] [review]
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.
Attachment #112184 - Attachment is obsolete: true
(Assignee)

Comment 7

15 years ago
Created attachment 112929 [details] [diff] [review]
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.
Attachment #112458 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #112929 - Flags: review?(jfrancis)
(Assignee)

Updated

15 years ago
Target Milestone: mozilla1.3beta → mozilla1.4alpha

Comment 8

15 years ago
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.
Attachment #112929 - Flags: review?(jfrancis) → review+
(Assignee)

Comment 9

15 years ago
> 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.
(Assignee)

Updated

15 years ago
Attachment #112929 - Flags: superreview?(sfraser)
(Assignee)

Comment 10

15 years ago
Created attachment 113639 [details] [diff] [review]
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.
Attachment #112929 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #113639 - Flags: superreview?(sfraser)
(Assignee)

Updated

15 years ago
Attachment #112929 - Flags: superreview?(sfraser)

Updated

15 years ago
Attachment #113639 - Flags: superreview?(sfraser) → superreview+
(Assignee)

Comment 11

15 years ago
Created attachment 114954 [details] [diff] [review]
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);
+  }
+
Attachment #113639 - Attachment is obsolete: true

Updated

15 years ago
Attachment #114954 - Flags: superreview+
Attachment #114954 - Flags: review+
(Assignee)

Comment 12

15 years ago
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.
(Assignee)

Updated

15 years ago
Target Milestone: mozilla1.4alpha → mozilla1.4beta
(Assignee)

Comment 13

15 years ago
Created attachment 120243 [details] [diff] [review]
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.
(Assignee)

Comment 14

15 years ago
Created attachment 120244 [details] [diff] [review]
Patch Rev 1.3 (txtsvc changes from Patch Rev 1.2 + Mods from attachment 120243 [details] [diff] [review] above)
(Assignee)

Updated

15 years ago
Attachment #120244 - Flags: superreview?(sfraser)
Attachment #120244 - Flags: review?(brade)

Updated

15 years ago
Attachment #120244 - Flags: superreview?(sfraser) → superreview+

Updated

15 years ago
Attachment #120244 - Flags: review?(brade) → review+
(Assignee)

Comment 15

15 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.