1592422 - Tooltips no longer show avatars

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Patrick Cloke [:clokep]]

STR:

1. Connect to an XMPP account
2. Hover over a user who has an avatar set
3. See that there's no user icon in the top left of the tooltip

I'm fairly certain this is a regression from bug 1506529.

Comment 1

[Patrick Cloke [:clokep]]

Khushil, do you know if this worked after converting the tooltip to a custom element?

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1592422_chat-tooltip-avatars-fix.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9109638 [details] [diff] [review]
Bug-1592422_chat-tooltip-avatars-fix.patch

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

::: chat/content/chat-tooltip.js
@@ +324,5 @@
>        let displayName = aBuddy.displayName;
>        this.setAttribute("displayname", displayName);
>        let account = aBuddy.account;
>        this.setAttribute("iconPrpl", account.protocol.iconBaseURI + "icon.png");
> +      this.setAttribute("userIcon", aBuddy.buddyIconFilename);

so how is "no buddyIconFilename" handled now?

I see the improvement. But I also (still) get a
JavaScript error: chrome://chat/content/chat-tooltip.js, line 320: TypeError: aAccount.prplAccount is undefined

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9109638 [details] [diff] [review]
Bug-1592422_chat-tooltip-avatars-fix.patch

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

::: chat/content/chat-tooltip.js
@@ +324,5 @@
>        let displayName = aBuddy.displayName;
>        this.setAttribute("displayname", displayName);
>        let account = aBuddy.account;
>        this.setAttribute("iconPrpl", account.protocol.iconBaseURI + "icon.png");
> +      this.setAttribute("userIcon", aBuddy.buddyIconFilename);

so how is "no buddyIconFilename" handled now?

I see the improvement. But I also (still) get a
JavaScript error: chrome://chat/content/chat-tooltip.js, line 320: TypeError: aAccount.prplAccount is undefined

Comment 5

[Richard Marti (:Paenglab)]

Not sure if it's related but I see when I hover over the #maildev tab that some text overwrites other text.

Comment 6

[Khushil Mistry [:khushil324]]

On Trunk, right now, no tooltip is shown. Also, the contextmenu for chat-imconv, chat-contact and chat-group is also not shown. Patrick, do you have any idea or any bug filed for this issue?

Comment 7

[Patrick Cloke [:clokep]]

Khushil, last I checked these worked fine. I haven't had time to make a build in a while though. I think the last bug filed on this was bug 1591357. I haven't been tracking another bug about this.

Comment 8

[Khushil Mistry [:khushil324]]

Magnus, can you check on the trunk? Is this regression for bug 1591357?

Comment 9

[Magnus Melin [:mkmelin]]

No, a regression from bug 1597131. I attached a fix there.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1592422_chat-tooltip-avatars-fix-1.patch

Comment 11

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #4)

so how is "no buddyIconFilename" handled now?

If aBuddy.buddyIconFilename is null then userIcon will be also null. And we have this in inheritedAttributes: ".userIcon": "src=userIcon"
Because userIcon is null, src will not be set and we have CSS: .userIcon:not([src]) { display: none; }

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to Khushil Mistry [:khushil324] from comment #11)

If aBuddy.buddyIconFilename is null then userIcon will be also null. And we have this in inheritedAttributes: ".userIcon": "src=userIcon"
Because userIcon is null, src will not be set and we have CSS: .userIcon:not([src]) { display: none; }

The tooltip element is reused, so if a previous tooltip had src, setting to null would not remove src, no? I think you need to conditionally remove the userIcon attribute.

Comment 13

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #12)

The tooltip element is reused, so if a previous tooltip had src, setting to null would not remove src, no? I think you need to conditionally remove the userIcon attribute.

I misunderstood it. Now in updateTooltipFromConversation, we are removing userIcon attribute. There is a linting error in the current patch. I will resubmit it now.

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: Bug-1592422_chat-tooltip-avatars-fix-2.patch

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9114528 [details] [diff] [review]
Bug-1592422_chat-tooltip-avatars-fix-2.patch

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

Looks like the same patch as earlier?

For the conditionally removing, consider e.g. that aParticipant.buddyIconFilename could be null

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1592422_chat-tooltip-avatars-fix-3.patch

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9115750 [details] [diff] [review]
Bug-1592422_chat-tooltip-avatars-fix-3.patch

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

::: chat/content/chat-tooltip.js
@@ +430,5 @@
>        this.setAttribute("status", "unknown");
>        this.setMessage(Status.toLabel("unknown"));
> +      if (
> +        aParticipant.buddyIconFilename ||
> +        aParticipant.buddyIconFilename == ""

"" should count as not set, I'd think?

Comment 18

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #17)

"" should count as not set, I'd think?

No, this means protocol supports buddy-icon but the user has not set yet. So one needs to show default buddy-icon. We are doing it here: https://searchfox.org/comm-central/source/mail/components/im/themes/chat.css#323

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2e35cc63542c
Fixed chat-tooltips no longer show avatars. r=mkmelin

Comment 20

[Geoff Lankow (:darktrojan)]

72.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/6a0479402139ab109836d93f61bc87e227e2680d