Refresh the code in nsContextMenu a little bit

RESOLVED FIXED in Thunderbird 39.0


Mail Window Front End
3 years ago
3 years ago


(Reporter: protz, Assigned: protz)


Thunderbird 39.0

Thunderbird Tracking Flags

(thunderbird38+ fixed)



(1 attachment)



3 years ago
Created attachment 8519627 [details] [diff] [review]

This is a very minor rewrite of the spellcheck-related code in nsContextMenu. I just use the new SpellCheckHelper object to determine whether an element is editable, and wheter it is a text field or a content-editable element. I also add a little bit of code to enable spell checking for content-editable elements, which was previously missing.

I'm essentially copying a few lines from then a few more lines from

(There's a reason why I'm doing this, of course. Since I switched Conversations' inline reply to a contenteditable iframe, spell checking is no longer available in the context menu.)

:mconley, feel free to suggest another reviewer. I initially wanted to bother Mark, but since he's cutting back on his involvement with Thunderbird, I thought it would be appropriate to bother someone else!
Attachment #8519627 - Flags: review?(mconley)

Comment 1

3 years ago
Ran a series of tests on try to make sure this doesn't break anything
Comment on attachment 8519627 [details] [diff] [review]

Review of attachment 8519627 [details] [diff] [review]:

Thanks protz!

::: mail/base/content/nsContextMenu.js
@@ +480,2 @@
>          if (! {
>            this.onEditableArea = true;

Is this still necessary, since we've already set onEditableArea above?
Attachment #8519627 - Flags: review?(mconley) → review+

Comment 3

3 years ago
Sorry about the terrible, terrible delay in replying to this.

That's a good point, however, for this assignment to be redundant, the test:

      } else if (editFlags & (SpellCheckHelper.INPUT | SpellCheckHelper.TEXTAREA)) {

has to imply that (editFlags & SpellCheckHelper.EDITABLE) was nonzero in the first place, per the original assignment:

    this.onEditableArea = (editFlags & SpellCheckHelper.EDITABLE) !== 0;

editFlags is defined as follows:

    let editFlags = SpellCheckHelper.isEditable(, window);

so we have to read the definition of editFlags to see if we can have editFlags & INPUT (or editFlags & TEXTAREA) while not having editFlags & EDITABLE

Reading, and from a cursory glance, it seems that there *may* be such codepaths. I mean, I don't know the exact semantics of:

461       if (element.mozIsTextField(false) || element.type == "number") {

but I know that I'm not understanding the full picture, so I'd don't feel confident getting read of this assignment that was in the original Firefox code, too...

If that's ok with you, please do check-in this patch, I just checked, it applies cleanly.

Thanks, and apologies again for the delay.

(I'll be asking for approval-aurora immediately after, this really needs to be in the next esr, as it breaks spell-checking in Thunderbird Conversations, which is one of the top bugs that have been reported lately.)




3 years ago
Keywords: checkin-needed

Comment 4

3 years ago
Comment on attachment 8519627 [details] [diff] [review]

Uplift to comm-aurora after a nightly cycle.
Attachment #8519627 - Flags: approval-comm-aurora?


3 years ago
Last Resolved: 3 years ago
tracking-thunderbird38: --- → +
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0

Comment 5

3 years ago
Comment on attachment 8519627 [details] [diff] [review]
Attachment #8519627 - Flags: approval-comm-aurora? → approval-comm-aurora+


3 years ago
status-thunderbird38: --- → fixed
You need to log in before you can comment on or make changes to this bug.