1556203 - Right click doesn't work any more in chat and dictionary choice is not used (TB 68 beta)

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Jorg K (CEST = GMT+2)]

I'm chatting on IRC maildev, I mistyped a word, the spellchecker tells me so, but when I try to use "right-click" to correct it, there is no right-click menu :-(

Fallout from bug 1519091 or bug 1521480?

Comment 1

[Jorg K (CEST = GMT+2)]

It's worse. The chosen dictionary (Tools > Options > Composition > Spelling) isn't used in chat any more. I switched that to German now and it still checks in en-US. That's pretty fatal.

I actually noticed typing "summarise" and that was flagged as an error despite using Marco's en-GB dictionary by default. Just now I realised that setting to German has no effect at all, so non-English speakers using an en-US version won't be happy at all.

Comment 2

[Jorg K (CEST = GMT+2)]

Looks like the "type in area" is a

<html:textarea xmlns:html="http://www.w3.org/1999/xhtml" anonid="inputBox" class="conv-textbox" flex="1" style="font-size: 16px; overflow-y: hidden;" spellcheck="true"></html:textarea>

If you add lang="de-DE" to it in the inspector, you get German spelling, no surprise. Somehow there is provision for chatConversationContextMenu, but it doesn't work.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-correct-spellcheck.patch

This sets the correct spellcheck dictionary, no idea how that worked without this code before. The patch isn't really chat specific, so I don't know who wants to review this. Would be good to get this into TB 68 beta 1 or else spelling won't work for non-English users of a non-localised version.

Also no idea where the context menu got lost, but that's another issue for Magnus to look at.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069306 [details] [diff] [review]
1556203-correct-spellcheck.patch

Or maybe Florian wants to see this since he reviewed bug 1519091 and bug 1521480. Those landed at TB 66 and checked that the right-click and correct spelling are already broken in TB 67 beta (and no one complained).

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: imconversation - left TB 68 - right TB 60.png

I've looked for the missing right-click menu in the Inspector, left TB 68, right TB 60.

TB 68 has a xul:stack with a html:textarea, TB 60 has a xul:stack with a xul:textbox which also contains a html:textarea and also a xul:popupmenu.
ESR 60 code here:
https://dxr.mozilla.org/comm-esr60/source/im/content/conversation.xml#57
Trunk code here:
https://searchfox.org/comm-central/rev/a1a23f8407df96699ab997e6bb619a09fe67e5db/mail/components/im/content/imconversation.xml#37

Looks like that xul:textbox somehow provided the context menu "for free" and we now need to graft it onto the textarea somehow.

Comment 6

[Magnus Melin [:mkmelin]]

Maybe related to https://hg.mozilla.org/mozilla-central/rev/71e5f2f8ca11 (bug 1531155)

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #4)

Or maybe Florian wants to see this since he reviewed bug 1519091 and bug
1521480. Those landed at TB 66 and checked that the right-click and correct
spelling are already broken in TB 67 beta (and no one complained).

Hmm, looking at
https://searchfox.org/comm-central/diff/932db4c2e4b4838ec8ce8acd3cfaa68b927926f1/mail/components/im/content/imconversation.xml#37
I blamed the wrong bugs and this regressed in bug 1532595.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch - WIP

Based on Magnus hit, it looks like we have to do launch the context menu ourselves now, à la:
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/toolkit/content/editMenuOverlay.js#62

This activates on right-click, now it's just a matter of constructing the right menu.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch

Hand-rolling out own context menu in either the XML of the JS file works.

Sadly the context menu only shows one item "Select all". Any idea what's going on there since the thing should be way bigger:
https://searchfox.org/comm-central/rev/a1a23f8407df96699ab997e6bb619a09fe67e5db/mail/components/im/content/chat-menu.inc.xul#29

BTW, what's the buddyListContextMenu, where should that be showing?

Comment 10

[Florian Quèze [:florian]]

Which context menu are other textareas getting in TB? Ideally that's the context menu we should open in that case.

The chatConversationContextMenu context menu is the menu we show when right clicking in the chat browser. The buddyListContextMenu context menu is the menu we show when right clicking on the left sidebar of the chat UI (ie. list of conversations and contacts).

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 9069306 [details] [diff] [review]
1556203-correct-spellcheck.patch

I don't remember all the details related to this, but it's largely a UX decision. Mostly the question is: when changing the spell check language from the context menu (yes, the one that's currently broken), should that change apply globally to everything else in Thunderbird, or only to that conversation?

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Florian Quèze [:florian] from comment #10)

Which context menu are other textareas getting in TB? Ideally that's the context menu we should open in that case.

Hmm, which other text areas? The compose window (message body) has a context which looks pretty standard, undo, cut, copy, paste, delete, select all, spelling, languages, but also "paste as quotation" and "paste without formatting". I can't tell right now where that comes from and where it's modified. I'm not aware of other "editing" context menus. Chat in TB 60 also has that menu, but without the added paste items.

EDIT: Comes from here:
https://searchfox.org/comm-central/rev/ba9e556dd988305051cd54ba1c6c13176e797f61/mail/components/compose/content/messengercompose.xul#609

The chatConversationContextMenu context menu is the menu we show when right clicking in the chat browser. The buddyListContextMenu context menu is the menu we show when right clicking on the left sidebar of the chat UI (ie. list of conversations and contacts).

I thought, the buddies/participants are on the right, where there is no context menu. But you're saying it's on the left. OK, there the context menu still works, but it only shows "Close Conversation", also in TB 60. That's a little surprising since there is more:
https://searchfox.org/comm-central/rev/a1a23f8407df96699ab997e6bb619a09fe67e5db/mail/components/im/content/chat-menu.inc.xul#7

(In reply to Florian Quèze [:florian] from comment #11)

Comment on attachment 9069306 [details] [diff] [review]
1556203-correct-spellcheck.patch
I don't remember all the details related to this, but it's largely a UX
decision. Mostly the question is: when changing the spell check language
from the context menu (yes, the one that's currently broken), should that
change apply globally to everything else in Thunderbird, or only to that
conversation?

Well, this is primarily about fixing the regression and the patch does this. Apparently the spellcheck functionality was more extensive in xul:textbox and we need to hand-roll more of this for the html:textarea.

I know all the details of spellchecking since I worked on all those bugs (I can provide details). There is a preference, spellchecker.dictionary, that determines the language in all new compose windows. Unlike in FF, when the user changes the language via the context menu, this preference is not affected, so the next window will be started with the preference value. Effectively you can write two or more messages in different languages concurrently, which I do occasionally.

Chat is different, it has the FF behaviour, you change the language in the chat, then the preference is changed. That may be unfortunate, we might want to make it more like the compose windows, that can be done, but in another bug. Right now, I'd like to fix the bustage, that the chosen dictionary is completely ignored. So any objections to the patch?

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Florian Quèze [:florian] from comment #10)

Which context menu are other textareas getting in TB? Ideally that's the context menu we should open in that case.

I right, I got confused. Different context menus for above the line (sent conversation) and below the line (type-in area). With the patch, the "above the line" context menu shows below the line. That's not what we want. Below the line we want the "standard" menu. I have to check where that comes from.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v3)

OK, wrapping the text area into a xul:moz-input-box gives us the "usual" context menu. Now I need to see how to activate the spellcheck items.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069473 [details] [diff] [review]
1556203-context-menu.patch (v3)

Brian, you wrote testbox.js. I have trouble working out how to activate the spellcheck parts of the context menu.

Please also take a look at the other patch. There you can see that we set "spellcheck" and "lang" on the enclosed html:textarea. That works, we can toggle the inline spellcheck on and off and set the language by changing the preferences. But the spellcheck part of the context menu isn't activated. So what's missing?

BTW, what is the `xbl:inherits="context,spellcheck"`, I've copied that from
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/toolkit/content/widgets/textbox.xml#24
and it's most likely wrong.

Comment 16

[Jorg K (CEST = GMT+2)]

Or maybe the spellcheck part is only supported if you use a <textbox>:
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/toolkit/content/widgets/textbox.js#173

Comment 17

[Jorg K (CEST = GMT+2)]

Brian, just as a background: that html:textarea used to be a xul:textarea and got changed here. I assume that's when the context menu was lost:
https://hg.mozilla.org/comm-central/rev/994cd1a152ed9637e38686000b0dc3409a80855f#l12.12

Magnus, why was that done? I have the impression that that change is wrong. All previous text boxes have lost their context menu, for example the task description in a Calendar task. Why did you remove all the <textbox>es in that bug?

They still exist in M-C:
https://searchfox.org/mozilla-central/search?q=%3Ctextbox&case=false&regexp=false&path=

Comment 18

[Brian Grinstead [:bgrins]]

FWIW, we are planning to migrate xul textboxes to html:input/textarea in bug 1513325 / bug 1547372, so restoring to a XUL textbox would likely be a temporary fix.

Comment 19

[Brian Grinstead [:bgrins]]

Are you including editMenuOverlay.js in these documents? That's where we are adding context menu support for plain inputs/textareas: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/toolkit/content/editMenuOverlay.js#62.

It doesn't support spellcheck but I would expect you to see the normal edit context menu at least.

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+2) from comment #16)

Or maybe the spellcheck part is only supported if you use a <textbox>:
https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/toolkit/content/widgets/textbox.js#173

Yeah, moz-input-box has only been used directly inside of a <xul:textbox> AFAIK so I wouldn't be surprised if it's not working stand-alone.

Comment 21

[Jorg K (CEST = GMT+2)]

Well, wrapping it in a xul:moz-input-box gives us the "usual" context menu, but not the spell part. I don't know whether we're including editMenuOverlay.js.
EDIT: We're not: https://searchfox.org/comm-central/search?q=editMenuOverlay.js&case=false&regexp=false&path=
I'm also not sure how you'd do it since TB chat isn't a XUL document in its own right.

But as I just discovered, the problem is bigger than chat. Many/all "text areas" have lost their context menu and some like Chat lost the spelling part of the context menu. As all this was already known and ignored :-( - Bug 1532595 comment #0.

OK, so if restoring a XUL textbox is not an option, we need to get the context menus back some other way and also the spelling parts.

Magnus, this is quite a big problem since it's all broken, including in TB 68 beta :-(

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+2) from comment #21)

Well, wrapping it in a xul:moz-input-box gives us the "usual" context menu, but not the spell part. I don't know whether we're including editMenuOverlay.js.
EDIT: We're not: https://searchfox.org/comm-central/search?q=editMenuOverlay.js&case=false&regexp=false&path=

OK, then I'd probably first try loading that, confirm it restores the normal edit menu, and then tackle adding spellcheck to the editMenuOverlay. Or if moz-input-box as a standalone works fine except for spellcheck, then add spellcheck support to a standalone moz-input-box.

I'm also not sure how you'd do it since TB chat isn't a XUL document in its own right.

What does that mean? What document is the chat loaded into?

Comment 23

[Jorg K (CEST = GMT+2)]

Hmm, I tried to include <script src="chrome://global/content/editMenuOverlay.js"/> in abCard.inc.xul to fix the missing context menu in the AB card notes, but I got this on right-click:
JavaScript error: chrome://global/content/editMenuOverlay.js, line 17: ReferenceError: goUpdateCommand is not defined

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to Brian Grinstead [:bgrins] from comment #22)

What does that mean? What document is the chat loaded into?

I let Magnus answer that. The sad part is that we need all this working in TB 68 beta which is already a week overdue.

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+2) from comment #23)

Hmm, I tried to include <script src="chrome://global/content/editMenuOverlay.js"/> in abCard.inc.xul to fix the missing context menu in the AB card notes, but I got this on right-click:
JavaScript error: chrome://global/content/editMenuOverlay.js, line 17: ReferenceError: goUpdateCommand is not defined

You'll have to load globalOverlay.js as well - we currently have the old separation that existed from the overlay days. Have considered folding those two files together as well, but for now you can just load them both.

Comment 26

[Jorg K (CEST = GMT+2)]

Thanks, Brian, that worked and I filed bug 1556582 to do that everywhere.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: backout.patch

Magnus and Richard, you broke this in bug 1532595. The transition to html:textarea was unnecessary at that point since xul:textbox still exists.

This patch is a backout the changes to imconversation.xml from that bug. With the backout, the chat context menu returns together with all the spelling features. We need this quickly for beta. EDIT: Some margin isn't right now, too much space above the entered text. Richard, can you please check that.

Going forward, you can revert that again and do a future-proof fix. But we can wait until M-C remove the xul:textbox for real and then hopefully also have a solution for the spell check.

Comment 28

[Richard Marti (:Paenglab)]

Comment on attachment 9069541 [details] [diff] [review]
backout.patch

It needs some more, if still possible with the actual de-XBL work. The textbox shows only one line centred in the box and doesn't break to a new line at the end.

Maybe something for a de-XBL specialist.

Comment 29

[Jorg K (CEST = GMT+2)]

OK, yes, I thought the margin was wrong, and I was happy to have the context menu back, so I didn't notice the missing wrapping. Can you fix those issues with CSS?

If we don't go back to xul:textbox, we'll have to hand-roll our own, like in attachment 9069306 [details] [diff] [review] and attachment 9069473 [details] [diff] [review], and then try to fix the spellcheck part of the context menu as suggested in comment #22. That's a bigger job and IMHO it would be better to wait until M-C get rid of xul:textbox in bug 1547372 and therefore have to provide the spellcheck menu somehow. I'd prefer not to do that work now.

Comment 30

[Richard Marti (:Paenglab)]

Multiline textbox does no more work since bug 1513343. It seems, the hand-roll approach is needed. :-(

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069361 [details] [diff] [review]
1556203-context-menu.patch

OK, un-obsoleting previous patches.

Comment 32

[Jorg K (CEST = GMT+2)]

OK, attachment 9069306 [details] [diff] [review] and attachment 9069473 [details] [diff] [review] restore the language choice and give some context menu, but now the text entry field is too narrow. We should try Brian's suggestion of adding <script src="chrome://global/content/editMenuOverlay.js"/> somewhere, but where? That still won't give us the spelling options.

Comment 33

[Magnus Melin [:mkmelin]]

For chat, you'd include editMenuOverlay.js in messenger.xul

But that doesn't in it self fix the problem. Due to the checks here, https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/toolkit/content/editMenuOverlay.js#67 our localName is imconversation meaning we don't get a context menu.
Seems de-xbl of imconversation (bug 1554633) would fix this issue. Not sure if it will give us the spell check in the context menu. Normal textareas in web pages do get one, so perhaps.

Comment 34

[Jorg K (CEST = GMT+2)]

So where does this leave us? Chat text entry is the second most visible text entry area in TB after the compose window, that is, for people who use TB chat. And it's currently broken and we're about to ship a beta and later ESR.

Sadly the breakage was known since bug 1532595 comment #0, so for about three months now[*]. I suggest you assign someone from the de-XBL team to fix this.

[*] Strangely no one noticed or complained despite shipping TB 67 beta broken. It took myself as a test rabbit for TB 68 beta to report the issue.

Comment 35

[Magnus Melin [:mkmelin]]

I think it doesn't need to block an initial beta. Like you say, nobody noticed for 67 beta either. But we'll do bug 1554633 to fix this.

Comment 36

[Jorg K (CEST = GMT+2)]

Maybe in the meantime we need to hand-roll the spelling and the popup, like in attachment 9069361 [details] [diff] [review].

It doesn't have to block the first beta, but it needs to be fixed for ESR.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069306 [details] [diff] [review]
1556203-correct-spellcheck.patch

Looks like we have to go with hand-rolling everything, I'll attach a patch for the menu soon.

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v4) - WIP

This gives a context menu, I still need to wire up the JS parts.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v5) - WIP

This is what I have so far, it's not working, inline spellcheck is totally misbehaving. You set it to enabled, query it, and it comes back disabled :-( - To be continued.

Comment 40

[Magnus Melin [:mkmelin]]

Bug 1556566 considers adding some api to modify the context menu

Comment 41

[Jorg K (CEST = GMT+2)]

Sure, but we really need something for beta. When I find the time, I'll get back to the patch.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v5b) - WIP

Not having any joy here. I create an inline spellchecker and enable it, and next time I query it, it's disabled. I set it to enabled again, and query it, and it comes back disabled.

STR: Enable inline spell checking in the compose options, start a chat. Spellcheck initialised to enabled. Then try the context menu and watch the debug :-(

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v6) - WIP

So this works somewhat. You can switch spelling on an off, there are dictionary choices, when you change the dictionary, the spelling is checked again.

Still to fix: When opening the menu again, dictionaries double up. Also, no spellcheck suggestions so far.

But we're getting there.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v7) - WIP

This works OK now, fixed the doubled-up dictionaries. Now the suggestions are missing and then it's good to go.

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v7) - WIP

Almost working. I removed the "ignore words" since the old chat context menu didn't have it.

Comment 46

[Jorg K (CEST = GMT+2)]

I need help here. This implements a context menu for our chat html:textarea:

https://searchfox.org/comm-central/rev/e7e026566431f1e2cd49d4e4dc2a5a90ad2c6c3d/mail/components/im/content/imconversation.xml#37

It's all working, apart from the spellcheck suggestions, see the patch:

+function openChatContextMenu(popup) {
+  let conv = chatHandler._getActiveConvView();
+  let spellchecker = conv.spellchecker;
+  let textbox = conv.editor;
+  
+  // The context menu uses gChatSpellChecker, so set it here for the duration of the menu.
+  gChatSpellChecker = spellchecker;
+
+  spellchecker.init(textbox.editor);
+  // In order to get spelling suggestion, the following function needs to be called, see
+  // InlineSpellChecker.initFromEvent. Debugging in the function shows that
+  // `spellsel.rangeCount` is not zero. However, no range is found in the end.
+  // Help!!
+  spellchecker.initFromEvent(textbox.ownerDocument.popupRangeParent,
+                             textbox.ownerDocument.popupRangeOffset);
+
+  let onMisspelling = spellchecker.overMisspelling;

As long as I pass non-null as parent into initFromEvent I can see that the misspelled words are found inside InlineSpellChecker.initFromEvent, however, due to the incorrect parameters, it ends up with getMisspelledWord(rangeParent, rangeOffset); returning null.

So the prize question is: What should be the parameters of initFromEvent. I tried a million things and none worked.

textbox.js uses document.popupRangeParent, document.popupRangeOffset.
https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/toolkit/content/widgets/textbox.js#121

Comment 47

[Brian Grinstead [:bgrins]]

Sorry, I don't really know anything about the spellchecker - when I migrated MozInputBox it was a direct translation of the old XBL code (https://hg.mozilla.org/mozilla-central/annotate/e123d0aa911c4fa3501042d2febecac1a2766b60/toolkit/content/widgets/textbox.xml).

What are the current return values of document.popupRangeParent and document.popupRangeOffset in the working case (one that uses textbox.js)? And what is "spellsel" is your code snippet referring to?

Comment 48

[Jorg K (CEST = GMT+2)]

Well, with my debug, I can see that in the functioning compose window, the first parameter is a text:

=== rangeParent [object Text]
=== spellsel.rangeCount 1
=== range thiss

That's the code:

  initFromEvent(rangeParent, rangeOffset) {
    dump(`=== rangeParent ${rangeParent}\n`);
    this.mOverMisspelling = false;

    if (!rangeParent || !this.mInlineSpellChecker)
      return;

    var selcon = this.mEditor.selectionController;
    var spellsel = selcon.getSelection(selcon.SELECTION_SPELLCHECK);
    dump(`=== spellsel.rangeCount ${spellsel.rangeCount}\n`);
    if (spellsel.rangeCount == 0)
      return; // easy case - no misspellings

    var range = this.mInlineSpellChecker.getMisspelledWord(rangeParent,
                                                          rangeOffset);
    dump(`=== range ${range}\n`);

I misspelled "thiss". So for chat, how do I get from the html:textarea, via some anomymous div to the text node? The spell checker knows that "thiss" is misspelled and wants to search it in the text. The textbox variable holds the html:textarea.

As for your question: I don't have a FF build which I could use to see those values. If I use document.popupRangeParent I get null.

Comment 49

[Jorg K (CEST = GMT+2)]

Actually, enhancing my debug, in the write window case I get:
=== rangeParent [object Text] 2
it gives the text I clicked together with the character position where I right-clicked to summon the menu.

Boris, can I trouble you with that sort of DOM question? Please read from comment #46 down. In a nutshell, I have a html:textarea and need to get the text node and the offset where I clicked into that node to pass into InlineSpellChecker.initFromEvent.

Comment 50

[Jorg K (CEST = GMT+2)]

Or maybe Geoff knows how to drill into the anonymous div to get to the text node. I'll try selection.getRangeAt(0).startContainer/Offset in a minute.

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v8)

This works now, InlineSpellChecker.initFromEvent now called with the editor selection. Not ideal, but better than nothing.

EDIT: All of the code was copied from here:
https://searchfox.org/comm-central/rev/e7e026566431f1e2cd49d4e4dc2a5a90ad2c6c3d/mail/components/compose/content/messengercompose.xul#610
https://searchfox.org/comm-central/rev/e7e026566431f1e2cd49d4e4dc2a5a90ad2c6c3d/mail/components/compose/content/MsgComposeCommands.js#1364

Comment 52

[Jorg K (CEST = GMT+2)]

Note that chat currently crashes, so if you want to test it, see bug 1559523 comment #1 for what needs to be backed out to make chat not crash.

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v8b)

I decided to get the context menu strings from Firefox, https://searchfox.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global/textcontext.dtd. Better then pulling in all the compose strings into the messenger.

Comment 54

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072364 [details] [diff] [review]
1556203-context-menu.patch (v8b)

Review of attachment 9072364 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/messenger.xul
@@ +56,5 @@
>  <!ENTITY % msgReadSMIMEDTD SYSTEM "chrome://messenger-smime/locale/msgReadSMIMEOverlay.dtd">
>  %msgReadSMIMEDTD;
>  <!ENTITY % msgViewPickerDTD SYSTEM "chrome://messenger/locale/msgViewPickerOverlay.dtd" >
>  %msgViewPickerDTD;
> +<!-- for the chat contect menu -->

Typo here. Will fix later.

Comment 55

[Jorg K (CEST = GMT+2)]

OK, I didn't try a million things, here's what I tried:

  dump(`=== document ${document}\n`);
  dump(`=== textbox.ownerDocument ${textbox.ownerDocument}\n`);
  dump(`=== textbox.editor.document ${textbox.editor.document}\n`);
  dump(`=== document ${document.popupRangeParent}\n`);
  dump(`=== document ${document.rangeParent}\n`);
  dump(`=== textbox.ownerDocument ${textbox.ownerDocument.popupRangeParent}\n`);
  dump(`=== textbox.ownerDocument ${textbox.ownerDocument.rangeParent}\n`);
  dump(`=== textbox.editor.document ${textbox.editor.document.popupRangeParent}\n`);
  dump(`=== textbox.editor.document ${textbox.editor.document.rangeParent}\n`);

gives

=== document [object XULDocument]
=== textbox.ownerDocument [object XULDocument]
=== textbox.editor.document [object XULDocument]
=== document null
=== document undefined
=== textbox.ownerDocument null
=== textbox.ownerDocument undefined
=== textbox.editor.document null
=== textbox.editor.document undefined

So whilst .popupRangeParent appears to be the right thing to use, it comes back null.

Comment 56

[Boris Zbarsky [:bzbarsky]]

I really don't know what the right thing to do is here. Maybe Masayuki does?

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

So the prize question is: What should be the parameters of initFromEvent. I tried a million things and none worked.

According to InlineSpellChecker, it sets UIEvent.rangeParent and UIEvent.rangeOffset.

They are not standardized event attributes... They are computed with nsLayoutUtils::GetContainerAndOffsetAtEvent() and nsLayoutUtils::GetEventCoordinatesRelativeTo(). According the latter, they are available only with pointing device events like MouseEvent, WheelEvent and touch device events like TouchEvent. And computed from their clientX and clientY. According to the former, they are the deepest DOM point at the place of the events. I.e., if the event is "contextmenu" caused by right click in a text node, rangeParent must be the text node and rangeOffset must the offset in text node at where the user clicked.

Comment 58

[Jorg K (CEST = GMT+2)]

Attachment: text-node-dom.png

Thanks, Masayuki-san, but as per my comment #55, .popupRangeParent comes back null and .rangeParent comes back undefined. The event I'm listening for is indeed "contextmenu". So there seems to be an issue in some M-C code. Maybe is doesn't see the right-click in the text node since the in the DOM we have the text buried deep inside some XBL element and an anonymous div. Looks like for now, we need to use the work-around getting the editor's selection :-(

Comment 59

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072364 [details] [diff] [review]
1556203-context-menu.patch (v8b)

Still experimenting.

Comment 60

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v9)

So .popupRangeParent/.pangeParent positively doesn't work, I even tried it in the "contextmenu" event listener. This is the best I can come up with, a giant hack to second-guess which misspelled word the user may be clicking on. I left the debug in place, so you can see.

Test cases: Type the following (between the |) and then right-click repeatedly onto the misspelled word:

|u |
|thiss |

On the first click, the div is hit, on the second click the text is hit and the debug shows "text 2". If you left-click into the text, everything works as expected with "text 1" showing in the debug.

Too bad we have to badly emulate missing core functionality in application code :-(

Comment 61

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu2.patch - Experiment, rebased on top of bug 1554633

I wanted to know whether cutting a little more through the de-XBL/XBL thicket by applying bug 1554633 first would help, so I rebased the patch onto of it. The result is that .rangeParent and friends still DO NOT WORK. So we're stuck with the hack for now.

Comment 62

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #57)

According to the former, they are the deepest DOM point at the place of the events. I.e., if the event is "contextmenu" caused by right click in a text node, rangeParent must be the text node and rangeOffset must the offset in text node at where the user clicked.

YES!! So sorry, I didn't use rangeParent on the event but on the document. I finally got it working. Thanks a lot.

Comment 63

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v10)

Finally it's fully working with Masayuki-san's help without any hack. The issue was to grab the RangeParent in the original event. It's not there any more in openChatContextMenu().

EDIT: Interdiff v9-v10 shows all the hackery that could be removed when doing it correctly:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9072429&action=interdiff&newid=9072466&headers=1

Comment 64

[:aceman]

Comment on attachment 9072466 [details] [diff] [review]
1556203-context-menu.patch (v10)

Review of attachment 9072466 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, it works great now in my tests.
Please fix the problems (like unknown objects) found by eslint (I do not list them all here).

::: mail/components/im/content/chat-menu.inc.xul
@@ +65,5 @@
> +             onpopupshowing="if (event.target != this) return true; openChatContextMenu(this);"
> +             onpopuphiding="if (event.target == this) { clearChatContextMenu(this); }">
> +
> +    <!-- Spellchecking menu items -->
> +    <menuitem id="spellCheckNoSuggestions" label="&spellNoSuggestions.label;" disabled="true"/>

Long line, please wrap.

@@ +67,5 @@
> +
> +    <!-- Spellchecking menu items -->
> +    <menuitem id="spellCheckNoSuggestions" label="&spellNoSuggestions.label;" disabled="true"/>
> +    <menuseparator id="spellCheckAddSep" />
> +    <menuitem id="spellCheckAddToDictionary" label="&spellAddToDictionary.label;" accesskey="&spellAddToDictionary.accesskey;"

Please wrap this, one attribute per line.

@@ +76,5 @@
> +    <menuitem label="&cutCmd.label;" accesskey="&cutCmd.accesskey;" command="cmd_cut"/>
> +    <menuitem label="&copyCmd.label;" accesskey="&copyCmd.accesskey;" command="cmd_copy"/>
> +    <menuitem label="&pasteCmd.label;" accesskey="&pasteCmd.accesskey;" command="cmd_paste"/>
> +    <menuseparator/>
> +    <menuitem label="&selectAllCmd.label;" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>

Please wrap too.

::: mail/components/im/content/chat-messenger.js
@@ +32,5 @@
> +  gChatSpellChecker = spellchecker;
> +
> +  spellchecker.init(textbox.editor);
> +  spellchecker.initFromEvent(gRangeParent, gRangeOffset);
> +  onMisspelling = spellchecker.overMisspelling;

Where is this variable declared?

@@ +743,5 @@
>          document.getElementById("contextPane").hidden = false;
> +        conv.addEventListener("contextmenu", (e) => {
> +          // Stash away the original event's parent and range for later use.
> +          gRangeParent = e.rangeParent;
> +          gRangeOffset = event.rangeOffset;

Should this be 'event' or 'e'?

Comment 65

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #64)

Long line, please wrap.

All the long lines were copied :-(

Where is this variable declared?

Shuffled this so many times, so the let got lost in the end :-(

Should this be 'event' or 'e'?

e but weirdly event also works. No idea where that comes from.

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: 1556203-context-menu.patch (v10b)

Review comments addressed.

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f72f5ad61b91
Fix context menu for chat by hand-rolling it, also use correct spellcheck dictionary. r=aceman

Comment 68

[Jorg K (CEST = GMT+2)]

TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/c07467a35921db59cb19f551e8c18d02e31f2da4

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/89ab13bca091
Follow-up: Attach contextmenu listener to input area, not entire conversation. r=me DONTBUILD

Comment 70

[Jorg K (CEST = GMT+2)]

Grrr, even the area above now had the hand-rolled menu. This takes care of this issue.
https://hg.mozilla.org/releases/comm-beta/rev/7a3d2579ef32b76c0b69fc1c8977058056783dbf

Comment 71

[:aceman]

(In reply to Pulsebot from comment #69)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/89ab13bca091
Follow-up: Attach contextmenu listener to input area, not entire
conversation. r=me DONTBUILD

Yes, that would make sense :) r=aceman