Right click doesn't work any more in chat and dictionary choice is not used (TB 68 beta)
Categories
(Thunderbird :: Instant Messaging, defect)
Tracking
(thunderbird68+ fixed, thunderbird69 fixed)
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file, 18 obsolete files)
11.33 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
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.
Assignee | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
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).
Assignee | ||
Comment 5•5 years ago
|
||
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•5 years ago
|
||
Maybe related to https://hg.mozilla.org/mozilla-central/rev/71e5f2f8ca11 (bug 1531155)
Assignee | ||
Comment 7•5 years ago
|
||
(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.
Assignee | ||
Comment 8•5 years ago
|
||
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.
Assignee | ||
Comment 9•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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?
Assignee | ||
Comment 12•5 years ago
•
|
||
(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?
Assignee | ||
Comment 13•5 years ago
|
||
(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.
Assignee | ||
Comment 14•5 years ago
|
||
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.
Assignee | ||
Comment 15•5 years ago
|
||
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.
Assignee | ||
Comment 16•5 years ago
|
||
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
Assignee | ||
Comment 17•5 years ago
|
||
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®exp=false&path=
Comment 18•5 years ago
•
|
||
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•5 years ago
|
||
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•5 years ago
|
||
(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.
Assignee | ||
Comment 21•5 years ago
•
|
||
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®exp=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•5 years ago
|
||
(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®exp=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?
Assignee | ||
Comment 23•5 years ago
|
||
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
Assignee | ||
Comment 24•5 years ago
|
||
(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•5 years ago
|
||
(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.
Assignee | ||
Comment 26•5 years ago
|
||
Thanks, Brian, that worked and I filed bug 1556582 to do that everywhere.
Assignee | ||
Comment 27•5 years ago
•
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 29•5 years ago
|
||
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•5 years ago
|
||
Multiline textbox does no more work since bug 1513343. It seems, the hand-roll approach is needed. :-(
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 31•5 years ago
|
||
Comment on attachment 9069361 [details] [diff] [review] 1556203-context-menu.patch OK, un-obsoleting previous patches.
Assignee | ||
Comment 32•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 34•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 36•5 years ago
|
||
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.
Assignee | ||
Comment 37•5 years ago
|
||
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.
Assignee | ||
Comment 38•5 years ago
|
||
This gives a context menu, I still need to wire up the JS parts.
Assignee | ||
Comment 39•5 years ago
|
||
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•5 years ago
|
||
Bug 1556566 considers adding some api to modify the context menu
Assignee | ||
Comment 41•5 years ago
|
||
Sure, but we really need something for beta. When I find the time, I'll get back to the patch.
Assignee | ||
Comment 42•5 years ago
|
||
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 :-(
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 43•5 years ago
|
||
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.
Assignee | ||
Comment 44•5 years ago
|
||
This works OK now, fixed the doubled-up dictionaries. Now the suggestions are missing and then it's good to go.
Assignee | ||
Comment 45•5 years ago
|
||
Almost working. I removed the "ignore words" since the old chat context menu didn't have it.
Assignee | ||
Comment 46•5 years ago
|
||
I need help here. This implements a context menu for our chat html:textarea:
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•5 years ago
|
||
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?
Assignee | ||
Comment 48•5 years ago
|
||
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.
Assignee | ||
Comment 49•5 years ago
|
||
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
.
Assignee | ||
Comment 50•5 years ago
•
|
||
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.
Assignee | ||
Comment 51•5 years ago
•
|
||
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
Assignee | ||
Comment 52•5 years ago
|
||
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.
Assignee | ||
Comment 53•5 years ago
|
||
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.
Assignee | ||
Comment 54•5 years ago
|
||
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.
Assignee | ||
Comment 55•5 years ago
|
||
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•5 years ago
|
||
I really don't know what the right thing to do is here. Maybe Masayuki does?
Comment 57•5 years ago
|
||
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 hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 62•5 years ago
|
||
(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 andrangeOffset
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.
Assignee | ||
Comment 63•5 years ago
•
|
||
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•5 years ago
|
||
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="©Cmd.label;" accesskey="©Cmd.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'?
Assignee | ||
Comment 65•5 years ago
•
|
||
(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.
Assignee | ||
Comment 66•5 years ago
|
||
Review comments addressed.
Comment 67•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f72f5ad61b91
Fix context menu for chat by hand-rolling it, also use correct spellcheck dictionary. r=aceman
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 68•5 years ago
|
||
TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/c07467a35921db59cb19f551e8c18d02e31f2da4
Comment 69•5 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/89ab13bca091 Follow-up: Attach contextmenu listener to input area, not entire conversation. r=me DONTBUILD
Assignee | ||
Comment 70•5 years ago
|
||
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•5 years ago
|
||
(In reply to Pulsebot from comment #69)
Pushed by mozilla@jorgk.com:
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
Description
•