Closed Bug 1592422 Opened 5 years ago Closed 5 years ago

Tooltips no longer show avatars

Categories

(Thunderbird :: Instant Messaging, defect)

Product:
Thunderbird
Component:
Instant Messaging
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird72 fixed, thunderbird73 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 73.0
Tracking Flags:
Tracking Status
thunderbird72 --- fixed
thunderbird73 --- fixed

People

(Reporter: clokep, Assigned: khushil324)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

Bug-1592422_chat-tooltip-avatars-fix-3.patch
(deleted), patch
mkmelin
: review+
mkmelin
: approval-comm-beta+
Details | Diff | Splinter Review

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.

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

Flags: needinfo?(khushil324)
Keywords: regression
Assignee: nobody → khushil324
Flags: needinfo?(khushil324)
Attached patch Bug-1592422_chat-tooltip-avatars-fix.patch (obsolete) (deleted) — — Splinter Review
Attachment #9109638 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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 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
Attachment #9109638 - Flags: review?(mkmelin+mozilla)

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

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?

Flags: needinfo?(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.

Flags: needinfo?(clokep)

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

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)
Attached patch Bug-1592422_chat-tooltip-avatars-fix-1.patch (obsolete) (deleted) — — Splinter Review
Attachment #9109638 - Attachment is obsolete: true
Attachment #9113721 - Flags: review?(mkmelin+mozilla)

(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; }

Attachment #9113721 - Flags: feedback?(clokep)

(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.

Attachment #9113721 - Flags: review?(mkmelin+mozilla)

(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.

Attached patch Bug-1592422_chat-tooltip-avatars-fix-2.patch (obsolete) (deleted) — — Splinter Review
Attachment #9113721 - Attachment is obsolete: true
Attachment #9113721 - Flags: feedback?(clokep)
Attachment #9114528 - Flags: review?(mkmelin+mozilla)
Attachment #9114528 - Attachment is patch: true
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
Attachment #9114528 - Flags: review?(mkmelin+mozilla)
Attachment #9114528 - Attachment is obsolete: true
Attachment #9115750 - Flags: review?(mkmelin+mozilla)
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?
Attachment #9115750 - Flags: review?(mkmelin+mozilla) → review+

(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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/2e35cc63542c
Fixed chat-tooltips no longer show avatars. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 73.0
Attachment #9115750 - Flags: approval-comm-beta+

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

Target Milestone: Thunderbird 73.0 → Thunderbird 72.0
status-thunderbird72: --- → fixed
Target Milestone: Thunderbird 72.0 → Thunderbird 73.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: