626076 - Add spellcheck UI in the context menu, à la Firefox, for all HTML Textareas

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Jonathan Protzenko [:protz]]

Attachment: Preliminary version of the patch

The idea is that the right-click menu should be able to deal with HTML Textareas that have spellcheck="true". Extensions might want a cheap way to spellcheck some of their user input, and I believe this is potentially useful for at least Thunderbird Air as well as Thunderbird Conversations.

This is mostly copy/paste from Firefox with minor improvements and some Thunderbird tweaks. This looks like a potentially nice feature to have: AFAIK, we have no HTML Textareas, so this won't impact our UI. Simply, it will allows extension authors to easily spellcheck their contents (assuming they use HTML, of course).

Comment 1

[Rain [:rain1] (inactive)]

Can you specify all the cases you're taking care of and when each item should be shown? Preferably via mozmill tests. For instance, I don't see why right-clicking in an area that isn't editable should show anything related to spell check at all.

Comment 2

[Jonathan Protzenko [:protz]]

Comment on attachment 504135 [details] [diff] [review]
Preliminary version of the patch

Will work on a new version of the patch with tests and details mentioned over IRC taken into account.

Comment 3

[Jonathan Protzenko [:protz]]

Attachment: Updated patch, with tests, amendments, etc.

I've updated the patch and fixed a few things:
- I had messed up with separators, this is now fixed,
- I've fixed the bad naming of the (now) spellChecker object,
- I've stuck to the Firefox way of doing things (we agreed on that on IRC, although it's not really pretty),
- I've added tests.

Concerning tests, I feel like we were almost never testing the context menu. Since the main advantage of this patch is that it will allow users to control their spellchecking in content tabs through the right-click menu, I've added the tests to the content tabs section. I also took the liberty of testing a little bit more.

Using mc.eid to get the elements inside the content tab doesn't seem to work (comments welcome), so I'm kind of forcing mc.rightClick into sending the event to the right element.

Comment 4

[Rain [:rain1] (inactive)]

Comment on attachment 510983 [details] [diff] [review]
Updated patch, with tests, amendments, etc.

+    <menuitem id="mailContext-spell-add-to-dictionary"
+              label="&spellAddToDictionary.label;"
+              accesskey="&spellAddToDictionary.accesskey;"
+              oncommand="InlineSpellChecker.addToDictionary();"/>

This is wrong: it should be gSpellChecker.addToDictionary().

> 
>+Components.utils.import("resource://gre/modules/InlineSpellChecker.jsm");
>+var spellChecker = new InlineSpellChecker();

Please rename this to gSpellChecker.

>+
>+function getDictionaryURL() {
>+  let formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>+                  .getService(Components.interfaces.nsIURLFormatter);
>+  return formatter.formatURLPref("spellchecker.dictionaries.download.url");
>+}
>+
> function nsContextMenu(aXulMenu) {
>   this.target         = null;
>   this.menu           = null;
>@@ -97,8 +106,54 @@
>     this.initMediaPlayerItems();
>     this.initBrowserItems();
>     this.initMessageItems();
>+    this.initSpellingItems();
>     this.initSeparators();
>   },
>+  addDictionaries: function CM_addDictionaries() {
>+    openURL(getDictionaryURL());
>+  },
>+  initSpellingItems: function CM_initSpellingItems() {
>+    var canSpell = spellChecker.canSpellCheck;
>+    var onMisspelling = spellChecker.overMisspelling;
>+    this.showItem("mailContext-spell-check-enabled", canSpell);
>+    this.showItem("mailContext-spell-separator", canSpell || this.onEditableArea);
>+    if (canSpell) {
>+      document.getElementById("mailContext-spell-check-enabled")
>+              .setAttribute("checked", spellChecker.enabled);
>+    }
>+
>+    this.showItem("mailContext-spell-add-to-dictionary", onMisspelling);
>+
>+    // suggestion list
>+    this.showItem("mailContext-spell-suggestions-separator", onMisspelling);
>+    if (onMisspelling) {
>+      var suggestionsSeparator =
>+        document.getElementById("mailContext-spell-add-to-dictionary");
>+      var numsug =
>+        spellChecker.addSuggestionsToMenu(suggestionsSeparator.parentNode,
>+                                                  suggestionsSeparator, 5);

Since you're changing this stuff anyway, please update this to let instead of var, and fix the indentation for the last line there.

>+    else
>+      this.showItem("mailContext-spell-no-suggestions", false);

Our code style says this should have braces around it.

>+
>+    // dictionary list
>+    this.showItem("mailContext-spell-dictionaries", spellChecker.enabled);
>+    if (canSpell) {
>+      var dictMenu = document.getElementById("mailContext-spell-dictionaries-menu");
>+      var dictSep = document.getElementById("mailContext-spell-language-separator");

Please update these to let too.

>+    else if (this.onEditableArea) {
>+      // when there is no spellchecker but we might be able to spellcheck
>+      // add the add to dictionaries item. This will ensure that people
>+      // with no dictionaries will be able to download them
>+      this.showItem("mailContext-spell-add-dictionaries-main", true);
>+    }
>+    else
>+      this.showItem("mailContext-spell-add-dictionaries-main", false);

- Braces around this.
- You might want to consider changing this to else { this.showItem("mailContext-spell-add-dictionaries-main", this.onEditableArea); } if you think it's still readable enough. I don't think it will be, though.

>+  },
>   initSaveItems : function CM_initSaveItems() {
>     this.showItem("mailContext-savelink", this.onSaveableLink);
>     this.showItem("mailContext-saveimage", this.onLoadedImage);
>@@ -291,7 +346,8 @@
>       "mailContext-sep-afterTagAddNew", "mailContext-sep-afterTagRemoveAll",
>       "mailContext-sep-afterMarkAllRead", "mailContext-sep-afterMarkFlagged",
>       "mailContext-sep-afterMarkMenu", "mailContext-sep-edit",
>-      "mailContext-sep-copy", "mailContext-sep-reportPhishing"
>+      "mailContext-sep-copy", "mailContext-sep-reportPhishing",
>+      "mailContext-spell-suggestions-separator", "mailContext-spell-separator",
>     ];
>     mailContextSeparators.forEach(this.hideIfAppropriate, this);
> 
>@@ -303,6 +359,18 @@
>    * its ancestors.
>    */
>   setTarget : function CM_setTarget(aNode) {
>+    // Clear any old spellchecking items from the menu, this used to
>+    // be in the menu hiding code but wasn't getting called in all
>+    // situations. Here, we can ensure it gets cleaned up any time the
>+    // menu is shown. Note: must be before uninit because that clears the
>+    // internal vars

Full stop here please.

>+        if (!this.target.readOnly) {
>+          this.onEditableArea = true;
>+          spellChecker.init(this.target.QueryInterface(Components.interfaces.nsIDOMNSEditableElement).editor);
>+          //spellChecker.initFromEvent(aRangeParent, aRangeOffset);

Please remove the commented-out line.

>    */
>-  showItem : function CM_showItem(aItemOrId, aShow) {
>+  showItem: function CM_showItem(aItemOrId, aShow) {

This doesn't seem like it should be part of the patch.

>+<!-- Spell checker context menu items -->
>+<!ENTITY spellAddToDictionary.label "Add to Dictionary">
>+<!ENTITY spellAddToDictionary.accesskey "o">
>+<!ENTITY spellCheckEnable.label "Check Spelling">
>+<!ENTITY spellCheckEnable.accesskey "S">
>+<!ENTITY spellNoSuggestions.label "(No Spelling Suggestions)">
>+<!ENTITY spellDictionaries.label "Languages">
>+<!ENTITY spellDictionaries.accesskey "l">
>+<!ENTITY spellAddDictionaries.label "Add Dictionariesâ¦">
>+<!ENTITY spellAddDictionaries.accesskey "A">
>+

Why not reuse the toolkit entities for these?

>   // Check that window.content is set up correctly wrt content-primary and
>   // content-targetable.
>-  if (mc.window.content.location != whatsUrl)
>-    throw new Error("window.content is not set to the url loaded, incorrect type=\"...\"?");
>+  assert_equals(mc.window.content.location, whatsUrl,
>+    "window.content is not set to the url loaded, incorrect type=\"...\"?");
> }
> 
>-  if (mc.tabmail.tabContainer.childNodes.length != preCount)
>-    throw new Error("A new content tab was opened when it shouldn't have been");
>+  assert_equals(mc.tabmail.tabContainer.childNodes.length, preCount,
>+    "A new content tab was opened when it shouldn't have been");
> 
>   // Double-check browser is still the same.
>-  if (mc.window.content.location != whatsUrl)
>-    throw new Error("window.content is not set to the url loaded, incorrect type=\"...\"?");
>+  assert_equals(mc.window.content.location, whatsUrl,
>+    "window.content is not set to the url loaded, incorrect type=\"...\"?");

Again, these don't seem like they should be part of the patch. (It messes up blame.)

>+/**
>+ * Just make sure that the context menu does what we expect in content tabs wrt.
>+ * spell checking options.
>+ */
>+function test_context_menu_in_content_tabs() {

If you're only testing spell check, you should call it something like test_spell_check_in_content_tabs().

>+  
>+  // Helper functions
>+  let visible = function (id) !mc.eid(id).getNode().hidden;
>+  let assert_visible = function (id) assert_true(visible(id));
>+  let assert_not_visible = function (id) assert_true(!visible(id));

Oh man, I completely messed the names of these functions up: <https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-content-tab-helpers.js?mark=235-277#235>. It would be very awesome if you could:
- rename all the functions to get_content_tab_element_display etc.
- Move these functions out to a helper file, and rename them to assert_element_visible and assert_element_not_visible. You could even make them polymorphic and accept both ID strings and DOM elements, and also add a second optional parameter for the controller (which would of course be mc by default).

>+
>+  // Test a few random items
>+  mc.rightClick({ getNode: function () textarea });
>+  assert_visible("mailContext-spell-dictionaries");
>+  assert_visible("mailContext-spell-check-enabled");
>+  assert_not_visible("mailContext-replySender"); // we're in a content tab!
>+  close_popup();
>+
>+  // Different test
>+  mc.rightClick({ getNode: function () w.document.body.firstElementChild});
>+  assert_not_visible("mailContext-spell-dictionaries");
>+  assert_not_visible("mailContext-spell-check-enabled");
>+  close_popup();
>+}

Can you add a test for a misspelled word here to test out add to dictionary and spelling corrections, or will it be too much work?

>+
>+/**
>+ * Test closing a tab.
>+ */
>+function test_inner_close() {
>+  let tabmail = mc.tabmail;
>+  let w = tabmail.selectedTab.browser.contentWindow;
>+
>+  w.close();
>+  assert_equals(tabmail.tabContainer.selectedIndex, 0);
>+}

I think we already have plenty of tests for this. Did you have a particular problem in mind here?

The patch looks fine overall, but I'd like to have another look at it. The patch also needs ui-review.

Comment 5

[Jonathan Protzenko [:protz]]

Comment on attachment 510983 [details] [diff] [review]
Updated patch, with tests, amendments, etc.

Bryan, I'd like to have your opinion on this before I spend more time on it.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 510983 [details] [diff] [review]
Updated patch, with tests, amendments, etc.

This looks good to me.  IIRC there is a bug out there to have the compose context menu fixed up to use this as well.  So the dictionary and on/off support is in the context menu.  Might be nice to try transferring this work over there afterward.

Comment 7

[Jonathan Protzenko [:protz]]

Attachment: Takes review comments into accounts

Carrying forward clarkbw's ui-review.

Here's an updated patch. Changelog:
- I've renamed all your functions from test-content-tab-helpers.js and changed all relevant tests. This removed name collisions, so I was able to add my own test-dom-helpers.js file, which implements assert_visible and assert_not_visible, as requested.
- I added a test for a misspelled word.
- Rewrote part of the code imported from Firefox so that it complies with Thunderbird dogmas, e.g. indentation, var vs let, brackets, alignment...
- Fixed all remarks, including punctuation, spaces, etc.
- Removed potentially useful cleanups in test-content-tabs.js.

Comment 8

[Jonathan Protzenko [:protz]]

Attachment: Upload the right file instead

Comment 9

[Jonathan Protzenko [:protz]]

Requesting blocking because this would be cool to have to go on par with Thunderbird Conversations 3.3a3.

Comment 10

[Jonathan Protzenko [:protz]]

Attachment: New version that fixes a typo

This patches fixes a small typo I had forgotten.

Comment 11

[Rain [:rain1] (inactive)]

Comment on attachment 517305 [details] [diff] [review]
New version that fixes a typo

Sorry for the delay. The patch looks great, and I'm building with it now.

>+      let suggestionsSeparator =
>+        document.getElementById("mailContext-spell-add-to-dictionary");

That isn't the suggestions separator.

>+      let numsug =
>+        gSpellChecker.addSuggestionsToMenu(suggestionsSeparator.parentNode,
>+                                           suggestionsSeparator, 5);
>+      this.showItem("mailContext-spell-no-suggestions", numsug == 0);

Nit: camelCase please, e.g. suggestionCount.

Comment 12

[Rain [:rain1] (inactive)]

Comment on attachment 517305 [details] [diff] [review]
New version that fixes a typo

>diff --git a/mail/test/mozmill/shared-modules/test-dom-helpers.js b/mail/test/mozmill/shared-modules/test-dom-helpers.js

>+ * @param aElt The element to query.
>+ * @return Whether the element is visible or not.
>+ */
>+function element_visible (aElt) {

Remove the space after the function name please.

>+  let e;
>+  if (typeof aElt == "string") {
>+    e = mc.eid(aElt);
>+  } else {
>+    e = elt;

This should be aElt.

As discussed on IRC, you need to move the spelling suggestions to above undo/select all etc, similar to what Firefox does. Please file a bug for that.

Comment 13

[Jonathan Protzenko [:protz]]

checked-in http://hg.mozilla.org/comm-central/rev/d02ad4f0a6db