Last Comment Bug 355064 - Editor and Composer should use InlineSpellCheckUI
: Editor and Composer should use InlineSpellCheckUI
Status: RESOLVED FIXED
: fixed-seamonkey1.1.1
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-01 13:27 PDT by neil@parkwaycc.co.uk
Modified: 2008-07-31 04:23 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Composer first pass (7.88 KB, patch)
2006-10-01 13:53 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
mnyromyr: review+
Details | Diff | Review
Editor backend only first pass (4.99 KB, patch)
2006-10-01 15:05 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Review
Editor dtd changes (2.40 KB, patch)
2006-10-29 08:10 PST, neil@parkwaycc.co.uk
kairo: review+
kairo: approval‑seamonkey1.1b+
Details | Diff | Review
Added Edit/Spellcheck As You Type menuitem (5.54 KB, patch)
2006-11-05 15:43 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Review
Editor patch for check in (6.30 KB, patch)
2006-11-25 04:24 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Screenshot with the broken context menu (69.08 KB, image/png)
2006-11-25 18:42 PST, Stephen Donner [:stephend]
no flags Details
Update spellcheck menuitems last (2.46 KB, patch)
2006-11-26 03:39 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Review
Combined branch patch (6.27 KB, patch)
2006-11-26 11:18 PST, neil@parkwaycc.co.uk
kairo: approval‑seamonkey1.1+
Details | Diff | Review
Branch port of 240849 (8.46 KB, patch)
2007-01-18 15:24 PST, neil@parkwaycc.co.uk
csthomas: approval‑seamonkey1.1.1+
Details | Diff | Review
Make spellcheck dialog update toolkit inline spellchecker too (1001 bytes, patch)
2007-01-21 06:26 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Review
[fixed on trunk]convert Thunderbird to use InlineSpellCheckerUI as well (15.60 KB, patch)
2007-01-25 19:58 PST, Scott MacGregor
neil: superreview+
Details | Diff | Review
Branch-only ignore word fix (1.01 KB, patch)
2007-01-29 01:40 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
kairo: approval‑seamonkey1.1.1+
Details | Diff | Review
Updated spellcheck dialog fix (1020 bytes, patch)
2007-01-30 02:01 PST, neil@parkwaycc.co.uk
neil: review+
mscott: superreview+
Details | Diff | Review

Description neil@parkwaycc.co.uk 2006-10-01 13:27:46 PDT
Currently Composer uses InlineSpellCheck and Editor has no UI at all.
We should standardise on InlineSpellCheckUI.
Comment 1 neil@parkwaycc.co.uk 2006-10-01 13:53:06 PDT
Created attachment 240849 [details] [diff] [review]
Composer first pass

For some reason turning spell checking on or changing the language isn't consistently checking the whole document. Midas works fine, of course :-(
Comment 2 neil@parkwaycc.co.uk 2006-10-01 15:05:39 PDT
Created attachment 240853 [details] [diff] [review]
Editor backend only first pass

No spell checking toggle or language selector yet.
Comment 3 Ian Neal 2006-10-03 16:33:15 PDT
(In reply to comment #1)
> Created an attachment (id=240849) [edit]
> Composer first pass
> 
> For some reason turning spell checking on or changing the language isn't
> consistently checking the whole document. Midas works fine, of course :-(
> 
I get this on pre-patched SM too, so I do not think it's anything new, but it does seem to happen more frequently on a patched SM :-(
Comment 4 Ian Neal 2006-10-09 15:46:21 PDT
Comment on attachment 240849 [details] [diff] [review]
Composer first pass

>Index: messengercompose.xul
>===================================================================
>@@ -212,13 +212,13 @@
> 
> <popup id="sidebarPopup"/>
> 
>-<popup id="msgComposeContext" onpopupshowing="openEditorContextMenu();">
>+<popup id="msgComposeContext" onpopupshowing="openEditorContextMenu(this);">
>   <menuitem id="spellCheckNoSuggestions" label="&spellCheckNoSuggestions.label;" disabled="true"/>
>   <menuseparator id="spellCheckAddSep"/>
>   <menuitem id="spellCheckAddToDictionary" label="&spellCheckAddToDictionary.label;" accesskey="&spellCheckAddToDictionary.accesskey;"
>-            oncommand="InlineSpellChecker.addToDictionary(null,null);"/>  
>+            oncommand="InlineSpellCheckerUI.addToDictionary();"/>  
Nit: Could you remove the trailing spaces at the end of the line above?
>   <menuitem id="spellCheckIgnoreWord" label="&spellCheckIgnoreWord.label;" accesskey="&spellCheckIgnoreWord.accesskey;"
>-            oncommand="InlineSpellChecker.ignoreWord(null, null)"/>  
>+            oncommand="InlineSpellCheckerUI.ignoreWord()"/>  
Nit: Trailing spaces again and there is no semi-colon.

>@@ -343,7 +343,7 @@
>           <menuitem label="&checkSpellingCmd.label;" id="menu_checkspelling" accesskey="&checkSpellingCmd.accesskey;" key="key_checkspelling" command="cmd_spelling"/>
>           <menuitem label="&enableInlineSpellChecker.label;" id="menu_inlineSpellCheck"
>                     accesskey="&enableInlineSpellChecker.accesskey;" type="checkbox"
>-                    oncommand="ToggleInlineSpellChecker(event.target)"/>
>+                    oncommand="InlineSpellCheckerUI.enabled = !InlineSpellCheckerUI.enabled"/>
Nit: Again there is no semi-colon.

>Index: MsgComposeCommands.js
>===================================================================
>@@ -2098,24 +2098,9 @@ function addRecipientsToIgnoreList(aAddr
>         tokenizedNames = tokenizedNames.concat(splitNames);
>     }
> 
>-    InlineSpellChecker.inlineSpellChecker.ignoreWords(tokenizedNames, tokenizedNames.length);
>+    InlineSpellCheckerUI.mInlineSpellChecker.ignoreWords(tokenizedNames, tokenizedNames.length);
>   }
> }
>-
>-function StopInlineSpellChecker()
>-{
>-  if (InlineSpellChecker.inlineSpellChecker)
>-    InlineSpellChecker.editor.setSpellcheckUserOverride(false);
>-}
>-
>-function ToggleInlineSpellChecker(target)
>-{
>-  if (InlineSpellChecker.inlineSpellChecker)
>-  {
>-    InlineSpellChecker.editor.setSpellcheckUserOverride(!InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell);
>-
>-    if (InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell)
>-      InlineSpellChecker.checkDocument(window.content.document);
>   }
> }
Why are these extra brackets here?

r=me with those things fixed.
Comment 5 neil@parkwaycc.co.uk 2006-10-29 08:10:31 PST
Created attachment 243986 [details] [diff] [review]
Editor dtd changes

Includes menuitem and pref panel item. There are no UI changes for mailnews.
Comment 6 Robert Kaiser (not working on stability any more) 2006-10-29 09:44:37 PST
Comment on attachment 243986 [details] [diff] [review]
Editor dtd changes

Looks good to me, also a=me for those locale changes if this whole patch should still go into 1.1 (1.1b is freeze for 1.1 locale strings)
Comment 7 Ian Neal 2006-10-29 16:29:27 PST
Comment on attachment 240853 [details] [diff] [review]
Editor backend only first pass

>Index: composer/content/EditorContextMenuOverlay.xul
>===================================================================
>@@ -45,10 +45,23 @@
> 
> <script type="application/x-javascript" src="chrome://editor/content/EditorContextMenu.js"/>
> <script type="application/x-javascript" src="chrome://editor/content/StructBarContextMenu.js"/>
>+<script type="application/x-javascript" src="chrome://global/content/inlineSpellCheckUI.js"/>
> 
> <popupset id="editorContentContextSet">
>  <popup id="editorContentContext"   
>    onpopupshowing="EditorFillContextMenu(event, this);"> 
>+    <menuitem id="spellCheckNoSuggestions" label="&spellCheckNoSuggestions.label;" disabled="true"/>
>+    <menuseparator id="spellCheckAddSep"/>
>+    <menuitem id="spellCheckAddToDictionary"
>+              label="&spellCheckAddToDictionary.label;"
>+              accesskey="&spellCheckAddToDictionary.accesskey;"
>+              oncommand="InlineSpellCheckerUI.addToDictionary();"/>  
Nit: Extra spaces at the end of the line above should be removed.

>+    <menuitem id="spellCheckIgnoreWord"
>+              label="&spellCheckIgnoreWord.label;"
>+              accesskey="&spellCheckIgnoreWord.accesskey;"
>+              oncommand="InlineSpellCheckerUI.ignoreWord()"/>  
Nit: Extra spaces at the end of the line above should be removed and missing a semi-colon.
Comment 8 neil@parkwaycc.co.uk 2006-11-05 15:43:04 PST
Created attachment 244774 [details] [diff] [review]
Added Edit/Spellcheck As You Type menuitem

As glazou mentioned, spellchecking a large document can be quite slow, so spellchecking always default to off when opening or when rebuilding source.

Should I turn spellchecking off when you switch to preview mode?
Comment 9 neil@parkwaycc.co.uk 2006-11-05 15:43:59 PST
Comment on attachment 244774 [details] [diff] [review]
Added Edit/Spellcheck As You Type menuitem

Due to bugzilla dataloss, please read the previous comment.
Comment 10 Ian Neal 2006-11-23 15:52:59 PST
Comment on attachment 244774 [details] [diff] [review]
Added Edit/Spellcheck As You Type menuitem

>Index: editorOverlay.xul
>===================================================================
>@@ -408,6 +408,10 @@
>       <menuitem id="menu_checkspelling" accesskey="&editcheckspelling.accesskey;"
>                 key="checkspellingkb"   observes="cmd_spelling" disabled="true"
>                 label="&checkSpellingCmd.label;"/>
>+      <menuitem id="menu_inlinespellcheck" type="checkbox"
>+                label="&enableInlineSpellChecker.label;"
>+                accesskey="&enableInlineSpellChecker.accesskey;"
>+                oncommand="InlineSpellCheckerUI.enabled = !InlineSpellCheckerUI.enabled"/>
Nit: Missing ;
r=me
Comment 11 Ian Neal 2006-11-23 15:55:13 PST
(In reply to comment #8)
> Created an attachment (id=244774) [edit]
> Added Edit/Spellcheck As You Type menuitem
> 
> As glazou mentioned, spellchecking a large document can be quite slow, so
> spellchecking always default to off when opening or when rebuilding source.
> 
> Should I turn spellchecking off when you switch to preview mode?
I would say on both HTML source and Preview modes.

Comment 12 neil@parkwaycc.co.uk 2006-11-25 04:24:44 PST
Created attachment 246520 [details] [diff] [review]
Editor patch for check in
Comment 13 Karsten Düsterloh 2006-11-25 09:11:57 PST
Comment on attachment 240849 [details] [diff] [review]
Composer first pass

Applying this patch onto Mac trunk, I get some rather large offsets (>20 lines) in the latter half - and a broken messengercompose window, I can't type anything. Since the patch was made with a rather small context, I'd suggest using -u8 or something...

CommandUpdate_MsgCompose is not defined
Source File: chrome://messenger/content/messengercompose/messengercompose.xul
Line: 1
Error: ComposeLoad is not defined
Source File: chrome://messenger/content/messengercompose/messengercompose.xul
Line: 1
Error: gMsgCompose is not defined
Source File: chrome://messenger-smime/content/msgCompSMIMEOverlay.js
Line: 72
Error: updateOptionItems is not defined
Source File: chrome://messenger/content/messengercompose/messengercompose.xul
Line: 1
Comment 14 Stephen Donner [:stephend] 2006-11-25 18:42:16 PST
Created attachment 246582 [details]
Screenshot with the broken context menu
Comment 15 Stephen Donner [:stephend] 2006-11-25 18:43:43 PST
(In reply to comment #14)
> Created an attachment (id=246582) [edit]
> Screenshot with the broken context menu

So, there's no real way to do a "Replace with suggested word"; likely that extra separator portends the missing menu items...

Build 2006-11-25-08 SeaMonkey trunk under Windows XP. 

Comment 16 Stephen Donner [:stephend] 2006-11-25 18:54:01 PST
(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=246582) [edit]
> > Screenshot with the broken context menu
> 
> So, there's no real way to do a "Replace with suggested word"; likely that
> extra separator portends the missing menu items...
> 
> Build 2006-11-25-08 SeaMonkey trunk under Windows XP. 

Sorry for the prolonged spam ;-(

It's not actually broken; when I was testing it I assumed it was on by default, but it's not.  Hence, the broken context menu is only when Spellcheck As You Type is off.

Comment 17 neil@parkwaycc.co.uk 2006-11-26 03:39:44 PST
Created attachment 246601 [details] [diff] [review]
Update spellcheck menuitems last

An editor context menu hack was conflicting with the spellcheck code.
Unfortunately my dev tree has an (unreviewed) fix for that hack...
Comment 18 neil@parkwaycc.co.uk 2006-11-26 11:18:01 PST
Created attachment 246620 [details] [diff] [review]
Combined branch patch

editor.js has slightly different context.
Comment 19 Robert Kaiser (not working on stability any more) 2006-11-27 03:37:42 PST
Comment on attachment 246620 [details] [diff] [review]
Combined branch patch

a=me for SeaMonkey 1.1
Comment 20 Andrew Schultz 2006-12-13 09:19:26 PST
did this land on the branch?  Is there more work to do for trunk/branch?
Comment 21 Ian Neal 2006-12-21 18:23:43 PST
(In reply to comment #20)
> did this land on the branch?  Is there more work to do for trunk/branch?
> 
I think it did land on the branch 2006-11-27 03:55

Neil might need to set fixed-seamonkey keyword though
Comment 22 Karsten Düsterloh 2007-01-17 14:14:51 PST
Comment on attachment 240849 [details] [diff] [review]
Composer first pass

I checked all hunks and they were applied at the intended position. But: All  issues noted in comment 13 still hold. :(
Comment 23 Karsten Düsterloh 2007-01-17 15:37:30 PST
Comment on attachment 240849 [details] [diff] [review]
Composer first pass

Okay, I found the problem:

>-
>-function ToggleInlineSpellChecker(target)
>-{
>-  if (InlineSpellChecker.inlineSpellChecker)
>-  {
>-    InlineSpellChecker.editor.setSpellcheckUserOverride(!InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell);
>-
>-    if (InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell)
>-      InlineSpellChecker.checkDocument(window.content.document);
>   }
> }

The last two lines are breaking the compose window, they have to go, too.


Assuming that fixed, two minor nits:

>-                    oncommand="ToggleInlineSpellChecker(event.target)"/>
>+                    oncommand="InlineSpellCheckerUI.enabled = !InlineSpellCheckerUI.enabled"/>

How about 
  InlineSpellCheckerUI.enabled ^= true;

>+  InlineSpellCheckerUI.enabled = sPrefs.getBoolPref("mail.spellcheck.inline");

Use '' instead of "" for consistency's sake.


r=me given the destructive braces are killed.


BTW: 
After opening the spellcheck dialog (^K), I have to click Close twice and get this error for the first try:
Error: window.opener.InlineSpellChecker.inlineSpellChecker has no properties
Source File: chrome://editor/content/EdSpellCheck.js
Line: 597
Comment 24 neil@parkwaycc.co.uk 2007-01-18 15:24:44 PST
Created attachment 251976 [details] [diff] [review]
Branch port of 240849
Comment 25 neil@parkwaycc.co.uk 2007-01-21 06:26:58 PST
Created attachment 252210 [details] [diff] [review]
Make spellcheck dialog update toolkit inline spellchecker too

Since we're hoping to unfork EdSpellCheck.js we can do one of two things:
a) support both the old editorInlineSpellCheck.js and the new toolkit
b) wait for Thunderbird to switch to toolkit and remove the old spellchecker
This patch implements (a).
Comment 26 Scott MacGregor 2007-01-22 21:43:55 PST
Neil, I'd be interested  in working on option (b) if you have more details. Are there two interfaces for accessing the inline spell checker? InlineSpellChecker and InlineSpellCheckerUI? And Thunderbird is using the wrong one?
Comment 27 neil@parkwaycc.co.uk 2007-01-23 05:44:42 PST
Scott, you may remember InlineSpellChecker was part of the original inline spell checker. However when Google implemented inline spell checking for web pages they created their own spell check UI module. SeaMonkey has since switched to using this module for all our spell checking needs, including web composer.

The API for the two spell checker UI modules is different, of course. Here are some examples:
InlineSpellChecker.inlineSpellChecker.ignoreWords becomes
InlineSpellCheckerUI.mInlineSpellChecker.ignoreWords
InlineSpellChecker.checkDocument(window.content.document); becomes
InlineSpellCheckerUI.mInlineSpellChecker.spellCheckRange(null);
if (InlineSpellChecker.inlineSpellChecker && InlineSpellChecker.inlineSpellChecker.enableRealTimeSpell) becomes
if (InlineSpellCheckerUI.enabled)

Of course, if you have any issues with the toolkit inline spell checker UI then feel free to contact pkasting who I believe is the current owner.
Comment 28 Scott MacGregor 2007-01-25 19:58:20 PST
Created attachment 252865 [details] [diff] [review]
[fixed on trunk]convert Thunderbird to use InlineSpellCheckerUI as well

I need to test this a little more, but this patch basically ports Neil's seamonkey changes to Thunderbird to use InlineSpellCheckerUI. As a result, no one else in the tree is using the now obsolete, InlineSpellChecker.js so I cvs removed that file as well.
Comment 29 neil@parkwaycc.co.uk 2007-01-26 10:01:17 PST
Comment on attachment 252865 [details] [diff] [review]
[fixed on trunk]convert Thunderbird to use InlineSpellCheckerUI as well

Looks OK to me. Thanks for cleaning up ;-)
Comment 30 Scott MacGregor 2007-01-26 12:40:43 PST
Comment on attachment 252865 [details] [diff] [review]
[fixed on trunk]convert Thunderbird to use InlineSpellCheckerUI as well

I've checked this patch into the trunk.
Comment 31 Ian Neal 2007-01-26 15:31:10 PST
Comment on attachment 252210 [details] [diff] [review]
Make spellcheck dialog update toolkit inline spellchecker too

r=me assuming you make necessary changes now mscott has checked in the switch for TB.
Comment 32 Scott MacGregor 2007-01-28 21:12:37 PST
Comment on attachment 251976 [details] [diff] [review]
Branch port of 240849

Neil, doesn't this patch for the branch depend on Bug 354580 landing on the branch too?
Comment 33 neil@parkwaycc.co.uk 2007-01-29 01:39:32 PST
Yes, it does :-( Patch for the ignore word menuitem coming up.
(The ignore author feature is unaffected.)
Comment 34 neil@parkwaycc.co.uk 2007-01-29 01:40:57 PST
Created attachment 253150 [details] [diff] [review]
Branch-only ignore word fix
Comment 35 Scott MacGregor 2007-01-29 21:19:42 PST
alternatively, couldn't we port Ian's original change to add ignore word support to InlineSpellCheckerUI as well?
Comment 36 Scott MacGregor 2007-01-29 21:42:25 PST
Comment on attachment 252865 [details] [diff] [review]
[fixed on trunk]convert Thunderbird to use InlineSpellCheckerUI as well

I put this patch (minus the changes to editor to remove editorInlineSpellCheck.js) on the branch too.
Comment 37 neil@parkwaycc.co.uk 2007-01-30 01:50:55 PST
(In reply to comment #35)
>alternatively, couldn't we port Ian's original change to add ignore word
>support to InlineSpellCheckerUI as well?
IanN could certainly try to port his backend changes (obviously he can't add strings to the Firefox UI at this stage).
Comment 38 neil@parkwaycc.co.uk 2007-01-30 02:01:31 PST
Created attachment 253308 [details] [diff] [review]
Updated spellcheck dialog fix

This is the last remaining reference to the old InlineSpellCheck.

Transferring IanN's review from attachment 252210 [details] [diff] [review].
Comment 39 Scott MacGregor 2007-01-30 08:34:59 PST
Comment on attachment 253308 [details] [diff] [review]
Updated spellcheck dialog fix

Neil, should this go into the branch too?
Comment 40 neil@parkwaycc.co.uk 2007-02-14 05:51:34 PST
Fix checked in to the branch, including the ignore word and suggestions fixes.
Comment 41 Alan 2007-04-11 18:51:51 PDT
Big thanks to all who worked on this.  Great to finally have spell checking throughout Mozilla, however I do have one small issue.  

How come Spelling was placed in the already crowded Edit menu rather than in the relatively underpopulated tools menu?  Why was it given the shortcut Ctrl+K rather than F7 a shortcut that millions of users have come to expect from products such as Microsoft Word, OpenOffice.org, and others?  F7 is a shortcut not otherwise in use in Mozilla and adding to the confusion Ctrl+K might be familiar to Firefox users as the shortcut to jump into the search box, and it would seem like it was chosen largely because it was available.  

I would have filed a separate request to change it but before doing that I must know the reason why it was done this way, there must be a reason?  (and I will file a request in due course if appropriate)  

I can understand the small possible ergonomics benefit of using a letter like K rather than a more awkward to reach Function key like F7 but the usability and consistency disadvantage of doing things differently from so many other programs should more than outweigh that concern.  



Comment 42 neil@parkwaycc.co.uk 2007-04-12 11:37:48 PDT
(In reply to comment #41)
>How come Spelling was placed in the already crowded Edit menu rather than in
>the relatively underpopulated tools menu?
I don't know. Moving it to the tools menu sounds reasonable to me.

>Why was it given the shortcut Ctrl+K rather than F7
Because that's what thousands of former Netscape users have come to expect.

>F7 is a shortcut not otherwise in use in Mozilla
But it is on use on the Mac...
Comment 43 Alan 2007-04-12 14:50:18 PDT
(In reply to comment #42)
> (In reply to comment #41)
> >How come Spelling was placed in the already crowded Edit menu rather than in
> >the relatively underpopulated tools menu?
> I don't know. Moving it to the tools menu sounds reasonable to me.
> 
> >Why was it given the shortcut Ctrl+K rather than F7
> Because that's what thousands of former Netscape users have come to expect.

Ah, right.  I'm a big fan of consistency.  Hard to know if the change is worth it (and I wish we could probably test the theory) but I would suspect those longterm Netscape users would have a significant overlap with users of Office products which use F7 as a shortcut for spellchecking.  

(For what it is worth Wordperfect uses Ctrl+F1.  The use of F7 does seem to stem from Microsoft Office and those who follow them.)

> >F7 is a shortcut not otherwise in use in Mozilla
> But it is on use on the Mac...

Hmm, Mac seems like a significant problem.  

Thanks, request filed 
https://bugzilla.mozilla.org/show_bug.cgi?id=377328


Note You need to log in before you can comment on or make changes to this bug.