Closed Bug 783687 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox Graveyard :: SocialAPI, defect)

17 Branch
defect
Not set
normal

Tracking

(firefox17+ verified)

RESOLVED FIXED
Firefox 18
Tracking Status
firefox17 + verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx17])

Attachments

(3 files, 5 obsolete files)

on title update, flash
Whiteboard: [Fx17]
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
Summary: Use title in the chat box to update the titlebar, similar to apptabs → Use custom event in the chat box to update the titlebar, similar to apptabs
Attached image title bar states.png (obsolete) —
Shows three states, the selected chatbox, an unselected chatbox, and an unselected chatbox with new activity.
Attached image chat nub states.png
shows the menuitems of two collapsed chatboxes, one with activity (bolded text)
Attached patch chat activity patch (obsolete) — Splinter Review
looking for feedback on direction of implementation
Assignee: nobody → mixedpuppy
Attachment #656231 - Flags: feedback?(mhammond)
Attachment #656231 - Flags: feedback?(jaws)
Attachment #656231 - Flags: feedback?(gavin.sharp)
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 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.
Attachment #656231 - Flags: feedback?(mhammond) → feedback+
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.
bug 782793 is implementing the selected state for the chat windows to finish out the style requirements in that bug.
Depends on: 782793
Attached patch chat activity patch (obsolete) — Splinter Review
updated patch based on top of bug 782793
Attachment #656231 - Attachment is obsolete: true
Attachment #656231 - Flags: feedback?(jaws)
Attachment #656231 - Flags: feedback?(gavin.sharp)
Attachment #657445 - Flags: feedback?(felipc)
Attached image title bar states.png
current chat titlebar showing "activity" state.  Boriss, what color would you like in that state (per-platform)?
Attachment #656229 - Attachment is obsolete: true
Attachment #657447 - Flags: ui-review?(jboriss)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → 17 Branch
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?
Attachment #657445 - Flags: feedback?(felipc) → feedback+
(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.
Attached patch chat activity patch (obsolete) — Splinter Review
Attachment #657445 - Attachment is obsolete: true
Attachment #658260 - Flags: feedback?(jaws)
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.
Attachment #658260 - Flags: feedback?(jaws) → feedback+
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.
Attachment #657447 - Flags: ui-review?(jboriss) → ui-review+
(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.
Attached patch chat activity patch (obsolete) — Splinter Review
updates css in patch, using placeholder color for activity state until boriss gets that to us.
Attachment #658260 - Attachment is obsolete: true
Attachment #664647 - Flags: review?(jaws)
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 ;)
Attachment #664647 - Flags: review?(jaws) → review+
fix the one wrong color in css from previous comment.  carry forward r+
Attachment #664647 - Attachment is obsolete: true
Blocks: 794252
(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).
https://hg.mozilla.org/mozilla-central/rev/987f0423f403
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #664653 - Flags: approval-mozilla-aurora+
Keywords: verifyme
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.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.