Last Comment Bug 623590 - Text field context menu broken [spell-check-enabled menuitem missing]
: Text field context menu broken [spell-check-enabled menuitem missing]
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-06 06:56 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-01-08 10:54 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.23 KB, patch)
2011-01-06 07:38 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 [Checkin: comment 7] (2.25 KB, patch)
2011-01-08 04:39 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2011-01-06 06:56:08 PST
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) 2011-01-06 07:38:39 PST
Created attachment 501664 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-01-06 13:17:17 PST
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) 2011-01-06 13:38:03 PST
(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 neil@parkwaycc.co.uk 2011-01-07 16:44:02 PST
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) 2011-01-08 04:39:47 PST
Created attachment 502222 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-01-08 07:45:42 PST
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) 2011-01-08 10:54:10 PST
Comment on attachment 502222 [details] [diff] [review]
patch v2 [Checkin: comment 7]

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

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