Closed Bug 346787 Opened 18 years ago Closed 18 years ago

add textbox binding with support for spellcheck

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: mconnor, Assigned: enndeakin)

References

Details

(Keywords: dev-doc-complete, fixed1.8.1.2)

Attachments

(2 files, 1 obsolete file)

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.
Target Milestone: --- → mozilla1.8.1beta2
(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.
Attached patch Add spellchecking for textbox (obsolete) — Splinter Review
Adds support for spellcheck="true" to enable spellchecking for XUL textboxes. The attribute can be removed dynamically.

Supported in chrome XUL documents only.
Attachment #231709 - Flags: first-review?(mconnor)
Blocks: 340177
>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).
Blocks: 3459
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?
Attachment #231709 - Flags: second-review+
Attachment #231709 - Flags: first-review?(mconnor)
Attachment #231709 - Flags: first-review?(mano)
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.
Attachment #231709 - Flags: first-review?(mano) → first-review-
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.
Attachment #231709 - Attachment is obsolete: true
Attachment #245670 - Flags: first-review?(mano)
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.
Attachment #245670 - Flags: first-review?(mano) → first-review+
Checked in
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: dev-doc-needed
Resolution: --- → FIXED
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? ;)
Flags: in-testsuite?
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.
Depends on: 361458
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.
Attachment #253133 - Flags: first-review?(enndeakin)
Attachment #253133 - Flags: first-review?(enndeakin) → first-review+
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.
Attachment #253133 - Flags: approval1.8.1.2?
Comment on attachment 253133 [details] [diff] [review]
port to the mozilla 1.8 branch

Approved for 1.8 branch, a=jay for drivers.
Attachment #253133 - Flags: approval1.8.1.2? → approval1.8.1.2+
Keywords: fixed1.8.1.2
Blocks: TB2SM
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.
No longer blocks: TB2SM
Depends on: 544520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: