1591506 - Can no longer cancel a connecting chat account

Status: VERIFIED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Patrick Cloke [:clokep]]

In TB 71b1 it is no longer possible to cancel a connecting account.

When a chat account is connecting right clicking should offer a "cancel" option. This option disappears again once the account is connected.

Looking through the code briefly (https://searchfox.org/comm-central/rev/a58553160c7536c98db48a12eddbef96f9b274b3/mail/components/im/content/imAccounts.js#453-458) it seems that all the "hidden" options are never shown.

Comment 1

[Patrick Cloke [:clokep]]

I thought this might be another regression from bug 1570954, but I don't think it is (I reverted it and the behavior is the same).

Comment 2

[Patrick Cloke [:clokep]]

Khushil, do you know if this worked after de-XBLing these bindings in bug 1531312?

Comment 3

[Patrick Cloke [:clokep]]

Rob confirmed that TB68 only has the two options to "Copy Debug Log" and "New Account". I think this might be a regression from bug 1531312 then.

Comment 4

[Jorg K (CEST = GMT+2)]

I'd say it's de-XBL fallout just like bug 1591505.

You need insider knowledge to know that you can drag account around and that there is meant to be a more sophisticated context menu in the "Show accounts" dialogue. Khushil, you can check it on TB 60. That said, I didn't see a "Cancel" option there, just a "Connect"/"Disconnect", but perhaps I connected too quickly.

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1591506_chat-account-contextmenu.patch

Comment 6

[Jorg K (CEST = GMT+2)]

Nice, the larger menu is back, but "cancel" isn't on it. I guess that was just an incorrect memory.

Comment 7

[Patrick Cloke [:clokep]]

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

Nice, the larger menu is back, but "cancel" isn't on it. I guess that was just an incorrect memory.

I believe you can only cancel a connection when it is reconnecting. I've usually tested this by being connected and then disconnecting from the Internet (i.e. shutting off my WiFi adapter). The connections should drop pretty quickly and Thunderbird will attempt to reconnect. It will fail (since you're not connected to the Internet...), but right clicking should show an option to cancel attempting to reconnect.

Comment 8

[Jorg K (CEST = GMT+2)]

I stand corrected. Under certain circumstances "Cancel reconnection" is shown on the context menu:
https://searchfox.org/comm-central/rev/50ac80fa95642644080f7b13b6073d8900d931dc/mail/components/im/content/imAccounts.xul#54
I saw it happening with the patch.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9104433 [details] [diff] [review]
Bug-1591506_chat-account-contextmenu.patch

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

::: chat/content/chat-account-richlistitem.js
@@ +54,5 @@
>  
> +      this.addEventListener(
> +        "contextmenu",
> +        event => {
> +          document.accountItem = this;

I don't think this is right. It's essentially adding a magic global.

I also don't understand why it's needed. Please as much as possible explain the changes in bugzilla at least.

::: mail/components/im/content/imAccounts.js
@@ +442,5 @@
>        }
>      }
>    },
>    onContextMenuShowing() {
> +    let targetElt = document.accountItem;

Why doesn't document.popupNode work?

Or maybe you want to add the event in the onContextMenuShowing call
https://searchfox.org/comm-central/rev/e56c0a86aa6c1c32badf0f9be18a9df37820993f/mail/components/im/content/imAccounts.xul#90 like gAccountManager.onContextMenuShowing(event)

Comment 10

[Khushil Mistry [:khushil324]]

With document.popupNode, it is taking internal elements of richlistitem like label, vbox and buttons depending upon whereever you click. We want richlistitem in document.popupNode. Do you know any other way?

Comment 11

[Magnus Melin [:mkmelin]]

You probably need to look it up by using popupNode.closest("chat-account-richlistitem")?

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1591506_chat-account-contextmenu.patch

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9104888 [details] [diff] [review]
Bug-1591506_chat-account-contextmenu.patch

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

::: mail/components/im/content/imAccounts.js
@@ +444,5 @@
>    },
>    onContextMenuShowing() {
> +    let targetElt = document.popupNode.closest("richlistitem");
> +    let isAccount =
> +      targetElt && targetElt.getAttribute("is") === "chat-account-richlistitem";

to avoid the extra, code just do

let targetElt = document.popupNode.closest('richlistitem[is="chat-account-richlistitem"]');

I'd expect there is no need to null check it either?

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: Bug-1591506_chat-account-contextmenu.patch

We need a null check as when we click on richlistbox(out of the richlistitems), targetElt will be null. So we need a check for targetElt directly which I have changed here.

Comment 15

[Jorg K (CEST = GMT+2)]

Seems to be working fine in my test.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 9104915 [details] [diff] [review]
Bug-1591506_chat-account-contextmenu.patch

I'll do a beta build now, so it will land there first. Landing on C-C in a few hours.

Comment 17

[Jorg K (CEST = GMT+2)]

TB 71 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/bc2d37427ba59cc2f93d32b9b2719b12f6f45d41

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a2c9e5e752b1
Fixed context menu in chat's IM status dialog. r=mkmelin

Comment 19

[Jorg K (CEST = GMT+2)]

TB 68.2.1 ESR:
https://hg.mozilla.org/releases/comm=esr68/rev/ca6503e6cab1f694b326a495575fa4797418854a

Comment 20

[Jorg K (CEST = GMT+2)]

Sadly this doesn't work in TB 68. Khushil, can you please take a look. As you know, TB 68 is a sad mix of XBL and de-XBL, I had to back out a few de-XBL bugs to make it work, see bug 1555136 comment #16 (backout of bug 1538548) and bug 1555136 comment #21 (backout of bug 1538550 and bug 1538549). Or maybe ESR 68 doesn't have chat-account-richlistitem and hence

+    let targetElt = document.popupNode.closest(
+      'richlistitem[is="chat-account-richlistitem"]'
+

won't yield a result?

Comment 21

[Khushil Mistry [:khushil324]]

Ohh, I haven't checked it on ESR68. I am submitting a patch for ESR68 ASAP.

Comment 22

[Jorg K (CEST = GMT+2)]

Maybe based on what I have already landed, or if it's totally different, I can also back that out. We'll see.

Comment 23

[Khushil Mistry [:khushil324]]

Attachment: Bug-1591506_chat-account-contextmenu_ESR68.patch

We just need to add this.setAttribute("is", "chat-account-richlistitem") in chat-account-richlistitem.
We have landed a patch for changes in Richlistitems on Trunk and this thing was added there.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105312 [details] [diff] [review]
Bug-1591506_chat-account-contextmenu_ESR68.patch

Somehow BMO doesn't recognise patches anymore :-(

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1591506-follow-up-esr68.patch

Thanks, this adds the additional line since the rest has already been landed and there's no point in backing it out. I'll land it with the next lot.

Comment 26

[Jorg K (CEST = GMT+2)]

I forgot to say: Tested by unpacking omni.ja of the 68.2.1 pre-release.

Comment 27

[Khushil Mistry [:khushil324]]

You will back out the previous patch for ESR68, right?

Comment 28

[Jorg K (CEST = GMT+2)]

No, why would I? All I need it to add one line. That's what I said in comment #25.

Comment 29

[Jorg K (CEST = GMT+2)]

TB 68.2.1 ESR (follow-up):
https://hg.mozilla.org/releases/comm-esr68/rev/f5af652ef843d4d9153ee76fa0686f9b19b552f3

Comment 30

[Jorg K (CEST = GMT+2)]

It's working now in the 68.2.1 build.