Last Comment Bug 748358 - Change the appearance of the chat button instead of reopening the chat tab when receiving a message
: Change the appearance of the chat button instead of reopening the chat tab wh...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 16.0
Assigned To: Florian Quèze [:florian] [:flo]
:
:
Mentors:
Depends on: 780643
Blocks: 740358
  Show dependency treegraph
 
Reported: 2012-04-24 07:42 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-08-06 09:47 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
WIP (8.05 KB, patch)
2012-05-30 13:06 PDT, Florian Quèze [:florian] [:flo]
bwinton: ui‑review-
Details | Diff | Splinter Review
WIP (v2) (185.01 KB, patch)
2012-06-11 03:11 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
WIP (v3) (187.78 KB, patch)
2012-06-11 04:59 PDT, Andreas Nilsson (:andreasn)
no flags Details | Diff | Splinter Review
this is what it currently looks on Aero (screenshot) (34.26 KB, image/png)
2012-06-11 06:16 PDT, Andreas Nilsson (:andreasn)
no flags Details
patch for linux, windows (xp+aero) and mac (205.66 KB, patch)
2012-06-11 09:58 PDT, Andreas Nilsson (:andreasn)
florian: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch (code changes) (11.70 KB, patch)
2012-06-13 09:53 PDT, Florian Quèze [:florian] [:flo]
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Additional fix above attachment 632746 (1.03 KB, patch)
2012-06-25 09:32 PDT, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Splinter Review
patch for linux, windows (xp+aero) and mac v2 (189.62 KB, patch)
2012-06-26 05:55 PDT, Andreas Nilsson (:andreasn)
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2012-04-24 07:42:15 PDT
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.
Comment 1 Florian Quèze [:florian] [:flo] 2012-05-30 13:06:39 PDT
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).
Comment 2 Blake Winton (:bwinton) (:☕️) 2012-05-31 13:14:07 PDT
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.
Comment 3 Florian Quèze [:florian] [:flo] 2012-05-31 13:21:00 PDT
(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! :)
Comment 4 Andreas Nilsson (:andreasn) 2012-06-07 08:39:02 PDT
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?
Comment 5 Florian Quèze [:florian] [:flo] 2012-06-07 08:53:58 PDT
(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.
Comment 6 Andreas Nilsson (:andreasn) 2012-06-11 03:11:04 PDT
Created attachment 631841 [details] [diff] [review]
WIP (v2)

With new graphics for pinstripe, gnomestripe and qute (non-aero).
Comment 7 Andreas Nilsson (:andreasn) 2012-06-11 04:59:46 PDT
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.
Comment 8 Andreas Nilsson (:andreasn) 2012-06-11 05:21:57 PDT
note that this patch needs the patch in #727598 applied first
Comment 9 Andreas Nilsson (:andreasn) 2012-06-11 06:16:15 PDT
Created attachment 631876 [details]
this is what it currently looks on Aero (screenshot)
Comment 10 Andreas Nilsson (:andreasn) 2012-06-11 09:58:46 PDT
Created attachment 631929 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac
Comment 11 Florian Quèze [:florian] [:flo] 2012-06-13 09:52:07 PDT
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.
Comment 12 Florian Quèze [:florian] [:flo] 2012-06-13 09:53:15 PDT
Created attachment 632746 [details] [diff] [review]
Patch (code changes)
Comment 13 Florian Quèze [:florian] [:flo] 2012-06-13 09:57:33 PDT
(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.
Comment 14 Blake Winton (:bwinton) (:☕️) 2012-06-18 12:35:18 PDT
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.
Comment 15 Blake Winton (:bwinton) (:☕️) 2012-06-18 12:52:19 PDT
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.
Comment 16 Florian Quèze [:florian] [:flo] 2012-06-18 13:18:39 PDT
(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.
Comment 17 Blake Winton (:bwinton) (:☕️) 2012-06-18 13:22:37 PDT
(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.
Comment 18 Florian Quèze [:florian] [:flo] 2012-06-25 09:15:25 PDT
(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.
Comment 19 Florian Quèze [:florian] [:flo] 2012-06-25 09:32:40 PDT
Created attachment 636342 [details] [diff] [review]
Additional fix above attachment 632746 [details] [diff] [review]

This patch fixes the first issue mentioned in comment 14.
Comment 20 Andreas Nilsson (:andreasn) 2012-06-26 05:55:33 PDT
Created attachment 636672 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac v2

Addressing ui-review remarks.
Comment 21 Andreas Nilsson (:andreasn) 2012-06-26 05:56:13 PDT
Note that the above patch don't depend on any external patches.
Comment 23 Florian Quèze [:florian] [:flo] 2012-06-28 09:18:21 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/52a6f232596f

Note You need to log in before you can comment on or make changes to this bug.