623590 - Text field context menu broken [spell-check-enabled menuitem missing]

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Neil found the following in bug 332538 comment 12:

STR:
1. Create an HTML file with a text box (in a form).
2. Create a new mail, attach the HTML file, save the mail.
3. Go to the Drafts folder and view the mail.
4. Do a right-click on the text box in the mail to invoke the context menu.

Result:
  Error: document.getElementById("spell-check-enabled") is null
  Source File: chrome://communicator/content/nsContextMenu.js
  Line: 287

Due to the error, the context menu is broken (shows all entries, irrespective of any disabled states).

Reason: The spell-check-enabled menuitem is only defined in contentAreaContextOverlay.xul, which seems to not be available in this context.

Possible fix: Do not assume the element exists in nsContextMenu.js, or provide a safe fall-back.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Since this is not in MailNews code, and for the sake of speed, requesting both r+sr from Neil. Karsten, feel free to steal r if you like.

Comment 2

[[e-mail]]

Comment on attachment 501664 [details] [diff] [review]
patch

>+    if (canSpell && menu)
>       document.getElementById("spell-check-enabled")
>               .setAttribute("checked", InlineSpellCheckerUI.enabled);
Rather than have all of those menu checks all over, can we not use this.setItemAttr here instead? (In case we add spell check to mail tabs?)

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #2)
> >+    if (canSpell && menu)
> >       document.getElementById("spell-check-enabled")
> >               .setAttribute("checked", InlineSpellCheckerUI.enabled);
> Rather than have all of those menu checks all over, can we not use
> this.setItemAttr here instead? (In case we add spell check to mail tabs?)

It works for this one case (so this is option 2, mine is option 1), yes, but I tried to fix the others, too. For example the second case needs "menu" to work, but also "suggestionsSeparator". The third needs "dictMenu" and "dictSep". My patch makes one check and is safe then (since the check is against the source of all those menu items), with the cost of needing a rewrite if anyone not including contentAreaContextOverlay.xul (like MailNews) wants any of that, too. Probably best would be to check all required menu items as needed (option 3). You choose.

Comment 4

[[e-mail]]

Comment on attachment 501664 [details] [diff] [review]
patch

>       document.getElementById("spell-check-enabled")
>               .setAttribute("checked", InlineSpellCheckerUI.enabled);
OK, so this should use setItemAttr...

>-      var menu = document.getElementById("contentAreaContextMenu");
>+    if (onMisspelling && menu) {
But I'm not sure about this. Maybe it should be using this.menu instead?

>+    if (canSpell && menu) {
>       var dictMenu = document.getElementById("spell-dictionaries-menu");
IMHO this should be null-checking dictMenu instead.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2 [Checkin: comment 7]

(In reply to comment #4)
> >-      var menu = document.getElementById("contentAreaContextMenu");
> >+    if (onMisspelling && menu) {
> But I'm not sure about this. Maybe it should be using this.menu instead?

Not really. this.menu is valid in the MailNews case as well, but not the menuitems that are used there. If I use this.menu, I get the following in the error console:

Error: menu is null
Source File: resource://gre/modules/InlineSpellChecker.jsm
Line: 146

Additional STR for the above (in addition to comment 0):
1. set layout.spellcheckDefault to 2 (spell check text boxes)
2. type some invalid word into the text box in the mail contents and possibly a space to trigger the squiggly line
3. right click on the invalid word

Comment 6

[[e-mail]]

Comment on attachment 501664 [details] [diff] [review]
patch

>       var numsug = InlineSpellCheckerUI.addSuggestionsToMenu(menu, suggestionsSeparator, 5);
You'd probably need to change this menu to this.menu too.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 502222 [details] [diff] [review]
patch v2 [Checkin: comment 7]

http://hg.mozilla.org/comm-central/rev/d93907368195