Currently Composer uses InlineSpellCheck and Editor has no UI at all. We should standardise on InlineSpellCheckUI.

For some reason turning spell checking on or changing the language isn't consistently checking the whole document. Midas works fine, of course :-(

No spell checking toggle or language selector yet.

(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 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.

Includes menuitem and pref panel item. There are no UI changes for mailnews.

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 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.

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 on attachment 244774 [details] [diff] [review] Added Edit/Spellcheck As You Type menuitem Due to bugzilla dataloss, please read the previous comment.

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

(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 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

(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.

(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.

An editor context menu hack was conflicting with the spellcheck code. Unfortunately my dev tree has an (unreviewed) fix for that hack...

editor.js has slightly different context.

Comment on attachment 246620 [details] [diff] [review] Combined branch patch a=me for SeaMonkey 1.1

did this land on the branch? Is there more work to do for trunk/branch?

(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 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 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

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).

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?

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.

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 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 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 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 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?

Yes, it does :-( Patch for the ignore word menuitem coming up. (The ignore author feature is unaffected.)

alternatively, couldn't we port Ian's original change to add ignore word support to InlineSpellCheckerUI as well?

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.

(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).

This is the last remaining reference to the old InlineSpellCheck. Transferring IanN's review from attachment 252210 [details] [diff] [review].

Comment on attachment 253308 [details] [diff] [review] Updated spellcheck dialog fix Neil, should this go into the branch too?

Fix checked in to the branch, including the ignore word and suggestions fixes.

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.

(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...

(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