User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4 We are close to having the editor used in form <textarea> tags having real time spell checking. This work has been done for Thunderbird in https://bugzilla.mozilla.org/show_bug.cgi?id=58612 so it should be pretty easy for someone acomplished in Mozilla hacking to get this going for Firefox too. Reproducible: Always Steps to Reproduce: Expected Results: When a <textarea> is displayed in a web page, spell check the words as the user types them. Perhaps tie this to an attribute on the <textarea> at first (an extension can optionally turn on this attribute for all <textarea>.

I have attached a patch that partially implements this feature.

Comment on attachment 190425 [details] [diff] [review] A patch to partially implement this feature. >--- embedding/config/basebrowser-unix 7 Jul 2005 16:01:41 -0000 1.81 >+++ embedding/config/basebrowser-unix 21 Jul 2005 03:07:55 -0000 >@@ -198,6 +198,14 @@ > ;components/nsHelperAppDlg.js > ;components/nsDownloadProgressListener.js > >+; stolen from mail/config/basemail-unix >+; spellchecker (may not be present) >+components/libmyspell.so >+components/spellchecker.xpt >+components/libspellchecker.so >+;; XXX these two files are bogus, they should not be in components/myspell... >+components/myspell/* >+ > ; psm2 > ; Optional - only if you need HTTPS support > components/libpipboot.so I don't think you need to add these to the embedding manifest, but you definitely do need to add it to: browser/installer/windows/packages-static browser/installer/unix/packages-static >--- layout/forms/nsTextControlFrame.cpp 28 Apr 2005 23:47:59 -0000 3.197 >+++ layout/forms/nsTextControlFrame.cpp 21 Jul 2005 03:07:55 -0000 >@@ -85,6 +85,9 @@ > #include "nsIPresShell.h" > #include "nsIComponentManager.h" > >+#include "nsString.h" >+#include "nsISupportsPrimitives.h" >+#include "nsIChromeRegistry.h" nsString.h is surely already being included; the other two includes here seem bogus. > #include "nsBoxLayoutState.h" > #include "nsLayoutAtoms.h" //getframetype > //for keylistener for "return" check >@@ -115,7 +118,8 @@ > #include "nsIDOMNode.h" > #include "nsITextControlElement.h" > >-#include "nsIEditorObserver.h" >+#include "nsIInlineSpellChecker.h" >+#include "nsIEditorSpellCheck.h" > #include "nsITransactionManager.h" > #include "nsIDOMText.h" //for multiline getselection > #include "nsNodeInfoManager.h" >@@ -1671,6 +1675,69 @@ > } > > nsresult >+nsTextControlFrame::InitSpellChecker() >+{ >+ nsresult rv = NS_OK; >+ >+ if (!mInlineSpellChecker) { >+ nsIInlineSpellChecker* checker; >+ NS_IMETHODIMP rv = mEditor->GetInlineSpellChecker(&checker); The variable type should just be "nsresult". NS_IMETHODIMP is only used for declaring the return type of XPCOM methods in the function definition itself. >+ NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), >+ "could not get inline spell checker"); Since builds like Minimo could very well ship without the spell checker, I'd say don't warn here since it will make debugging annoying. >+nsresult >+nsTextControlFrame::SetEnableRealTimeSpell(PRBool aToggle) aEnabled might be a clearer name for the parameter... you're not toggling the current value, you're explicitly setting whether it should be enabled. >+{ >+ nsresult rv = NS_OK; >+ >+ NS_ASSERTION(!aToggle || !IsPasswordTextControl(), >+ "don't enable real time spell for password controls"); >+ >+ if (aToggle && !mInlineSpellChecker) { >+ rv = InitSpellChecker(); >+ } >+ >+ if (NS_SUCCEEDED(rv) && mInlineSpellChecker) { >+ mInlineSpellChecker->SetEnableRealTimeSpell(aToggle); >+ } >+} You need to return a value (rv) from this method. >+ >+nsresult >+nsTextControlFrame::SyncRealTimeSpellWithAttribute() >+{ >+ nsresult rv = NS_OK; >+ PRBool enable = AttributeExists(nsHTMLAtoms::spellcheck); >+ >+#ifdef DEBUG >+ nsCOMPtr<nsIPrefBranch> prefBranch = >+ do_GetService(NS_PREFSERVICE_CONTRACTID, &rv); >+ >+ if (NS_SUCCEEDED(rv) && prefBranch) { >+ PRBool pref; >+ rv = prefBranch->GetBoolPref("spellchecker.inline-always-on", &pref); >+ if (NS_SUCCEEDED(rv)) { >+ enable = pref; >+ } >+ } >+#endif >+ >+ if (IsPasswordTextControl()) { >+ enable = PR_FALSE; >+ } You may as well just not bother checking the attribute if it's a password field. >+ >+ SetEnableRealTimeSpell(enable); >+} >+ This method needs to return a value as well. >+ >+nsresult > nsTextControlFrame::InitEditor() > { > // This method must be called during/after the text >--- layout/forms/nsTextControlFrame.h 28 Apr 2005 23:47:59 -0000 3.68 >+++ layout/forms/nsTextControlFrame.h 21 Jul 2005 03:07:55 -0000 >@@ -54,6 +54,7 @@ > class nsISupportsArray; > class nsIEditor; > class nsISelectionController; >+class nsIInlineSpellChecker; On some compilers, it's necessary to #include the header for a class if you use it with nsCOMPtr. This section of forward declarations is a little misleading since the class definitions for all of these are #included either elsewhere in this file or through other headers. Looks ok otherwise, but I'd like to see a revised patch.

not quite ready yet, but has the review comments addressed

This patch adds the suggestions to the context menu for edit controls. I removed the requirement in the previous patches that the HTML element have "spellcheck=true" and inline spellcheck all textarea elements. It seems unlikely that many people will set this attribute and it makes the feature useless. We may want to allow the user to enable or disable spellchecking in individual controls, however. This also won't work for email titles, which are usually single-line controls. Maybe we want to add the attribute back and use it as a hint for how to set the default spellcheck mode (perhaps webmail providers could set it for the subject line). There seems to be a textarea rendering bug on Linux that is exposed by this. The spellcheck selection is not always shown until you press enter or change focus, or select the text with the mouse. This may not work with Windows accelerator-key context menus (I haven't set up my Windows development machine yet).

This new patch adds more UI: The ability to change dictionaries (if you have them installed), and the ability to turn on and off spell checking for an individual field. It currently defaults textareas to spellchecking on, and single line inputs to spellchecking off. I added a "save options as default" menu item which is not currently connected to anything. We need to decide what this should do. Does it change the default for this type of control (textarea/single-line)? Or do we try something smarter where we remember this exact form field of this page? This might be overkill for now.

Comment on attachment 198469 [details] [diff] [review] Patch with more UI, dictionary selection Brett, I doubt I'm the best person to be reviewing all of these changes to mozilla/browser and layout. In any case, my trunk review queue is going to be slow as I'm focused on the branch and getting 1.5 out the door right now, so I won't get to this for a little while.

Comment on attachment 197910 [details] [diff] [review] Spellcheck patch with context menu UI clearing obsolete requests

The previous patch was missing nsEditorSpellCheck.cpp and had a bug if you right-clicked on file input boxes. This patch fixes those problems.

Comment on attachment 199373 [details] [diff] [review] Patch with minor enhancements >--- browser/base/content/browser.js 4 Oct 2005 05:02:47 -0000 1.520 >+++ browser/base/content/browser.js 13 Oct 2005 00:35:49 -0000 >@@ -4182,6 +4189,98 @@ >+ initSpellingItems : function () { >+ if (this.onMisspelling) { >+ if (this.spellSuggestions.length == 0) { >+ // no suggestions >+ this.showItem("spell-nosuggestions", 1); >+ this.showItem("spell-add-separator", 1); >+ this.showItem("spell-add", 1); >+ } else { >+ // show the suggestions >+ this.showItem("spell-nosuggestions", 0); >+ for (var i = 0; i < this.spellSuggestions.length; i ++) { >+ var item = document.getElementById("spell-sug" + i); >+ item.hidden = false; >+ item.label = this.spellSuggestions[i]; >+ } >+ this.showItem("spell-add-separator", 1); >+ this.showItem("spell-add", 1); >+ } >+ } else { >+ // not on a misspelling, hide the spelling items >+ this.showItem("spell-nosuggestions", 0); >+ this.showItem("spell-add-separator", 0); >+ this.showItem("spell-add", 0); >+ } You could factor this a little to make it more concise: var haveSuggestions = this.onMisspellings && this.spellSuggestions.length > 0; this.showItem("spell-nosuggestions", !haveSuggestions); if (haveSuggestions) { // show the suggestions } this.showItem("spell-add-separator", this.onMisspellings); this.showItem("spell-add", this.onMisspellings); >+ // now get the languages to put in the dictionary list >+ if (this.inlineSpellChecker != null && this.inlineSpellChecker.enableRealTimeSpell) { style nit: avoid "!= null" when testing whether an object exists. That's my preference, anyway, but this file is all over the map. >+ document.getElementById("spell-check-on").setAttribute("checked", "true"); >+ >+ // get the list >+ var spellchecker = this.inlineSpellChecker.spellChecker; >+ var o1 = {}; >+ var o2 = {}; >+ spellchecker.GetDictionaryList(o1, o2); >+ var list = o1.value; >+ var listcount = o2.value; >+ var curlang = spellchecker.GetCurrentDictionary(); >+ >+ var spellpopup = document.getElementById("spell-options-menu"); >+ var separator = document.getElementById("spell-language-separator"); >+ >+ this.clearSpellingLanguages(); >+ for (var i = 0; i < list.length; i ++) { >+ var item = document.createElement("menuitem"); >+ item.setAttribute("id", "spell-language-" + i); >+ item.setAttribute("label", list[i]); >+ if (curlang == list[i]) { >+ item.setAttribute("checked", "true"); >+ } else { >+ item.setAttribute("oncommand", "gContextMenu.spellSelectDictionary(\"" + list[i] + "\");"); >+ } This won't attach an oncommand handler to the item that's current when this is called... so if I switch to an alternate dictionary, I won't be able to switch back. Am I missing something? >+ spellpopup.insertBefore(item, separator); >+ } >+ this.showItem("spell-check-on-separator", 1); These should all use true/false instead of 1/0. It corresponds directly to the type of 'hidden' on nsIDOMXULElement. Also, I'd prefer here (as above) to avoid repeating the list of menuitem ids for the two cases, and instead use a variable or comparison for the parameter to showItem. >@@ -4225,8 +4324,68 @@ > // Show if user clicked on something which has metadata. > this.showItem( "context-metadata", this.onMetaDataItem ); > }, >+ initMisspelling : function (event) { >+ // Called when you right-click on an edit box to see if you are over a >+ // misspelled word. If so, this finds the suggestions for the misspelling. >+ this.onMisspelling = false; >+ this.spellSuggestions = []; >+ this.inlineSpellChecker = null; >+ this.spellRange = null; >+ >+ var uievent; >+ try { >+ uievent = event.QueryInterface(Components.interfaces.nsIDOMNSUIEvent); >+ } catch(e) { >+ // this event is not one we know how to handle >+ return; >+ } >+ You can actually do this easier in JS, and without an extra variable: if (!(event instanceof UIEvent)) { return; } then just use 'event'. >+ var editor = this.target.QueryInterface(Components.interfaces.nsIDOMNSEditableElement).editor; Here, however, you do need the QI, since nsIDOMNSEditableElement is not flattened. (just to clarify) checkpointing my review here, I'll pick it back up a little later.

Comment on attachment 199373 [details] [diff] [review] Patch with minor enhancements >--- browser/base/content/browser.js 4 Oct 2005 05:02:47 -0000 1.520 >+++ browser/base/content/browser.js 13 Oct 2005 00:35:49 -0000 >@@ -4273,9 +4436,13 @@ > this.onStandaloneImage = true; > } else if ( this.target instanceof HTMLInputElement ) { > this.onTextInput = this.isTargetATextBox(this.target); >+ if (this.target.type != "password" && >+ this.target.type != "file") >+ this.initMisspelling(event); Hm, you really want to just do this for type=text, right? You don't want to spellcheck type=checkbox/image/button/... > this.onKeywordField = this.isTargetAKeywordField(this.target); > } else if ( this.target instanceof HTMLTextAreaElement ) { > this.onTextInput = true; >+ this.initMisspelling(event); Also, for both of these, I wonder if it shouldn't skip the spell check for readonly inputs. >@@ -4636,6 +4803,41 @@ >+ spellSelectDictionary : function (langName) { >+ if (this.inlineSpellChecker == null || ! this.inlineSpellChecker.enableRealTimeSpell) >+ return; >+ this.inlineSpellChecker.spellChecker.SetCurrentDictionary(langName); >+ var kOpLoadHtml = 3013; // from extensions/spellcheck/src/mozInlineSpellChecker.h It would be awesome if you could move these constants from mozInlineSpellChecker.h into nsIInlineSpellChecker.idl. Then you could just refer to it here (Components.interfaces.nsIInlineSpellChecker.kOpLoadHtml). >--- browser/base/content/browser-context.inc 13 Aug 2005 17:36:09 -0000 1.15 >+++ browser/base/content/browser-context.inc 13 Oct 2005 00:35:49 -0000 >@@ -38,6 +38,36 @@ > <popup id="contentAreaContextMenu" > onpopupshowing="if (event.target != this) return true; gContextMenu = new nsContextMenu( this ); return gContextMenu.shouldDisplay;" > onpopuphiding="if (event.target == this) gContextMenu = null;"> >+ <menuitem id="spell-nosuggestions" >+ disabled="true" >+ label="&spellCheckNoSuggestions.label;"/> >+ <menuitem id="spell-sug0" class="spellsuggestion" >+ oncommand="gContextMenu.replaceSpellSuggestion(0);"/> >+ <menuitem id="spell-sug1" class="spellsuggestion" >+ oncommand="gContextMenu.replaceSpellSuggestion(0);"/> Isn't the parameter to replaceSpellSuggestion supposed to match up with the index of this suggestion? (same for the other items) >--- dom/public/idl/xul/nsIDOMXULDocument.idl 30 Aug 2005 21:57:01 -0000 1.9 >+++ dom/public/idl/xul/nsIDOMXULDocument.idl 13 Oct 2005 00:35:49 -0000 >@@ -46,6 +46,13 @@ > interface nsIDOMXULDocument : nsISupports > { > attribute nsIDOMNode popupNode; >+ >+ /** >+ * The event that triggered the popup. This is only valid during >+ * the popup showing event. >+ */ >+ attribute nsIDOMEvent popupEvent; >+ > attribute nsIDOMNode tooltipNode; > > readonly attribute nsIDOMXULCommandDispatcher commandDispatcher; Rev the uuid for this interface. >--- content/xul/document/src/nsXULDocument.cpp 29 Sep 2005 22:00:26 -0000 1.674 >+++ content/xul/document/src/nsXULDocument.cpp 13 Oct 2005 00:35:49 -0000 >@@ -1627,6 +1627,36 @@ > } > > NS_IMETHODIMP >+nsXULDocument::GetPopupEvent(nsIDOMEvent** aEvent) >+{ >+ nsresult rv; >+ >+ // get focus controller >+ nsCOMPtr<nsIFocusController> focusController; >+ GetFocusController(getter_AddRefs(focusController)); >+ NS_ENSURE_TRUE(focusController, NS_ERROR_FAILURE); >+ // get popup node >+ rv = focusController->GetPopupEvent(aEvent); >+ >+ return rv; >+} >+ nit: the comments don't explain anything that's not obvious from the code. And you don't need rv: return focusCOntroller->GetPopupEvent(aEvent); >+NS_IMETHODIMP >+nsXULDocument::SetPopupEvent(nsIDOMEvent* aEvent) >+{ Similar comments for this function. >--- dom/public/base/nsIFocusController.h 17 Apr 2004 21:50:08 -0000 1.12 >+++ dom/public/base/nsIFocusController.h 13 Oct 2005 00:35:49 -0000 >@@ -76,6 +76,9 @@ > NS_IMETHOD GetPopupNode(nsIDOMNode** aNode)=0; > NS_IMETHOD SetPopupNode(nsIDOMNode* aNode)=0; > >+ NS_IMETHOD GetPopupEvent(nsIDOMEvent** aEvent)=0; >+ NS_IMETHOD SetPopupEvent(nsIDOMEvent* aEvent)=0; >+ > NS_IMETHOD GetControllerForCommand(const char * aCommand, nsIController** aResult)=0; > NS_IMETHOD GetControllers(nsIControllers** aResult)=0; > Rev the uuid for this interface as well. Looks good otherwise. (We discussed the comment I made earlier about not setting oncommand for the default dictionary menuitem -- I didn't realize this happened each time the menu is opened, so it should be ok).

This patch implements bryner's suggestions. Also, it fixes an exception if the spellchecker component is not present, and read-only text boxes will not be spell checked. Note that support for keyboard-generated context menus (Shift+F10 or the context menu key next to the right-hand Windows key) requires my patch in bug 271498. I compiled & tested Thunderbird with this patch and it was fine (there is an incompatable change in the nsIEditor interface for getting the inline spellcheck object). Still incomplete: persisting the values for language and whether an item is spellchecked. This will probably have to wait for the annotation service. Perhaps we want a global pref for the default language, or a "save as default" item on the context menu? Also, the "Add dictionaries" menu item does nothing. This will probably bring up a web page with extensions containing various dictionaries. This is pending some discussion on dicionary distribution and changes in the spellchecker to find dictionaries in extension locations. You can get additional dictionaries from: http://lingucomponent.openoffice.org/spell_dic.html Put the .dic and .aff files in dist/bin/components/myspell/

Comment on attachment 200096 [details] [diff] [review] New patch >+nsresult >+nsTextControlFrame::SyncRealTimeSpellWithAttribute() minor nit: This method name made sense in the previous version of the patch that looked for a "spellcheck" attribute, but doesn't really make sense with this implementation. Rename it to just SyncRealTimeSpell? >--- layout/forms/nsTextControlFrame.h 19 Sep 2005 02:15:50 -0000 3.71 >+++ layout/forms/nsTextControlFrame.h 19 Oct 2005 16:05:48 -0000 >@@ -45,16 +45,17 @@ >+ nsresult SetEnableRealTimeSpell(PRBool aToggle); I meant to fix this variable name, but I must have missed the header. This should be aEnable. >--- editor/idl/nsIEditor.idl 1 Feb 2005 21:12:47 -0000 1.24 >+++ editor/idl/nsIEditor.idl 19 Oct 2005 16:05:48 -0000 rev the uuid for this interface Looks good otherwise, r=me with those changes.

Comment on attachment 200227 [details] [diff] [review] Patch with bryner's suggestions Thanks, I'll go ahead and check this in.

Comment on attachment 200227 [details] [diff] [review] Patch with bryner's suggestions I won't be able to sr most of this in the near future (at least a week), but just for the layout parts: >Index: layout/forms/nsTextControlFrame.cpp >+nsTextControlFrame::SetEnableRealTimeSpell(PRBool aEnabled) >+ PRBool autoCreate = PR_FALSE; >+ if (aEnabled) >+ autoCreate = PR_TRUE; PRBool autoCreate = aEnabled; would do the same thing. Which raises the question of why you even need the autoCreate boolean. Just pass aEnabled to GetInlineSpellChecker. >+ rv = mEditor->GetInlineSpellChecker(autoCreate, >+ getter_AddRefs(inlineSpellChecker)); You need to null-check mEditor here. Or if that's never needed remove the corresponding null-checks from some of its callers, since they would not be needed there either. >+nsTextControlFrame::SyncRealTimeSpell() >+ PRBool enable; >+ if (readOnly) { >+ enable = PR_FALSE; >+ } else if (IsSingleLineTextControl()) { >+ enable = PR_FALSE; >+ } else { >+ enable = PR_TRUE; >+ } PRBool enable = !readOnly && !IsSingleLineTextControl(); It looks like the spellchecker is enabled if it exists for all Gecko consumers (in nsTextFrame). But you're only adding UI in Firefox. So for other Gecko consumers, what will happen? Will the enabling there actually do nothing (that is, is the Firefox UI is needed to enable this inline spell-checking)? Or will every single other Gecko consumer be stuck with inline spell-checking in all textareas with no user ability to turn it off? If it's the latter, I don't think that's acceptable; this should default to off in apps that don't provide reasonable UI to interact with the spellchecker, imo. Or the UI should move into the core somehow.

s/nsTextFrame/nsTextControlFrame/ in that last comment.

Also, it'd be really nice to do the core changes in a Core bug and then do the Firefox UI separately, as far as I'm concerned... That lets us evaluate the two on their own merits, especially since said evaluation needs to be done by different people -- I'm really not qualified to evaluate any of the UI changes in this patch.

This patch improves the previous implementation, moving most of the UI code to a common module, and adding chrome support as per bz comment (before, you would get spellchecking in chrome textareas, but wouldn't get any UI). This adds UI to chrome, and cleans up the implementation. This also fixes the old implementation if you have no dictionaries. It does not give you the option of spellchecking if there are no dictionaries installed, and it will only check for dictionaries once per run (as opposed to once per box creation). If you don't see any spellchecking, make sure it is enabled from the context menu, and select a dictionary from the "Languages" web page. Currently, there isn't always a default language. Implementation of this is dependent on some policy decisions about dictionary distribution and localization. If you want to set a default, create a pref called "spellchecker.dictionary" and set it to your dictionary name (like "en-US").

> and adding chrome support as per bz comment (before, you would > get spellchecking in chrome textareas, but wouldn't get any UI). I'm not sure I follow that... Could you describe the behavior of the current patch in the following cases (if it doesn't change behavior at all, just say so)? Firefox, Thunderbird, Nvu, Seamonkey browser, Seamonkey mailnews, Seamonkey editor, Camino, Epiphany

> Could you describe the behavior of the current patch in the following cases > (if it doesn't change behavior at all, just say so)? Everybody: Gets spell checking in text entry boxes. You can use the spellcheck="true" element on text boxes to turn it on. Defaults to off for single line, on for multi line. The user can always turn it on or off from the context menu. The value is not currently persisted, we had some discussions about this but there are some challenges. This should probably be added in the future. Any app that doesn't have the spellchecker component or has no dictionaries will not change. When you right-click, or create any control with spellchecking enabled by default the context menu code will check for the spell checking component and dictionaries to see whether to enable the menu items. This value is cached so these are only queried once per app run. Firefox: Adds same thing for fields in web pages. Thunderbird: Keeps it's own spell checking for the compose window like it currently has (uses the same APIs). It will get the added spellchecking for any edit box.

Whoops, the previous patch forgot an updated version of browser-context.inc

Comment on attachment 201669 [details] [diff] [review] New patch that includes chrome support >--- extensions/spellcheck/src/mozInlineSpellChecker.h 1 Jun 2005 22:48:31 -0000 1.2 >+++ extensions/spellcheck/src/mozInlineSpellChecker.h 2 Nov 2005 21:06:16 -0000 >@@ -51,16 +51,20 @@ > > class nsIDOMMouseEventListener; > > class mozInlineSpellChecker : public nsIInlineSpellChecker, nsIEditActionListener, nsIDOMMouseListener, nsIDOMKeyListener, > nsSupportsWeakReference > { > private: > >+ // Access with CanEnableInlineSpellChecking >+ // -1 (uninitialized), 0 (no), 1 (yes) >+ static int gCanEnableSpellChecking; generally NSPR types are preferred, so PRInt32. Also, might be nice to use an enum for the values. I didn't look too closely at the accesskeys, but you should verify that they don't conflict with any platform-specific keyboard shortcuts. >--- editor/composer/src/nsEditorSpellCheck.cpp 1 Feb 2005 21:12:46 -0000 1.18 >+++ editor/composer/src/nsEditorSpellCheck.cpp 2 Nov 2005 21:06:16 -0000 >@@ -68,16 +68,40 @@ nsEditorSpellCheck::nsEditorSpellCheck() >+nsEditorSpellCheck::CanSpellCheck(PRBool* _retval) >+{ >+ nsresult rv; >+ nsCOMPtr<nsISpellChecker> spellChecker; >+ if (! mSpellChecker) { >+ spellChecker = do_CreateInstance(NS_SPELLCHECKER_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else { >+ spellChecker = mSpellChecker; >+ } >+ nsStringArray dictList; >+ rv = spellChecker->GetDictionaryList(&dictList); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ *_retval = dictList.Count(); PRBools should either be PR_TRUE or PR_FALSE, so maybe: *_retval = dictList.Count() != 0; >--- layout/forms/nsTextControlFrame.cpp 1 Nov 2005 20:40:54 -0000 3.204 >+++ layout/forms/nsTextControlFrame.cpp 2 Nov 2005 21:06:16 -0000 >@@ -1658,16 +1658,64 @@ nsTextControlFrame::CreateFrameFor(nsPre >+nsTextControlFrame::SetEnableRealTimeSpell(PRBool aEnabled) >+{ >+ nsresult rv = NS_OK; >+ >+ NS_ASSERTION(!aEnabled || !IsPasswordTextControl(), >+ "don't enable real time spell for password controls"); >+ >+ // The editor will lazily create the spell checker object if it has not been >+ // created. We only want one created if we are turning it on, since not >+ // created implies there's no spell checking yet. >+ PRBool autoCreate = PR_FALSE; >+ if (aEnabled) >+ autoCreate = PR_TRUE; >+ nsCOMPtr<nsIInlineSpellChecker> inlineSpellChecker; >+ rv = mEditor->GetInlineSpellChecker(autoCreate, Can't you just pass in aEnabled here, instead of creating a new variable with the same value? >--- layout/forms/nsTextControlFrame.h 28 Oct 2005 11:25:19 -0000 3.72 >+++ layout/forms/nsTextControlFrame.h 2 Nov 2005 21:06:16 -0000 >@@ -45,16 +45,17 @@ > #include "nsIAnonymousContentCreator.h" > #include "nsIEditor.h" > #include "nsITextControlFrame.h" > #include "nsFormControlHelper.h"//for the inputdimensions > #include "nsIFontMetrics.h" > #include "nsWeakReference.h" //for service and presshell pointers > #include "nsIScrollableViewProvider.h" > #include "nsIPhonetic.h" >+#include "nsIInlineSpellChecker.h" You shouldn't need this #include here (#include it from the .cpp file if necessary). >--- configure.in 2 Nov 2005 14:49:05 -0000 1.1541 >+++ configure.in 2 Nov 2005 21:06:17 -0000 >@@ -4127,17 +4127,17 @@ suite) > > browser) > MOZ_APP_NAME=firefox > MOZ_APP_DISPLAYNAME=DeerPark > MOZ_XUL_APP=1 > MOZ_UPDATER=1 > MOZ_PHOENIX=1 > MOZ_APP_VERSION=$FIREFOX_VERSION >- MOZ_EXTENSIONS_DEFAULT=" cookie xml-rpc xmlextras pref transformiix universalchardet webservices inspector gnomevfs auth permissions reporter" >+ MOZ_EXTENSIONS_DEFAULT=" cookie xml-rpc xmlextras pref transformiix universalchardet webservices inspector gnomevfs auth permissions reporter spellcheck" I think we probably don't want to do this until we're ready to start bundling spellcheck in the installer (pending meeting the download size criteria). >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/content/inlineSpellCheckUI.js 2 Nov 2005 21:06:17 -0000 >+ // returns false if there should be no spellchecking UI enabled at all, true >+ // means that you can at least give the user the ability to turn it on. >+ canSpellCheck: function() This is a perfect use case for a getter, so you can access the property without (). >+ // returns true if the current element is spellchecking >+ isEnabled: function() Same. >+ // turns on or off spellchecking according to the parameter >+ setEnabled: function(isEnabled) And perhaps a setter, here. >+ // returns true if the given event is over a misspelled word >+ overMisspelling: function() getter. >+ // this prepends up to "maxNumber" suggestions at the given menu position >+ // for the word under the cursor. Returns the number of suggestions inserted. >+ addSuggestionsToMenu: function(menu, insertBefore, maxNumber) >+ { ... >+ var callback = function(me, val) { return function(evt) { me.replaceMisspelling(val); } }; >+ item.addEventListener("command", callback(this, i), true); I'll admit to getting periodically confused about this aspect of javascript... I'm fairly confident that what you have here will invoke replcaeMisspelling with an appropriate "this" object, but not sure if there's a cleaner way. >--- toolkit/content/widgets/textbox.xml 9 Jun 2005 20:30:32 -0000 1.21 >+++ toolkit/content/widgets/textbox.xml 2 Nov 2005 21:06:17 -0000 >@@ -201,19 +249,99 @@ > var controller = document.commandDispatcher.getControllerForCommand(command); > var enabled = controller.isCommandEnabled(command); > if (enabled) > children[i].removeAttribute("disabled"); > else > children[i].setAttribute("disabled", "true"); > } > } >+ >+ // -- spell checking -- >+ >+ // try to find the outer textbox for this message, don't search Hm, what is "message" in this context? If you mean the same thing as "popup", we should stick with the same term. >+ var isenabled = spellui.isEnabled(); intercaps, for consistency (isEnabled, or just enabled) r=me with those addressed.

Comment on attachment 201693 [details] [diff] [review] Omission corrected Same comments as above.

> Everybody: Gets spell checking in text entry boxes. What does that mean? Are those different from <html:input> and <html:textarea>? If you mean <xul:textbox>, then only Firefox and Thunderbird out of my list get spellchecking in them, which is ok, I guess, if there's a followup bug to port the changes to xpfe. > Any app that doesn't have the spellchecker component or has no dictionaries > will not change. Ok. So seamonkey, for example, has a spellchecker component and dictionaries... So does Thunderbird. I don't know about Camino or embedding. Anything based on XULRunner would have spellchecker, presumably. > When you right-click, or create any control with spellchecking > enabled by default the context menu code Is this still talking about all the apps involved? I believe you changed only one out of at least 5 context menu implementations... > Firefox: Adds same thing for fields in web pages. Again, what about other Gecko consumers? That is, nsTextControlFrame seems to unconditionally enable spellcheck. Is that correct? Or am I missing something? > Thunderbird: Keeps it's own spell checking for the compose window like it > currently has (uses the same APIs). It will get the added spellchecking for > any edit box. You mean any <html:textarea>? Or any <xul:textbox>? Or both?

Could you also be sure that this is applied to Text Fields as well because that is handy in spell checking titles and so forth. Please don't just limit this to Text Areas.

(In reply to comment #24) > > Everybody: Gets spell checking in text entry boxes. > > What does that mean? Are those different from <html:input> and > <html:textarea>? If you mean <xul:textbox>, then only Firefox and Thunderbird > out of my list get spellchecking in them, which is ok, I guess, if there's a > followup bug to port the changes to xpfe. OK, I was a little confused myself here, so let me clarify: For <xul:textbox> (single line text entries in the chrome) you will get a context menu that allows you to turn on spell checking. This uses an anonymous <html:input> element, which defaults to no spellchecking. This context menu will also provide spelling suggestions, etc. For <xul:texbox multiline="true"> (multi-line text entry in the chrome) this will use an anonymous <html:textarea> which defaults to spellchecking on. This uses the same menu as above, so you can correct spelling and turn spellchecking off. If you include <html:textbox> in your XUL, you will get spellchecking but no UI. There is no context menu provided at all, so I don't think this is a problem. XUL users should use multiline textboxes. Other projects that use their own textbox bindings will have to provide their own UI (I guess this includes seamonkey?). For single-line text boxes, nothing will change. For things that use <html:textbox> for whatever reason and happen to have a spellchecker and dictionaries installed, will get spellchecking defaulted to on and will have no way to turn it off or select suggestions. My preference would be to enable something that basically works for Firefox and Thunderbird (and probably also XULRunner, it uses toolkit's textbox, right?) and to fix other cases that come up. There will be several of them, even one in Firefox: the search box provides a different context menu that includes "Clear Search History" and doesn't get the option to turn spellchecking on. It would be nice to have spellchecking on your queries. Brett

(In reply to comment #26) > If you include <html:textbox> There is no such thing. > in your XUL, you will get spellchecking but no UI. I really don't care so much what happens in XUL, actually. What I want to know is what happens if I load a webpage that has an <html:textarea> on it in a Gecko-based browser other than Firefox. If I read your patch correctly, spellchecking will be enabled for the textarea, with no way to control it or disable it.... > For things that use <html:textbox> for whatever reason and happen > to have a spellchecker and dictionaries installed, will get spellchecking > defaulted to on and will have no way to turn it off or select suggestions. Assuming you meant <html:textarea>? If you're talking about content pages using textareas, that's not acceptable as far as Gecko is concerned (at least if you're asking me). > My preference would be to enable something that basically works for Firefox > and Thunderbird (and probably also XULRunner, it uses toolkit's textbox Again, I don't care about what happens with XUL -- as you noted that can be fixed at leisure. I DO care about making basic HTML forms unusable in every Gecko-based browser but Firefox.

Surely it'd be easy just to have a global "enable spellchecking" switch that Firefox and Thunderbird can turn on. (I'm assuming we're planning to package dictionaries with xulrunner ... if not, embedders wouldn't see any problem.)

Other Gecko-based apps already do use spellchecking (eg seamonkey mailnews does). If you mean a "enable spell-checking in HTML" pref, then yes, I think that's the way to go. Default to false in the C++, set to true for Firefox (and Thunderbird? Should ask mscott what he wants as far as textareas in mail messages).

(I wrote a previous comment, but it seems to have gotten eaten...) How about a "spellcheck.defaultEnable". If it doesn't exist or is 0, then spellchecking will never be turned on by default. If it is 1, then spellchecking will be enabled by default for multiline controls (<html:textarea> and <xul:textbox multiline="true">). Only Firefox would enable this to 1. Clients that have spellchecking support but have this flag at 0 would still get the context menu for <xul:textbox> to enable spellchecking if the user desires. If, then, Thunderbird (for example) wanted to support having spellchecked textareas inside email messages (not sure why), they would write the necessary UI for the context menus and set this flag. They could also just enable it themselves. The only wrinkle would be that this flag (if implemented the easy way) would apply to both chrome and web pages. If some app has this set, they would have to supply UI for both web page textareas and <xul:textbox multiline="true"> or just not use multiline in chrome (the common case, Firefox doesn't seem to have any). This seems OK to me.

Yes, I think it's reasonable to expect an app to provide UI for both or neither. Sounds like we have a plan, then? I'd suggest a name that's a little more descripting than spellcheck.defaultEnable, since it's not really a pref the spellchecker uses but a pref that Gecko uses. Perhaps layout.textarea.spellcheckEnabled?

This patch is the same as above with bryner's suggestions and the preference that controls the default as described by bz above.

This is the frontend portion of the combined patch with bryner's suggestions incorporated.

I'll try to get to this sometime in the next week or two... I need to review Neil's templates changes first.

Comment on attachment 201877 [details] [diff] [review] Frontend patch Wouldn't it be cleaner to have a "textbox-spellcheck" binding that extends the basic xul:textbox? This adds a whole lot of code to the basic textbox binding that for many uses isn't necessary.

Comment on attachment 201876 [details] [diff] [review] Backend patch >Index: layout/forms/nsTextControlFrame.cpp >+#define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault" You only use it in one place... but I guess it can't hurt to have a define. >+nsresult >+nsTextControlFrame::SyncRealTimeSpell() No one checks the return value. Might as well make this method, and also SetEnableRealTimeSpell(), return void. >+ if (! readOnly && ! IsSingleLineTextControl()) { No space after '!', please. >+ // multi-line text control: check the pref to see what the default should be >+ nsCOMPtr<nsIPrefBranch> prefBranch = >+ do_GetService(NS_PREFSERVICE_CONTRACTID); >+ if (prefBranch) >+ prefBranch->GetBoolPref(PREF_DEFAULT_SPELLCHECK, &enable); How about: // GetBoolPref defaults the value to PR_FALSE is the pref is not set enable = nsContentUtils::GetBoolPref(PREF_DEFAULT_SPELLCHECK); instead of getting the service, etc? Should be a bit smaller and so forth, and not depend on undocumented pref service behavior as the code above does. >Index: layout/forms/nsTextControlFrame.h >+ nsresult SetEnableRealTimeSpell(PRBool aEnabled); >+ nsresult SyncRealTimeSpell(); Please document these (including what state they use, which determines when they should be called on state changes). >Index: editor/idl/nsIEditorSpellCheck.idl >+ boolean CanSpellCheck(); Unless there was an earlier review comment on this, please use camelCase for IDL methods. I know the interface had some existing methods that used UpperCase, but it also had camelCase (eg setFilter), and it's better to be consistent with usual IDL naming, in my opinion. >+ * (this controls the behavior of GetNextMisspelledWord). For inline >+ * spellchecking, we don't care about this. It's not clear how inline spellchecking is related to this interface... Perhaps just document that this argument doesn't mean anything for a spellchecker that never works with the current selection? >+ * to build a list. When there are no more suggestions, an empty string will >+ * be returned. Might want to add "(not null)" after "empty" there... Or something to make it clear it actually returns a valid PRUnichar*, just one that points to '\0'. >+ * Use when modaly checking the document to replace a word. "modally", maybe? Thanks for documenting this interface! We might want to file a followup bug on cleaning up the various misnamings, etc, for these methods... sr=bzbarsky with the above nits picked. My apologies this took so long, and many thanks for splitting the back end changes off into a separate diff!

Comment on attachment 201877 [details] [diff] [review] Frontend patch >+ // returns false if there should be no spellchecking UI enabled at all, true >+ // means that you can at least give the user the ability to turn it on. >+ canSpellCheck getter: function() Shaver tells me this getter/setter syntax is deprecated, and that you should use this syntax instead: get canSpellCheck() { ... } and set canSpellCheck(foo) { } >+ <!-- use this property "spellCheckUI" instead of "InlineSpellCheckUI", >+ because this object does the lazy creation and initialization >+ you need --> >+ <property name="spellCheckerUI" readonly="true"> nit: you say "spellCheckUI" in your comment but the name is "spellCheckerUI" >+ <getter><![CDATA[ >+ if (! this.InlineSpellCheckerUI) { >+ var loader = Components.classes['@mozilla.org/moz/jssubscript-loader;1'].getService(Components.interfaces.mozIJSSubScriptLoader); nit: wrap long lines, use consistent quote style (double). >+ <constructor><![CDATA[ >+ var str = this.boxObject.getProperty('value'); >+ if (str) { >+ this.inputField.value=str; >+ this.boxObject.removeProperty('value'); Use consistent quote styles to other XUL/XBL: double quotes. >+ // -- spell checking -- >+ >+ // try to find the outer textbox for this box, don't search >+ // up too far. >+ var textboxElt = popupNode; >+ for (var i = 0; i < 5; i ++) { Why climb 5? What's the context here? Is it that hard or slow to do: do { if (temp.localName == "textbox") break; temp = temp.parentNode; } while (temp); ? >+ if (textboxElt.tagName == "textbox") use .localName, not .tagName. It appears to be a bug that XUL does not return the string "xul:localName" as the .tagName for its elements. If you look at the implementation in nsGenericElement etc, you see that ::GetTagName calls mNodeInfo->GetQualifiedName, which does "ns:localName", vs. localName which just gets the actual tag name less the namespace prefix. >+ break; >+ textboxElt = textboxElt.parentNode; >+ } >+ if (textboxElt.tagName != "textbox") { Ditto. >+ <method name="doPopupItemDisabling"> I don't think you mean to expose this method on the public API of textbox. I know JS provides no information hiding capabilities, but the way we generally get around this with script and XBL is to prefix private member functions just like variables, e.g. <method name="_doPopupItemEnabling"> >+ <method name="setMenuItemVisibility"> Ditto on implied visibility. >+ <method name="setNoSpellCheckingAllowed"> >+ <method name="setAllowSpellCheck"> Are these meant to be visible outside?

Comment on attachment 203956 [details] [diff] [review] Frontend patch with Ben's comments addressed >+ if (textboxElt.tagName != "textbox") { -> localName Brett tells me that if the spellcheck extension isn't built (which it isn't now by default for Firefox) this UI does not show up at all, despite the default settings. This is good, since we don't want broken or non-functioning UI to appear for people. This gives us the capability to solve the hosting problem while maintaining all the FE hooks. Change to localName before you check in & r=ben@mozilla.org

(In reply to comment #37) >>+ // try to find the outer textbox for this box, don't search >>+ // up too far. >>+ var textboxElt = popupNode; >>+ for (var i = 0; i < 5; i ++) { >Why climb 5? What's the context here? Is it that hard or slow to do: >do { > if (temp.localName == "textbox") > break; > temp = temp.parentNode; >} > while (temp); Better still, document.getBindingParent(this) should return the textbox.

Comment on attachment 203956 [details] [diff] [review] Frontend patch with Ben's comments addressed >+ var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]. >+ getService(Components.interfaces.mozIJSSubScriptLoader); >+ loader.loadSubScript("chrome://global/content/inlineSpellCheckUI.js", this); So if you have several textboxes do they each load the script independently? I thought the point of XBL was to share code...

Comment on attachment 201693 [details] [diff] [review] Omission corrected >Index: toolkit/content/inlineSpellCheckUI.js I couldn't find a version of this in an unobsoleted patch, but my question is, have you compared this to editorInlineSpellCheck.js so as to avoid reinventing the wheel? In particular, editorInlineSpellCheck.js doesn't seem to require passing the event to find the misspelled word, but also it would be useful to avoid the duplication of code that Thunderbird, SeaMonkey and NVu would otherwise require.

(In reply to comment #40) >Better still, document.getBindingParent(this) should return the textbox. Or it could return an editable menulist, which you ought to expect anyway.

> So if you have several textboxes do they each load the script independently? I > thought the point of XBL was to share code... Yes. This is kind of a lot of code, and if it lived in XBL, a copy will live with each text box that is created. This way, the code is loaded lazily. More importantly, this code is shared with browser.js. Lazy loading of the js file is not ideal, since there is sometimes a slight delay for the context menu to pop up. Perhaps a better solution can be found later. I didn't know about the other inline spellchecker JS when I first wrote this. The problem is that the new interface is really at one level higher than the thunderbird one, which has more logic associated with the window (for example, the new one doesn't require any external callbacks so it can be used both from XBL and from browser.js). It would be nice to merge these in the future. tbird can probably use this one with few or no changes: using it from browser.js is really easy.

Fixed on trunk Notes: To build you need to say "--enable-extensions=default,spellcheck" in configure. You will also need to specify your language because there is no default (needs to be worked out how this will be set up). You can set it temporarily for a single box through the context menu. To permanently set it, create a configure entry called "spellchecker.dictionary" and set it to "en-US".

I looked at the nsIEditorSpellCheck IDL file, and it seems to not follow most of our conventions for IDL. It uses methods rather than attributes, and leading uppercase method names.

(In reply to comment #46) > I looked at the nsIEditorSpellCheck IDL file, and it seems to not follow most > of our conventions for IDL. It uses methods rather than attributes, and leading > uppercase method names. Yes! Please volunteer to refactor it; I only documented it, which was enough for me for now.

(In reply to comment #45) > Fixed on trunk > Did you take any of Neil's comments on board?

(In reply to comment #44) >This is kind of a lot of code, and if it lived in XBL, a copy will live >with each text box that is created. Not true, XBL loads once per DOM window. >More >importantly, this code is shared with browser.js Not true, each textbox loads its own copy. >The problem is that the new interface is really at one level higher than the >thunderbird one, which has more logic associated with the window (for example, >the new one doesn't require any external callbacks so it can be used both from >XBL and from browser.js). It would be nice to merge these in the future. Thanks for the clarification. Would the ideal place for the new interface be a JS component?

toolkit/content/widgets/textbox.xml now depends on browser/locales/en-US/chrome/browser/browser.dtd for the spelling entities. Shouldn't those spelling entities gone into toolkit/locales/en-US/chrome/global/ or something?

I guess this is nothing new... filing a seperate bug..

(In reply to comment #50) > toolkit/content/widgets/textbox.xml now depends on > browser/locales/en-US/chrome/browser/browser.dtd for the spelling entities. There are already entities in toolkit/locales/en-US/chrome/global/textcontext.dtd for the spellchecking items. There are duplicate ones in browser provides the entities for the browser context menu. This kind of sucks, but I was trying to avoid having to load another file for browser, which will slow it down.

Er... browser can depend on toolkit, but not the other way around. If they need to share something, the something needs to be in toolkit.

> Er... browser can depend on toolkit, but not the other way around. If they > need to share something, the something needs to be in toolkit. Brett: Is there a way that you can flip the dependency around and still benefit from not having to load an extra file?

> Brett: Is there a way that you can flip the dependency around and still benefit > from not having to load an extra file? As far as I know, there is no dependency. Everything used in XUL textboxes is in toolkit/locales/en-US/chrome/global/textcontext.dtd and everything used in browser is there, even though there is sone overlap.

sorry for the confusion -- I saw entities in the browser's dtd that totally matched what brett added to the textbox.xml widget. This point coupled with an errror that I was seeing in minimo: XML Parsing Error: undefined entity Location: jar:file:////Program%20Files/Minimo/chrome/toolkit.jar!/content/global/bindings/textbox.xml Line Number 267, Column 9: caused me to believe that this checkin caused a bad dependency. The error in minimo was caused by partially rebuilding toolkit. So, i think, in summary -- there is nothing to see here, move along. :-)

I'm noticing lots of inline spell checker assertions when closing a mail composition window after this change went in. The message has already been sent, and while we are closing the compose window it looks like we now end up trying to spell check the document again. Probably a collision between these spell check changes and the recycled compose window. brett can you reproduce these assertions too? nsContentIterator::First() line 965 + 41 bytes nsFilteredContentIterator::First() line 158 nsTextServicesDocument::FirstTextNode(nsIContentIterator * 0x04bb2ae0, nsTextServicesDocument::TSDIteratorStatus * 0x04bd947c) line 4118 nsTextServicesDocument::FirstBlock(nsTextServicesDocument * const 0x04bd9468) line 601 + 24 bytes nsTextServicesDocument::SetExtent(nsTextServicesDocument * const 0x04bd9468, nsIDOMRange * 0x046c4b88) line 354 + 12 bytes mozInlineSpellChecker::SpellCheckRange(nsIDOMRange * 0x049d1770, nsISelection * 0x04b68520) line 964 + 47 bytes mozInlineSpellChecker::SpellCheckBetweenNodes(nsIDOMNode * 0x04b694b4, int 0, nsIDOMNode * 0x04b694b4, int 0, nsISelection * 0x04b68520) line 707 + 26 bytes mozInlineSpellChecker::SpellCheckRange(mozInlineSpellChecker * const 0x04bdf7c0, nsIDOMRange * 0x00000000) line 405 + 39 bytes mozInlineSpellChecker::SpellCheckAfterEditorChange(mozInlineSpellChecker * const 0x04bdf7c0, int 3013, nsISelection * 0x04b68488, nsIDOMNode * 0x04b694b4, int 0, nsIDOMNode * 0x04b694b4, int 0, nsIDOMNode * 0x04b694b4, int 0) line 344 nsEditor::HandleInlineSpellCheck(int 3013, nsISelection * 0x04b68488, nsIDOMNode * 0x04b694b4, int 0, nsIDOMNode * 0x04b694b4, int 0, nsIDOMNode * 0x04b694b4, int 0) line 5327 + 73 bytes nsHTMLEditRules::AfterEditInner(int 3013, short 1) line 547 + 78 bytes nsHTMLEditRules::AfterEdit(nsHTMLEditRules * const 0x04b969bc, int 3013, short 1) line 391 + 20 bytes nsHTMLEditor::EndOperation(nsHTMLEditor * const 0x04b51080) line 4133 + 62 bytes nsAutoRules::~nsAutoRules() line 125 nsHTMLEditor::LoadHTML(nsHTMLEditor * const 0x04b51080, const nsAString_internal & {...}) line 244 + 85 bytes nsHTMLEditor::RebuildDocumentFromSource(nsHTMLEditor * const 0x04b5115c, const nsAString_internal & {...}) line 1797 + 70 bytes XPTC_InvokeByIndex(nsISupports * 0x04b5115c, unsigned int 20, unsigned int 1, nsXPTCVariant * 0x0012e508) line 102 XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode CALL_METHOD) line 2139 + 43 bytes XPC_WN_CallMethod(JSContext * 0x01980ea0, JSObject * 0x0493b390, unsigned int 1, long * 0x049bedb0, long * 0x0012e7d4) line 1444 + 14 bytes js_Invoke(JSContext * 0x01980ea0, unsigned int 1, unsigned int 0) line 1211 + 23 bytes js_Interpret(JSContext * 0x01980ea0, unsigned char * 0x047b6ea6, long * 0x0012f294) line 3754 + 15 bytes js_Invoke(JSContext * 0x01980ea0, unsigned int 0, unsigned int 2) line 1231 + 19 bytes nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x049a8c10, nsXPCWrappedJS * 0x049a8cc0, unsigned short 3, const nsXPTMethodInfo * 0x03534708, nsXPTCMiniVariant * 0x0012f5dc) line 1376 + 22 bytes nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x049a8cc0, unsigned short 3, const nsXPTMethodInfo * 0x03534708, nsXPTCMiniVariant * 0x0012f5dc) line 466 PrepareAndDispatch(nsXPTCStubBase * 0x049a8cc0, unsigned int 3, unsigned int * 0x0012f68c, unsigned int * 0x0012f67c) line 117 + 31 bytes SharedStub() line 147 nsMsgCompose::CloseWindow(nsMsgCompose * const 0x04a68218, int 1) line 1276

> brett can you reproduce these assertions too? There are similar assertions in FF. I didn't realize I introduced them in TB as well. I'll see if I can track them down.

It's correct that this cannot be tested on nightly builds since the spellchecker isn't build by default in the nightly builds? --enable-application=browser --enable-update-channel=nightly --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-svg --enable-canvas --enable-update-packaging

I created bug 320443 for the assertions.

So uh, somewhere between bug 239373 and this bug, textbox.xml grew two constructors and two destructors. See http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/textbox.xml#111 and http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/textbox.xml#138

(In reply to comment #61) > So uh, somewhere between bug 239373 and this bug, textbox.xml grew two > constructors and two destructors. See > http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/textbox.xml#111 > and > http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/textbox.xml#138 That blames to brett.

I'll clean that up in my patch on bug 253481

(In reply to comment #63) > I'll clean that up in my patch on bug 253481 Thanks Ted. Sorry for the mess...

Is this trunk-only, or planned for 1.8.1 branch too? If the latter, please make sure to check bug 151040 in there too -- otherwise going from FF2 to FF3 will rather suck for users...

Brett, I think we should be hiding the "Spell Check this field" menu item when the text box is read only. Spell Check This Field seems to be acting like a toogle for me. text items get underlined then the underlining gets removed every time I check the spell check this field menu item. Should it be toggling spell check on and off? If so, maybe the menu item should be a checked menu item or at least the text should change.

One last comment :), it seems like "Spell Check" would be more user friendly than "Spell Check this field". The menu item only shows up if you are inside a text field anyway, so the extra text seems somewhat redundant.

(In reply to comment #66) > Spell Check This Field seems to be acting like a toogle for me. It is. I recall that I messed up the styles so on some platforms it doesn't work, but I thought somebody else patched this. I created bug 325819 for this so I don't forget. I'll start cleaning up the spellchecking stuff when I get history a little more complete.

Boris, Scott: I'm working on bug 282187, and would like to know what is the status of theses backend+frontend patches regarding porting/copying them to SM ?

The back end is just the back end. The front end is your business, apparently. ;)

What is the status of this bug for the 1.8 branch? It seems to me like it is not clearly suitable, given the backend API changes, yet bug 325819 is targetted at Firefox 2. I'm trying to review the 1.8 branch/trunk diff for the directories that will be mirrored so that ifdefs can be added as necessary, and this is one of the larger changes (to the textbox widget).

It's my understanding that this will go in the 1.8 branch. I'll probably start working on the remaining issues later this week.

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

bz: You said no checkin on branch without bug 151040. Were you referring only to the relatively small core fix patch, or to some wider Mac-specific stuff going in?

I mean the core changes, since those changed the preference name and we don't want that changing from FF2 to FF3.

Comment on attachment 201876 [details] [diff] [review] Backend patch Want to land on branch. Bug 329668 is the tracking bug for this landing.

Comment on attachment 203956 [details] [diff] [review] Frontend patch with Ben's comments addressed Want to land on branch. Bug 329668 is the tracking bug for this landing.

(In reply to comment #76) > (From update of attachment 201876 [details] [diff] [review] [edit]) > Want to land on branch. Bug 329668 is the tracking bug for this landing. How can this patch land on the branch, given that it changes nsIEditorSpellCheck.idl, nsIEditor.idl and nsIDOMXULDocument.idl?

Brett, I'd like to see some of the follow up regressions fixed before we talk about taking this for the branch. comment 66, 67 and 68. I think I saw that you may have fixed the toggle issue already. And we're still getting inline spell check assertions when closing a mail compose window due to this change. comment 37. I'd like to see that get addressed as well before I'd approve the changes to the inline spell checker for the 1.8.1 branch. I think I'm the one that has to do that since i'm the module owner for the spell checker. Thanks!

> Brett, I'd like to see some of the follow up regressions fixed before we talk > about taking this for the branch. comment 66, 67 and 68. I think I saw that > you may have fixed the toggle issue already. The toggle issue (comment 66 and comment 68) should have been fixed by bug 320437. Please reopen if you're still seeing this problem. Comment 67 is just renaming the menuitem. I'm happy to change it, but it seems silly to hold up a landing for something trivial like menu text that is easily changed at any time. I added "in this field" because I was worried that people would think it would be for all fields or for the whole web page. When I tell people about the spell check function in the Google toolbar, many people initially think it is for the web page itself. Maybe this doesn't matter. I suggest the main UI people should get together and decide what it should say in parallel to this landing. > And we're still getting inline spell check assertions when closing a mail > compose window due to this change. I filed bug 329672 for these assertions. This patch touches so many different areas that requires many different people's approval so I was just asking for approval for each separate patch. Bug 329668 tracks these. When they all have approval, I'll land the whole thing. Any other blockers can be filed as separate bugs blocking that one. I'm happy to entertain different landing and approval approaches if you don't think this is appropriate.

Comment on attachment 203956 [details] [diff] [review] Frontend patch with Ben's comments addressed a=ben@mozilla.org

Comment on attachment 201876 [details] [diff] [review] Backend patch Gavin is right -- this can't land on branch as-is because it changes Gecko apis.

(In reply to comment #82) > (From update of attachment 201876 [details] [diff] [review] [edit]) > Gavin is right -- this can't land on branch as-is because it changes Gecko > apis. Ok. Then what should we do? I don't really have time to figure out how to rewrite it... :( Could I create a bunch of stupid interfaces that each add the one function I need (ie nsISpellcheckingEditor, nsIDOMXULDocumentWithPopupEventThingy), etc?

I believe we've been giving branch-only interfaces that we add because we can't change interfaces names like nsIDOMXULDocument_MOZILLA_1_8_BRANCH... Can you do that in this case?

(In reply to comment #84) > I believe we've been giving branch-only interfaces that we add because we can't > change interfaces names like nsIDOMXULDocument_MOZILLA_1_8_BRANCH... Can you > do that in this case? Yes, I suppose so. So this would be a new interface exposed by the object. Would it only contain my additions or would it be a dupe of the entire object on the trunk? Also, do I need to worry about the non-.idl interfaces like nsIFocusController, or can those be changed on branch?

The new interface would only expose new stuff. And yes, IDL vs non-IDL doesn't matter -- all Gecko interfaces on the 1.8 branch are not subject to change except in cases of dire emergency.

That is, you do need to worry about nsIFocusController.

I know this will be brought up sooner or later, but what about rich text editors (i.e., iframes with @designMode="on")?

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

This is a branchified version of the backend patch. Changes are: Moved popupEvent from nsIDOMXULDocument to nsIDOMXULDocument2 and rev-ed the UUID Changed nsXULDocument::Get/SetPopupEvent to QI the focusController to nsIFocusController_MOZILLA_1_8_BRANCH and call Get/SetPopupEvent on that interface. Changed XULPopupListenerImpl::PreLaunchPopup to use nsIDOMXULDocument2 COM ptr instead of nsIDOMXULDocument. Added nsIFocusController_MOZILLA_1_8_BRANCH inheriting from nsIFocusController and moved Get/SetPopupEvent there. Updated nsFocusController to inherit from nsIFocusController_MOZILLA_1_8_BRANCH instead of nsIFocusController and updated the IMPL_ISUPPORTS to include the new interface. QI spellchecker to nsIEditorSpellCheck_MOZILLA_1_8_BRANCH so we can call CanSpellCheck in mozInlineSpellChecker::CanEnableInlineSpellChecking Merged fix for bug 329672 into mozInlineSpellChecker::SpellCheckBetween and ::HandleNavigationEvent Inherit nsEditorSpellCheck from nsIEditorSpellCheck_MOZILLA_1_8_BRANCH instead of nsIEditorSpellCheck In nsTextControlFrame::SetEnableRealTimeSpell try to QI mEditor to nsIEditorSpellCheck_MOZILLA_1_8_BRANCH so we can call GetInlineSpellCheckerOptionally. Fall back to GetInlineSpellChecker if that fails (will be slightly less efficient). Added nsIEditor_MOZILLA_1_8_BRANCH inheriting from nsIEditor that declares getInlineSpellCheckerOptionally. The name had to change from trunk because getInlineSpellChecker caused conflicts with the inlineSpellChecker attribute on nsIEditor. Created new nsIEditorSpellCheck_MOZILLA_1_8_BRANCH deriving from nsIEditorSpellCheck, moved the function CanSpellCheck there.

Comment on attachment 215926 [details] [diff] [review] Branch patch This is ok for 1.8.1 branch if the core changes for bug 151040 also land. As in, make sure we don't ship different pref names on trunk and branch!

Comment on attachment 201876 [details] [diff] [review] Backend patch I'm having serious worries now about the fact that this patch exposes the popupEvent property to untrusted code. What's the security exposure?

On branch.

For Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060508 BonEcho/2.0a1 ID:2006050803 suggestions do not appear when pressing the context menu key on a Windows keyboard, but only on a RMB (RightMouseButton) click. Does that belong here - or is there a bug filed?

(In reply to comment #94) > For > Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060508 > BonEcho/2.0a1 ID:2006050803 > suggestions do not appear when pressing the context menu key on a Windows > keyboard, but only on a RMB (RightMouseButton) click. Hmm, it used to work... I filed bug 337368 for this.

I'm not sure this is the right place for this, but: why do the spellchecker doesn't use html/xml language code of the web page rendered to automatically select the appropriate dictionary on a page? Each page is supposed to have a html and/or xml lang-code attached, and each html/xml element within it can cascade the lang-code from it. So it shouldn't be difficult for the spellchecker to detect the appropriate language (and if something goes wrong, the user still can manually select one).

Sounds like an excellent idea for an enhancement request! Please file it as a separate bug?

I wasn't sure it qualified as a new bug/ticket. I'll add it, thanks.

(In reply to comment #97) > Sounds like an excellent idea for an enhancement request! Please file it as a > separate bug? Already filed as bug 338427