The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

Trunk
seamonkey2.1b2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #501664 - Flags: superreview?(neil)
Attachment #501664 - Flags: review?(neil)

Comment 2

6 years ago
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?)
(Assignee)

Comment 3

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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
Attachment #501664 - Attachment is obsolete: true
Attachment #502222 - Flags: superreview?(neil)
Attachment #502222 - Flags: review?(neil)
Attachment #501664 - Flags: superreview?(neil)
Attachment #501664 - Flags: review?(neil)

Comment 6

6 years ago
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.

Updated

6 years ago
Attachment #502222 - Flags: superreview?(neil)
Attachment #502222 - Flags: superreview+
Attachment #502222 - Flags: review?(neil)
Attachment #502222 - Flags: review+
(Assignee)

Comment 7

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

http://hg.mozilla.org/comm-central/rev/d93907368195
Attachment #502222 - Attachment description: patch v2 → patch v2 [Checkin: comment 7]
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
You need to log in before you can comment on or make changes to this bug.