783687 - Use custom event in the chat box to update the titlebar, similar to apptabs

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

on title update, flash

Comment 1

[Shane Caraveo (:mixedpuppy)]

This bug is all about having a titlebar indicator that chat activity has occurred in a non-current chatbox.  After investigating the use of title or icon changes to show this, I feel that the activity indicator needs to be separate from those.  Listening for a custom event to do this works well.

This patch also updates the chat nub menubutton for those chatboxes that have overflowed with the icon, title and activity of the collapsed chatboxes.  Due to what seems like platform limitations with native menu's, I've choosen to bold the title text to reflect activity in the menu's.

images and patch to follow

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: title bar states.png

Shows three states, the selected chatbox, an unselected chatbox, and an unselected chatbox with new activity.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: chat nub states.png

shows the menuitems of two collapsed chatboxes, one with activity (bolded text)

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: chat activity patch

looking for feedback on direction of implementation

Comment 5

[Shane Caraveo (:mixedpuppy)]

Boriss, i've got a way to handle the notification of chat activity, but need a design to shoot towards.  You can see the attached images for the various available states.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 656231 [details] [diff] [review]
chat activity patch

Looks good to me.  I don't quite understand the comment in the block:

+            if (aChatbox.getAttribute("activity")) {
+              // checked seems to be the only thing I can affect in native menus
+              // as an activity indicator
+              menuitem.setAttribute("activity", true);
+              this.nub.setAttribute("activity", true);

but that might just be me being extremely rusty with xbl.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Sorry about that comment, it is cruft from an earlier idea, it will need to be removed.

(In reply to Mark Hammond (:markh) from comment #6)
> Comment on attachment 656231 [details] [diff] [review]
> chat activity patch
> 
> Looks good to me.  I don't quite understand the comment in the block:
> 
> +            if (aChatbox.getAttribute("activity")) {
> +              // checked seems to be the only thing I can affect in native
> menus
> +              // as an activity indicator
> +              menuitem.setAttribute("activity", true);
> +              this.nub.setAttribute("activity", true);
> 
> but that might just be me being extremely rusty with xbl.

Comment 8

[Shane Caraveo (:mixedpuppy)]

bug 782793 is implementing the selected state for the chat windows to finish out the style requirements in that bug.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: chat activity patch

updated patch based on top of bug 782793

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: title bar states.png

current chat titlebar showing "activity" state.  Boriss, what color would you like in that state (per-platform)?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 657445 [details] [diff] [review]
chat activity patch

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

It would be nice if you could handle this with a single event listener in the chatbar instead of one for each chatbox, eliminating the need for the update() calls. However I suppose it would be tricky to get which chatbox the event came from. Or perhaps you already do something similar in another part of the code?

::: browser/base/content/socialchat.xml
@@ +90,5 @@
> +        iframeWindow.addEventListener("chatActivity", chatActivity);
> +        iframeWindow.addEventListener("unload", function unload() {
> +          iframeWindow.removeEventListener("unload", unload);
> +          iframeWindow.removeEventListener("chatMessage", chatActivity);
> +        });

"chatActivity" was used in addEventListener and "chatMessage" in removeEventListener.  But why do you need to add/remove this listener? can't they be a <handler> like the others?

Are events like these being added prefixed with moz/social?

@@ +215,5 @@
>            }
>          ]]></body>
>        </method>
>  
> +      <method name="update">

update is too generic, needs a better name to describe its purpose

@@ +332,5 @@
>  
>      </implementation>
>      <handlers>
> +      <handler event="popupshown"><![CDATA[
> +        this.nub.removeAttribute("activity");

should this clear the activity attribute on all menuitems as well? If the user has seen it by opening the menu I think it should (or perhaps not clear the nub). I think that in any case they going out of sync would be confusing.

::: browser/themes/pinstripe/browser.css
@@ +3455,5 @@
>  .chat-titlebar[selected="true"] {
>    background-color: #f0f0f0;
>  }
> +  
> +.chat-titlebar[activity]:not([selected="true"]) {

:not([selected="true"]) is not necessary because the selected setter removes the activity attribute, right?

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes from comment #11)
> Comment on attachment 657445 [details] [diff] [review]
> chat activity patch
> 
> Review of attachment 657445 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would be nice if you could handle this with a single event listener in
> the chatbar instead of one for each chatbox, eliminating the need for the
> update() calls. However I suppose it would be tricky to get which chatbox
> the event came from. Or perhaps you already do something similar in another
> part of the code?
> 
> ::: browser/base/content/socialchat.xml
> @@ +90,5 @@
> > +        iframeWindow.addEventListener("chatActivity", chatActivity);
> > +        iframeWindow.addEventListener("unload", function unload() {
> > +          iframeWindow.removeEventListener("unload", unload);
> > +          iframeWindow.removeEventListener("chatMessage", chatActivity);
> > +        });
> 
> "chatActivity" was used in addEventListener and "chatMessage" in
> removeEventListener.  But why do you need to add/remove this listener? can't
> they be a <handler> like the others?

Capturing or not, I could not catch the event in a <handler>.  Perhaps because it is a content generated event?

> Are events like these being added prefixed with moz/social?

I could.  Our other event is "ActivateSocialFeature".


> @@ +332,5 @@
> >  
> >      </implementation>
> >      <handlers>
> > +      <handler event="popupshown"><![CDATA[
> > +        this.nub.removeAttribute("activity");
> 
> should this clear the activity attribute on all menuitems as well? If the
> user has seen it by opening the menu I think it should (or perhaps not clear
> the nub). I think that in any case they going out of sync would be confusing.

The nub shows that there is new activity in one of the hidden chats since you last looked at the menu.  If you look at the menu, but you decide to NOT look at one of the chats that had activity, the menuitem shouldn't be cleared so you still have an indicator for later which chat had activity.  However, you want the nub to clear, so that you'll see the next time there is activity in one of the hidden chats.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: chat activity patch

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 658260 [details] [diff] [review]
chat activity patch

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

Sorry for the slow feedback response.

::: browser/base/content/socialchat.xml
@@ +84,5 @@
> +        let chatbox = this;
> +        function chatActivity() {
> +          chatbox.setAttribute("activity", true);
> +          chatbox.parentNode.updateTitlebar(chatbox);
> +        };

Can we use event.target or something similar to determine where the event came from? If we did that then we might be able to only have one event listener for all chats instead of a separate one for each chatbox.

::: browser/themes/gnomestripe/browser.css
@@ +2771,5 @@
>    background-color: #f0f0f0;
>  }
>  
> +.chat-titlebar[activity]:not([selected]) {
> +  background-image: -moz-radial-gradient(left 99%, circle cover, rgba(254,254,255,1) 3%, rgba(210,235,255,.9) 12%, rgba(148,205,253,.6) 30%, rgba(148,205,253,.2) 70%);

radial-gradient has been unprefixed. Please remove the -moz- prefix here.

::: browser/themes/pinstripe/browser.css
@@ +3457,5 @@
>  .chat-titlebar[selected] {
>    background-color: #f0f0f0;
>  }
>  
> +.chat-titlebar[activity]:not([selected]) {

Felipe's feedback was that the selected & activity states should not intersect since "activity" is removed when selected=true. Can we remove this |not([selected])| part of the selector?

@@ +3495,5 @@
> +  background-image: -moz-radial-gradient(center 99%, circle cover, rgba(254,254,255,1) 3%, rgba(210,235,255,.9) 12%, rgba(148,205,253,.6) 30%, rgba(148,205,253,.2) 70%);
> +}
> +
> +.chatbar-button menuitem[activity] {
> +  font-weight: bold;

This rule should be present in gnomestripe and winstripe. Also, please use the child selector here.

Comment 15

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 657447 [details]
title bar states.png

The colors & styles need tweaking, but let's get this out and fix those in a followup bug.

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #14)

> ::: browser/base/content/socialchat.xml
> @@ +84,5 @@
> > +        let chatbox = this;
> > +        function chatActivity() {
> > +          chatbox.setAttribute("activity", true);
> > +          chatbox.parentNode.updateTitlebar(chatbox);
> > +        };
> 
> Can we use event.target or something similar to determine where the event
> came from? If we did that then we might be able to only have one event
> listener for all chats instead of a separate one for each chatbox.

That event doesn't seem to propagate outside of the iframe, I'm assuming because it is a custom event created in content.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: chat activity patch

updates css in patch, using placeholder color for activity state until boriss gets that to us.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 664647 [details] [diff] [review]
chat activity patch

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

Please file a new bug for the updated colors and assign it to Boriss. Also, mark it as tracking-firefox17 please :)

::: browser/themes/pinstripe/browser.css
@@ +3499,5 @@
>    display: none;
>  }
>  
> +.chatbar-button[activity] {
> +  background-color: #acd5f0;

just curious, but if these are placeholder colors, what's the reason for this one background-color to be different than the other two themes? seems like it might make find/replace harder to perform ;)

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: chat activity patch

fix the one wrong color in css from previous comment.  carry forward r+

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> That event doesn't seem to propagate outside of the iframe, I'm assuming
> because it is a custom event created in content.

You can probably work around that by installing the listener with addEventListener and passing "true" for the optional fourth parameter (accept untrusted events).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/987f0423f403

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/987f0423f403

Comment 23

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a4a9b716142

Comment 24

[Virgil Dicu [:virgil] [QA]]

Verified on
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 Beta 5
Build ID: 20121106195758
using Facebook Social API. On every new message the chat is updating it's titlebar, near the name of the chat user, the number of unread messages appears in brackets and the color of the titlebar is changed.

Comment 25

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old