Spun off from bug 336799: We want a way to add a textbox with spellchecking UI enabled, ideally as a new binding that extends the textbox widget (spellcheck="true") This shouldn't be risky, since it will be a new widget we're not using, but we want to get that in place for this week. Assigning to Neil, can you take a look at what was removed in 336799 and restore that functionality as described above (or a better way, if you have one)

Is this for the trunk or branch? For 1.9, I plan on creating an unprivileged API, such as Toolkit.spell which has all the necessary spellchecking functions.

Hasn't this already been done by brentw in the current textbox.xml? Either way, could this be added to the 'editor' widget as well?

Enn, target is branch.

(In reply to comment #2) > Hasn't this already been done by brentw in the current textbox.xml? > > Either way, could this be added to the 'editor' widget as well? No. See bug 302050 comment 35, which was ignored :)

Personally, for the branch, I think the correct fix is to just back out bug 336799 and fix it the way it should have been to hide the spellcheck ui when the spellcheck attribute isn't true. For the trunk, the use of the subscript loader needs to be removed anyway. Creating a separate binding isn't a good idea in my opinion, especially not if its keyed on the spellcheck attribute.

(In reply to comment #5) > Creating a separate binding isn't a good idea in my opinion, especially not if > its keyed on the spellcheck attribute. Why not? That's how the autocomplete binding works, for <textbox type="autocomplete">. If most in-chrome textboxes currently don't want spellchecking, why should the code for it belong in the base textbox binding?

Because differences in functionality should only be keyed off of the type attribute. Otherwise, one could do: <textbox type="autocomplete" spellcheck="true"/>

(In reply to comment #7) > Because differences in functionality should only be keyed off of the type > attribute. Otherwise, one could do: > > <textbox type="autocomplete" spellcheck="true"/> Is there a desire to be able to dynamically change whether a given textbox has spellcheck turned on? If not, we could use |type="spellcheck"| instead of spellcheck="true", right?

I think it is desirable, though its absence would not be a deal-breaker. type="spellcheck" would be fine for Fx2, though using type will make it harder to compatibly add dynamic control later, I suspect.

Adds support for spellcheck="true" to enable spellchecking for XUL textboxes. The attribute can be removed dynamically. Supported in chrome XUL documents only.

>Is there a desire to be able to dynamically change whether a given textbox has spellcheck turned on? Yes, specifically for an extension to give users a choice if they want it enabled/disabled by default. I can think of 8 extensions I use that could make use of this, plus all the ones I don't. >>(In reply to comment #10) > Adds support for spellcheck="true" to enable spellchecking for XUL textboxes. > The attribute can be removed dynamically. Great patch Neil, any chance to see this for the editor binding as well?

I don't know anything about XUL, so this may be nonsensical, but I think it would be a win to preserve the HTML element semantics for spellchecking over in XUL as well, if possible (see bug 339127).

Is there any reason why Neil's patch won't make Firefox 2.0? While it should be off by default, my testing seems to show it's pretty solid for it to be an option that an extension could use on one of it's textbox's (by setting spellcheck="true" )

Comment on attachment 231709 [details] [diff] [review] Add spellchecking for textbox This seems right from a fast pass. Mano, can you look a little deeper for me, since I'm on vacation and all?

Comment on attachment 231709 [details] [diff] [review] Add spellchecking for textbox For trunk, I don't think a separate binding is the right thing given comment 7.

mano, the implementation doesn't use a separate binding for <textbox>. It only changes the binding used for an internal box.

Comment on attachment 231709 [details] [diff] [review] Add spellchecking for textbox Sorry, I obviously read this wrong. After applying this patch I got: Error: undefined entity ... <xul:menuitem label="&spellNoSuggestions.label;" anonid="spell-no-suggestions" disabled="true"/> because spellNoSuggestions.label is set in browser.dtd. >Index: toolkit/content/widgets/textbox.xml >=================================================================== >+ <implementation> >+ <field name="_spellCheckInitialized">false</field> >+ <property name="spellCheckerUI" readonly="true"> >+ <getter><![CDATA[ >+ if (!this._spellCheckInitialized) { >+ this._spellCheckInitialized = true; >+ >+ const CI = Components.interfaces; >+ if (!document instanceof CI.nsIDOMXULDocument) >+ return null; >+ >+ var textbox = document.getBindingParent(this); >+ if (!textbox || !textbox instanceof CI.nsIDOMXULTextBoxElement) >+ return null; >+ >+ try { >+ var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]. >+ getService(CI.mozIJSSubScriptLoader); >+ loader.loadSubScript("chrome://global/content/inlineSpellCheckUI.js", this); >+ if ("InlineSpellCheckerUI" in this) >+ this.InlineSpellCheckerUI.init( >+ textbox.inputField.QueryInterface(CI.nsIDOMNSEditableElement).editor); >+ } catch(ex) { } >+ } >+ >+ return this.InlineSpellCheckerUI; >+ ]]></getter> >+ </property> >+ >+ <constructor> >+ <![CDATA[ >+ // can't initialize the spell checker in the constructor as not >+ // everything is initialized and the editor will fail to create the >+ // inline spell checker object >+ setTimeout(this._delayedInitSpellCheck, 0, this) >+ ]]> >+ </constructor> >+ >+ <method name="_delayedInitSpellCheck"> please inline _delayedInitSpellCheck instead (var _delayedInitSpellCheck = function(self) that is). >+ document.getAnonymousElementByAttribute(this, "anonid", >+ "spell-check-enabled").setAttribute("checked", enabled); cache this. >+ // dictionary list >+ var dictmenu = document.getAnonymousElementByAttribute(this, "anonid", >+ "spell-dictionaries-menu"); ditto.

Comment on attachment 245670 [details] [diff] [review] fix review comments >Index: toolkit/locales/en-US/chrome/global/textcontext.dtd >=================================================================== >+<!ENTITY spellAddToDictionary.label "Add to dictionary"> >+<!ENTITY spellAddToDictionary.accesskey "o"> >+<!ENTITY spellEnable.label "Spell check this field"> >+<!ENTITY spellEnable.accesskey "S"> >+<!ENTITY spellNoSuggestions.label "(No spelling suggestions)"> >+<!ENTITY spellDictionaries.label "Languages"> >+<!ENTITY spellDictionaries.accesskey "l"> Please remove those from browser.dtd and add textcontext.dtd to browser-doctypes.inc. r=mano with that fixed.

Checked in

I'm guessing no, but is there a chance this can make it for 2.0.0.1? This would be really handy for extension developers...

Neil, this code needs to have accompanying unit tests per the new rules Benjamin laid out, right? ;)

There are some testcases at http://xulplanet.com/ndeakin/tests/xts/textbox/ They will need to adapted for automated testing once we have a means for doing so.

We already have the ability to do unit-testing based on mochikit. Please let's not let this fall of the radar.

(In reply to comment #24) > We already have the ability to do unit-testing based on mochikit. Please let's > not let this fall of the radar. > No, that isn't adequate for testing this bug. Testing this feature requires event simulation and image comparisons so that we can ensure that the spellchecking menu works properly.

(In reply to comment #25) > > No, that isn't adequate for testing this bug. Testing this feature requires > event simulation Is the event simulation found here enough: http://lxr.mozilla.org/mozilla/source/testing/mochitest/tests/test_bug359754.xul or do you need something more extensive? > and image comparisons so that we can ensure that the > spellchecking menu works properly. Can't we check that it is working correctly via the DOM? Or was there a painting bug here?

We should get bugs on file for any needed enhancements to the test infrastructure, too, but I agree with what I think sayrer is saying: what we can test with the framework today we should do that way, so that it's easier to automate and run reliably.

The trunk patch applied cleanly to the branch. Neil, do you have any objections to nominating this patch for the branch? This change woul empower inline spell checking support for the subject field in Thunderbird / Seamonkey mail compose. And that would be really sweet! Note: the strings added to textcontext.dtd in the original patch already exist on the 1.8 branch so we wouldn't be introducing any new string changes. And this patch isn't changing any of the APIs for a texbox.

Comment on attachment 253133 [details] [diff] [review] port to the mozilla 1.8 branch thanks Neil. Triage team: this patch gives thunderbird and seamonkey the ability to perform inline spell checking in the subject for mail compose. It doesn't change the existing behavior of text boxes. The new code textbox code is only triggered if you add the spell check attribute to your text box so the risk is very low for existing consumers. The patch has been on the trunk since November. Lemme know if I can provide any more information.

Comment on attachment 253133 [details] [diff] [review] port to the mozilla 1.8 branch Approved for 1.8 branch, a=jay for drivers.

Marking dev-doc-complete; this was documented ages ago.

OK, actually it was the HTML version of the same thing that was documented. I just added this attribute to the XUL textbox documentation, so now this is really done.

The HTML doc was http://developer.mozilla.org/en/docs/Controlling_spell_checking_in_HTML_forms The new XUL attribute is documented here: http://developer.mozilla.org/en/docs/XUL:textbox#a-spellcheck