302050 - Want realtime spell checking mechanism.

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Matt Armstrong]

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

Comment 1

[Matt Armstrong]

Attachment: A patch to partially implement this feature.

I have attached a patch that partially implements this feature.

Comment 2

[Brian Ryner (not reading)]

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.

Comment 3

[Brian Ryner (not reading)]

Attachment: cleaned up patch

not quite ready yet, but has the review comments addressed

Comment 4

[Brett Wilson]

Attachment: Spellcheck patch with context menu UI

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

Comment 5

[Brett Wilson]

Attachment: Patch with more UI, dictionary selection

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 6

[Scott MacGregor]

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 7

[Brian Ryner (not reading)]

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

clearing obsolete requests

Comment 8

[Brett Wilson]

Attachment: Patch with minor enhancements

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 9

[Brian Ryner (not reading)]

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 10

[Brian Ryner (not reading)]

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

Comment 11

[Brett Wilson]

Attachment: New patch

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 12

[Brian Ryner (not reading)]

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 13

[Brett Wilson]

Attachment: Patch with bryner's suggestions

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 200227 [details] [diff] [review]
Patch with bryner's suggestions

Thanks, I'll go ahead and check this in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

s/nsTextFrame/nsTextControlFrame/ in that last comment.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Brett Wilson]

Attachment: New patch that includes chrome support

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

> 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

Comment 20

[Brett Wilson]

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

Comment 21

[Brett Wilson]

Attachment: Omission corrected

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

Comment 22

[Brian Ryner (not reading)]

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 23

[Brian Ryner (not reading)]

Comment on attachment 201693 [details] [diff] [review]
Omission corrected

Same comments as above.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[lists]

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.

Comment 26

[Brett Wilson]

(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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[Brett Wilson]

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

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?

Comment 32

[Brett Wilson]

Attachment: Backend patch

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

Comment 33

[Brett Wilson]

Attachment: Frontend patch

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

Comment 35

[:Gavin Sharp [email: [e-mail]]]

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 36

[Boris Zbarsky [:bzbarsky]]

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 37

[Ben Goodger (use ben at mozilla dot org for email)]

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 38

[Brett Wilson]

Attachment: Frontend patch with Ben's comments addressed

Comment 39

[Ben Goodger (use ben at mozilla dot org for email)]

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=[e-mail]

Comment 40

[[e-mail]]

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

[[e-mail]]

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 42

[[e-mail]]

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.

Comment 43

[[e-mail]]

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

Comment 44

[Brett Wilson]

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

Comment 45

[Brett Wilson]

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

Comment 46

[Simon Fraser [no longer active]]

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.

Comment 47

[Brett Wilson]

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

Comment 48

[Ian Neal]

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

Comment 49

[[e-mail]]

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

Comment 50

[Doug Turner (:dougt)]

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?

Comment 51

[Doug Turner (:dougt)]

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

Comment 52

[Brett Wilson]

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

Comment 53

[Boris Zbarsky [:bzbarsky]]

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.

Comment 54

[Darin Fisher]

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

Comment 55

[Brett Wilson]

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

Comment 56

[Doug Turner (:dougt)]

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

Comment 57

[Scott MacGregor]

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

Comment 58

[Brett Wilson]

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

Comment 59

[Henrik Gemal]

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

Comment 60

[Brett Wilson]

I created bug 320443 for the assertions.

Comment 61

[(not currently active) Ted Mielczarek]

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

Comment 62

[Simon Fraser [no longer active]]

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

Comment 63

[(not currently active) Ted Mielczarek]

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

Comment 64

[Brett Wilson]

(In reply to comment #63)
> I'll clean that up in my patch on bug 253481

Thanks Ted. Sorry for the mess...

Comment 65

[Boris Zbarsky [:bzbarsky]]

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

Comment 66

[Scott MacGregor]

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.

Comment 67

[Scott MacGregor]

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.

Comment 68

[Brett Wilson]

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

Comment 69

[Serge Gautherie (:sgautherie)]

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 ?

Comment 70

[Boris Zbarsky [:bzbarsky]]

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

Comment 71

[:Gavin Sharp [email: [e-mail]]]

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

Comment 72

[Brett Wilson]

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.

Comment 73

[Phil Ringnalda (:philor)]

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

Comment 74

[Brett Wilson]

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?

Comment 75

[Boris Zbarsky [:bzbarsky]]

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

Comment 76

[Brett Wilson]

Comment on attachment 201876 [details] [diff] [review]
Backend patch

Want to land on branch. Bug 329668 is the tracking bug for this landing.

Comment 77

[Brett Wilson]

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.

Comment 78

[:Gavin Sharp [email: [e-mail]]]

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

Comment 79

[Scott MacGregor]

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!

Comment 80

[Brett Wilson]

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

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 203956 [details] [diff] [review]
Frontend patch with Ben's comments addressed

a=[e-mail]

Comment 82

[Boris Zbarsky [:bzbarsky]]

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.

Comment 83

[Brett Wilson]

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

Comment 84

[Boris Zbarsky [:bzbarsky]]

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?

Comment 85

[Brett Wilson]

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

Comment 86

[Boris Zbarsky [:bzbarsky]]

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.

Comment 87

[Boris Zbarsky [:bzbarsky]]

That is, you do need to worry about nsIFocusController.

Comment 88

[nrlz]

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

Comment 89

[Phil Ringnalda (:philor)]

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

Comment 90

[Brett Wilson]

Attachment: Branch patch

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 91

[Boris Zbarsky [:bzbarsky]]

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 92

[Boris Zbarsky [:bzbarsky]]

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?

Comment 93

[Brett Wilson]

On branch.

Comment 94

[Thomas Stache]

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?

Comment 95

[Brett Wilson]

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

Comment 96

[Jérémie Bouillon]

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

Comment 97

[Boris Zbarsky [:bzbarsky]]

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

Comment 98

[Jérémie Bouillon]

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

Comment 99

[Brett Wilson]

(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