Change the appearance of the chat button instead of reopening the chat tab when receiving a message

RESOLVED FIXED in Thunderbird 16.0

Status

Thunderbird
Instant Messaging
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

Trunk
Thunderbird 16.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird15 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Reopening the chat tab automatically is apparently too obtrusive for email users. It was suggested that we do something with the chat button instead.

I'm closing bug 740358 as I checked-in the fix that's attached there, so I'm opening this new bug to further discuss and implement this proposed additional improvement.

See comment 1 to comment 9 in bug 740358 for more details.
(Assignee)

Comment 1

5 years ago
Created attachment 628432 [details] [diff] [review]
WIP

This patch contains the required code changes to stop reopening the chat tab automatically, and to set an attribute on the chat toolbar button when there are unread messages directed to the user (either private messages, or messages containing the user's nick).

For the chat button, the current patch sets the "checked" attribute to "true" when there are unread messages, as it's the only thing that I found that causes a difference of appearance without any theming changes. The attribute name should probably be changed ("highlight"? "unread"?) once Andreas has some CSS changes to go with this patch. (It can trivially be edited in the _updateChatButtonState method. It would also be trivial to have the number of unread messages in the attribute; if there's a way to display that inside the button and it's wanted).
Assignee: nobody → florian
Attachment #628432 - Flags: ui-review?(bwinton)
Attachment #628432 - Flags: feedback?(nisses.mail)
Comment on attachment 628432 [details] [diff] [review]
WIP

On Mac: 

- I think we want the icon to be highlighted, too.  Currently just the text is.

- How hard would it be to change the label of the button to "Chat (1)" when I had one incoming message?


On Linux:

- The button looks pressed in, which I'm really not a fan of.  I think we need to do something explicitly different here.


On Windows:

- My Windows builds are still broken, so no ui comments from me for this, I'm sorry to say.  :(  Perhaps Andreas will post a screenshot…


So, given the Linux behaviour, I think I'm going to say ui-r-, but I bet we could use a different attribute, and get Andreas to do some fancier styling…  ;)

Thanks,
Blake.
Attachment #628432 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 3

5 years ago
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #2)

> - How hard would it be to change the label of the button to "Chat (1)" when
> I had one incoming message?

Relatively trivial, but I'm afraid it would increase the width of the Chat button, and make all the other buttons after it move.

> So, given the Linux behaviour, I think I'm going to say ui-r-, but I bet we
> could use a different attribute, and get Andreas to do some fancier styling…
> ;)

That (getting Andreas to polish the appearance) is my plan! :)
What about doing font-weight bold on the linux label and adding a flare to the icon just like we do for mail folders with new messages in them?
(Assignee)

Comment 5

5 years ago
(In reply to Andreas Nilsson (:andreasn) from comment #4)
> What about doing font-weight bold on the linux label and adding a flare to
> the icon just like we do for mail folders with new messages in them?

That idea is OK with me.
Created attachment 631841 [details] [diff] [review]
WIP (v2)

With new graphics for pinstripe, gnomestripe and qute (non-aero).
Created attachment 631858 [details] [diff] [review]
WIP (v3)

Sets #button-chat[unreadMessages="true"] in the themes so we can do stuff with the icons. Still need to work out the text styling on aero and pinstripe.
Attachment #631841 - Attachment is obsolete: true
note that this patch needs the patch in #727598 applied first
Created attachment 631876 [details]
this is what it currently looks on Aero (screenshot)
Created attachment 631929 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac
Attachment #631858 - Attachment is obsolete: true
Attachment #631929 - Flags: ui-review?(bwinton)
Attachment #631929 - Flags: review?(florian)
(Assignee)

Comment 11

5 years ago
Comment on attachment 631929 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac

Andreas, I tested this on Mac only. I like the new icon, and your CSS changes seem OK to me, so the r+ is for these parts of the patch.

I found some issues with the JS code changes I proposed before in attachment 628432 [details] [diff] [review] and anyway, I wrote that part so I couldn't r+ it.

I'll attach an updated version of that part of the patch in a separate attachment and request a code review on it.
Attachment #631929 - Flags: review?(florian) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 632746 [details] [diff] [review]
Patch (code changes)
Attachment #628432 - Attachment is obsolete: true
Attachment #628432 - Flags: feedback?(nisses.mail)
Attachment #632746 - Flags: review?(bwinton)
(Assignee)

Comment 13

5 years ago
(In reply to Florian Quèze from comment #12)
> Created attachment 632746 [details] [diff] [review]
> Patch (code changes)

This patch removes the 2 more or less broken pieces of code that used to change automatically the selection in the left pane.
I mentioned already in bug 735292 comment 7 that I wasn't fully satisfied with it. The issues became more obvious here as clicking the chat button displayed the chat tab but without selecting the conversation with unread messages; so the unread indicator on the chat button wasn't removed.

For this new patch I wrote a better algorithm to select something that makes when the Chat tab becomes visible. I added lots of comments in the code, so I'm not going to details what it does here.
(Assignee)

Updated

5 years ago
Attachment #632746 - Flags: ui-review?(bwinton)
Comment on attachment 631929 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac

On Mac:  I like it, but it seemed to remain highlit even after I viewed the email.  How am I supposed to clear out the highlight?

On Windows Aero: The highlit version of the Chat icon seems too dark, particularly in the default theme, since everything is slightly blue.

On Linux: The bold seems a little subtle, but I'm not really sure what else we could do there that would still fit in.  I also like the tiny yellow star, but I think that giving it a bit of a darker edge might help it be more recognizable.  (Since it's on a background of white and light grey…)

Other than those tweaks, I like it, so I guess ui-r=me.
Attachment #631929 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 632746 [details] [diff] [review]
Patch (code changes)

(Carrying forward the ui-review from the previous patch, since this is the patch I actually ran with…)

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -228,36 +223,66 @@ var chatHandler = {
>+  _updateChatButtonState: function() {
>+    delete this._chatButtonUpdatePending;
>+    let chatButton = document.getElementById("button-chat");
>+    if (!chatButton)
>+      return;
>+    if (this.countUnreadMessages())
>+      chatButton.setAttribute("unreadMessages", "true");
>+    else
>+      chatButton.removeAttribute("unreadMessages");

What do you think about doing:
  chatButton.setAttribute("unreadMessages", this.countUnreadMessages());
instead, and getting rid of the "if"?

Other than that (and the things I asked for in the previous patch), I'm pretty happy with it.  r=me!

Thanks,
Blake.
Attachment #632746 - Flags: ui-review?(bwinton)
Attachment #632746 - Flags: ui-review+
Attachment #632746 - Flags: review?(bwinton)
Attachment #632746 - Flags: review+
(Assignee)

Comment 16

5 years ago
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)
> Comment on attachment 631929 [details] [diff] [review]
> patch for linux, windows (xp+aero) and mac
> 
> On Mac:  I like it, but it seemed to remain highlit even after I viewed the
> email.  How am I supposed to clear out the highlight?

The email?
The highlight of the chat button goes away as soon as the conversation with unread messages is selected.
Could it be that during your testing you had more than one conversation with unread messages, and missed one? (could easily happen if you had nickserv or chanserv telling you some nonsense for example)

Otherwise, I would need steps to reproduce.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #15)

> >+    if (this.countUnreadMessages())
> >+      chatButton.setAttribute("unreadMessages", "true");
> >+    else
> >+      chatButton.removeAttribute("unreadMessages");
> 
> What do you think about doing:
>   chatButton.setAttribute("unreadMessages", this.countUnreadMessages());
> instead, and getting rid of the "if"?

That would force the CSS to use a selector for #chat-button:not([unreadMessages=0]) instead of just #chat-button[unreadMessages] I don't think it's a simplification.
(In reply to Florian Quèze from comment #16)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)
> > On Mac:  I like it, but it seemed to remain highlit even after I viewed the
> > email.  How am I supposed to clear out the highlight?
> The email?

The chat.  :P  Slow brain day, apparently.

> The highlight of the chat button goes away as soon as the conversation with
> unread messages is selected.
> Could it be that during your testing you had more than one conversation with
> unread messages, and missed one? (could easily happen if you had nickserv or
> chanserv telling you some nonsense for example)

I didn't think so.  There might have been twitter messages which remained unread…

> Otherwise, I would need steps to reproduce.

If I notice it happening again, I'll post STR.  Probably as a new bug.
(Assignee)

Comment 18

5 years ago
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)

> On Mac:  I like it, but it seemed to remain highlit even after I viewed the
> email.  How am I supposed to clear out the highlight?

I found the steps to reproduce. It happens when the conversation receiving new messages was already selected.
A workaround is to just click the conversation again in the left pane. I'll debug this.
(Assignee)

Comment 19

5 years ago
Created attachment 636342 [details] [diff] [review]
Additional fix above attachment 632746 [details] [diff] [review]

This patch fixes the first issue mentioned in comment 14.
Created attachment 636672 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac v2

Addressing ui-review remarks.
Note that the above patch don't depend on any external patches.
(Assignee)

Comment 22

5 years ago
Landed attachment 636672 [details] [diff] [review] + attachment 636342 [details] [diff] [review] as http://hg.mozilla.org/comm-central/rev/fb66eb0893fc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
(Assignee)

Updated

5 years ago
Attachment #636672 - Flags: approval-comm-aurora?

Updated

5 years ago
Attachment #636672 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/releases/comm-aurora/rev/52a6f232596f
status-thunderbird15: --- → fixed
(Assignee)

Updated

5 years ago
Depends on: 780643
You need to log in before you can comment on or make changes to this bug.