Last Comment Bug 346787 - add textbox binding with support for spellcheck
: add textbox binding with support for spellcheck
Status: RESOLVED FIXED
: dev-doc-complete, fixed1.8.1.2
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla1.8.1beta2
Assigned To: Neil Deakin (away until Oct 3)
:
:
Mentors:
: 359063 366035 (view as bug list)
Depends on: 361458 544520
Blocks: 3459 340177
  Show dependency treegraph
 
Reported: 2006-07-31 20:44 PDT by Mike Connor [:mconnor]
Modified: 2010-02-24 06:28 PST (History)
20 users (show)
ispiked: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add spellchecking for textbox (9.40 KB, patch)
2006-08-01 19:59 PDT, Neil Deakin (away until Oct 3)
asaf: first‑review-
mconnor: second‑review+
Details | Diff | Splinter Review
fix review comments (10.48 KB, patch)
2006-11-15 10:41 PST, Neil Deakin (away until Oct 3)
asaf: first‑review+
Details | Diff | Splinter Review
port to the mozilla 1.8 branch (8.61 KB, patch)
2007-01-28 21:43 PST, Scott MacGregor
enndeakin: first‑review+
jaymoz: approval1.8.1.2+
Details | Diff | Splinter Review

Description Mike Connor [:mconnor] 2006-07-31 20:44:10 PDT
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)
Comment 1 Neil Deakin (away until Oct 3) 2006-07-31 21:16:45 PDT
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.
Comment 2 Jed B 2006-07-31 23:28:09 PDT
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?
Comment 3 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-01 01:05:27 PDT
Enn, target is branch.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-08-01 04:35:19 PDT
(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 :)
Comment 5 Neil Deakin (away until Oct 3) 2006-08-01 08:24:05 PDT
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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-08-01 08:38:44 PDT
(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?
Comment 7 Neil Deakin (away until Oct 3) 2006-08-01 09:25:40 PDT
Because differences in functionality should only be keyed off of the type attribute. Otherwise, one could do:

<textbox type="autocomplete" spellcheck="true"/>
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-08-01 10:35:51 PDT
(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?
Comment 9 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-08-01 10:39:46 PDT
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.
Comment 10 Neil Deakin (away until Oct 3) 2006-08-01 19:59:40 PDT
Created attachment 231709 [details] [diff] [review]
Add spellchecking for textbox

Adds support for spellcheck="true" to enable spellchecking for XUL textboxes. The attribute can be removed dynamically.

Supported in chrome XUL documents only.
Comment 11 Jed B 2006-08-03 08:49:15 PDT
>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?
Comment 12 Peter Kasting 2006-08-17 16:07:54 PDT
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).
Comment 13 Jed B 2006-09-28 16:07:37 PDT
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 14 Mike Connor [:mconnor] 2006-10-30 06:42:28 PST
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 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-01 16:13:19 PST
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.
Comment 16 Neil Deakin (away until Oct 3) 2006-11-10 14:09:39 PST
mano, the implementation doesn't use a separate binding for <textbox>. It only changes the binding used for an internal box.
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-14 06:18:46 PST
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 18 Neil Deakin (away until Oct 3) 2006-11-15 10:41:56 PST
Created attachment 245670 [details] [diff] [review]
fix review comments
Comment 19 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-15 12:31:51 PST
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.
Comment 20 Neil Deakin (away until Oct 3) 2006-11-20 19:54:36 PST
Checked in
Comment 21 Shawn Wilsher :sdwilsh 2006-11-20 20:05:54 PST
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...
Comment 22 Adam Guthrie 2006-11-20 20:21:23 PST
Neil, this code needs to have accompanying unit tests per the new rules Benjamin laid out, right? ;)
Comment 23 Neil Deakin (away until Oct 3) 2006-11-20 20:29:18 PST
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.
Comment 24 Benjamin Smedberg [:bsmedberg] 2006-11-28 08:48:05 PST
We already have the ability to do unit-testing based on mochikit. Please let's not let this fall of the radar.
Comment 25 Neil Deakin (away until Oct 3) 2006-11-28 09:08:19 PST
(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.
Comment 26 Robert Sayre 2006-11-28 10:03:23 PST
(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?

Comment 27 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-11-28 10:06:17 PST
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.
Comment 28 Jo Hermans 2007-01-05 08:08:13 PST
*** Bug 359063 has been marked as a duplicate of this bug. ***
Comment 29 Jo Hermans 2007-01-05 08:08:20 PST
*** Bug 366035 has been marked as a duplicate of this bug. ***
Comment 30 Scott MacGregor 2007-01-28 21:43:57 PST
Created attachment 253133 [details] [diff] [review]
port to the mozilla 1.8 branch

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 31 Scott MacGregor 2007-01-29 08:55:14 PST
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 32 Jay Patel [:jay] 2007-01-29 11:08:52 PST
Comment on attachment 253133 [details] [diff] [review]
port to the mozilla 1.8 branch

Approved for 1.8 branch, a=jay for drivers.
Comment 33 Eric Shepherd [:sheppy] 2007-03-06 08:53:09 PST
Marking dev-doc-complete; this was documented ages ago.
Comment 34 Eric Shepherd [:sheppy] 2007-03-06 11:01:41 PST
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.
Comment 35 Nickolay_Ponomarev 2007-03-06 13:37:54 PST
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

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