1184921 - Implement the refreshed design for the conversation/ chatbox titlebar

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

To meet the acceptance criteria as stated in bug 1179164, we should:

 - re-instate the light design of the chatbox, thus removing the dark theme we introduced. The old theme might not be good enough yet, so please make adjustments to it as necessary.
 - Replace the 'Close' button (x) with a 'Hang Up' button. Note that this button will be removed from the conversation toolbar and will only appear here.
 - The close button should _not_ close the conversation window right away, but instead behave just like the current 'Hang Up' button does - it might show the feedback request after the call! Therefore it's important that a button press sends an event to the content window to request to leave the conversation and close the window when appropriate.

For the visual design spec, please check out the ones attached to bug 1179164.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: remove the deprecated dark chatbox theme

I folded the socialchat.xml change into Patch 2, because I'm touching the same function twice.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: custom buttons in titlebar and chatbox theme refresh

Comment 3

[Mike de Boer [:mikedeboer]]

I need a new SVG version for the pop-out icon as shown here: https://www.dropbox.com/sh/46pyq5wwgnif6g8/AACEqjLwDw1be0oMYw-okHA9a?dl=0&preview=Share_Screen.png

Sevaan, can you or Michael provide it? Thanks!

Comment 4

[Sevaan Franks [:sevaan]]

Mike, would you be able to create an asset? http://i.sevaan.com/image/0m0F0k2V1s2j

Comment 5

[Andrei Oprea [:andreio]]

Comment on attachment 8650385 [details] [diff] [review]
Patch 2: custom buttons in titlebar and chatbox theme refresh

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

I am not familiar with some of the codebase this touches so most of my review comments are questions :)
I expected to see some tests for the code that was added in.

::: browser/modules/Chat.jsm
@@ +252,5 @@
> +    let document = chatbox.ownerDocument;
> +    let titlebarNode = document.getAnonymousElementByAttribute(chatbox, "class",
> +      "chat-titlebar");
> +    let buttonsSeen = new Set();
> +    

Whitespace.

@@ +268,5 @@
> +        let button = gCustomButtons.get(buttonId);
> +        let buttonClass = "chat-" + buttonId;
> +        // Custom buttons are not defined in the chatbox binding, thus not
> +        // anonymous elements.
> +        node = titlebarNode.querySelector("." + buttonClass);

Could we use getElementsByClassName since it will always be a CSS class that we query for.

@@ +290,5 @@
> +        }
> +
> +        // When the chat is undocked and the button wants to be visible then, it
> +        // will be.
> +        node.hidden = isUndocked && !button.visibleWhenUndocked ? true : false;

Couldn't this just be `node.hidden = isUndocked && !button.visibleWhenUndocked;`

@@ +299,5 @@
> +
> +    // Hide any button that is part of the default set, but not of the current set.
> +    for (let button of kDefaultButtonSet) {
> +      if (buttonsSeen.has(button))
> +        continue;

Could this be `if (!buttonsSeen.has(button))` and skip the continue statement?

::: browser/themes/shared/social/chat.inc.css
@@ +238,5 @@
>    padding: 0px;
> +  border-top-left-radius: 8px;
> +  border-top-right-radius: 8px;
> +  border-bottom-left-radius: 8px;
> +  border-bottom-right-radius: 8px;

Could this be `border-radius: 8px;`?

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: custom buttons in titlebar and chatbox theme refresh

Now with Andrei's review comments addressed and an integration test.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8650383 [details] [diff] [review]
Patch 1: remove the deprecated dark chatbox theme

Deferring review to Mark...

Comment 8

[Mark Banner (:standard8)]

Attachment: Screen shot of current patches

Vicky/Sevaan: There's an issue here - here's the conversation window overlaying the start page. I think the title bar colours are a little bit too close to make this scheme sensible...

Sevaan, we're also still waiting on the image from Michael as well.

Comment 9

[Sevaan Franks [:sevaan]]

There should be a dropshadow, which will help separate it. Vicky, what do you think?

Comment 10

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8657105 [details]
Screen shot of current patches

Oh this looks so much better! Only thing is to update the exit icon as it has changed to match the style of others in the conversation window. You can find it with the conversation icons sprite in the assets on Dropbox.

THanks!

Comment 11

[Mark Banner (:standard8)]

Handing back to Mike...

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8655435 [details] [diff] [review]
Patch 2.1: custom buttons in titlebar and chatbox theme refresh

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

Ok, generally looks good, but this needs update for the UX comments, and for the comments I added below.

::: browser/components/loop/modules/MozLoopService.jsm
@@ +87,5 @@
>  
>  // See LOG_LEVELS in Console.jsm. Common examples: "All", "Info", "Warn", & "Error".
>  const PREF_LOG_LEVEL = "loop.debug.loglevel";
>  
> +const kChatboxCloseButton = {

I think it might be better to call this kChatboxHangupButton - the id of it is hangup, so its a bit strange later on when you register a 'close' but then make visible the 'loop-hangup' button.

@@ +1014,3 @@
>        chatboxInstance.setAttribute("customSize", "loopDefault");
>        chatboxInstance.parentNode.setAttribute("customSize", "loopDefault");
> +      Chat.loadButtonSet(chatboxInstance, "minimize,swap,loop-hangup");

How about "minimize,swap," + kChatboxCloseButton.id to help future maintainability? (or maybe a separate constant defined near kChatboxCloseButton).

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +81,5 @@
>    });
>  
>    describe("#init", function() {
>      var OTRestore;
> +

Unnecessary change? or maybe a missing LoopHangupNow test?

::: browser/modules/Chat.jsm
@@ +212,5 @@
>    },
> +
> +  /**
> +   * Adds a button to the collection of custom buttons that can be added to the
> +   * titlebar of a chatbox.

I think it would be useful to reference that for the button to be visible, loadButtonSet has to be called with the new button's id in the buttonSet parameter. i.e. just a way to tie these two functions together through documentation.

::: browser/themes/shared/social/chat.inc.css
@@ +123,5 @@
>    text-shadow: none;
>    cursor: inherit;
>  }
>  
>  .chat-titlebar {

Presumably we're happy that the changes we're doing here will affect social API as well?

Comment 13

[Sevaan Franks [:sevaan]]

Attachment: Pop Out Icon (SVG)

(In reply to Sevaan Franks [:sevaan] from comment #4)
> Mike, would you be able to create an asset?
> http://i.sevaan.com/image/0m0F0k2V1s2j

Icon attached from :mmaslaney

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #12)
> Presumably we're happy that the changes we're doing here will affect social
> API as well?

Yes, since we're basically the only consumer of this part of the API. Apart from that fact, consider this a small visual refresh of the chat window styles as well and it's good that all consumers, including Hello, look the same.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Screen shot of current patches

If I need to replace the icon of the hangup button, please let me know where I can find it - I don't know which URL(s) to check anymore :-/

Comment 16

[Mike de Boer [:mikedeboer]]

Oh and a question! What should the hover and active (click) states look like of the camera, minimize and expand buttons?

Comment 17

[Sevaan Franks [:sevaan]]

Comment on attachment 8665953 [details]
Screen shot of current patches

Let's move the icons left of the red area over, so the spacing between the popout icon and the red area is the same as the spacing between the minimize and popout icon.

http://i.sevaan.com/image/1t390t1C0m09

In this mockup I moved the icons over 22px.

+r with that change.

Comment 18

[Sevaan Franks [:sevaan]]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> Oh and a question! What should the hover and active (click) states look like
> of the camera, minimize and expand buttons?

Just colour changes:

Camera Icon

- Hover: #EB6B30
- Active: #EB6B30

Minimize + Pop-out Icons

- Hover: #4a4a4a
- Active: #4a4a4a

Leave Conversation (Icon remains white, and red background changes)

- Hover: #EF6745
- Active: #EF6745

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Sevaan Franks [:sevaan] from comment #18)
> Just colour changes:
> 
> Camera Icon
> 
> - Hover: #EB6B30
> - Active: #EB6B30
> 
> Minimize + Pop-out Icons
> 
> - Hover: #4a4a4a
> - Active: #4a4a4a
> 
> Leave Conversation (Icon remains white, and red background changes)
> 
> - Hover: #EF6745
> - Active: #EF6745

Sorry, I'd really like a quick mockup, because I also need to know how much of the titlebar needs to be covered by the button. Also, #4a4a4a is quite dark; should the icons change color as well?

Comment 20

[Sevaan Franks [:sevaan]]

> Sorry, I'd really like a quick mockup, because I also need to know how much
> of the titlebar needs to be covered by the button. Also, #4a4a4a is quite
> dark; should the icons change color as well?


No button. Only changing the colours of the elements.

http://i.sevaan.com/image/0s3o1D2y470s

Comment 21

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: custom buttons in titlebar and chatbox theme refresh

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8666688 [details] [diff] [review]
Patch 2.2: custom buttons in titlebar and chatbox theme refresh

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

Per irc, I think we're missing the hangup button when the window is popped-out.

::: browser/components/loop/content/shared/js/views.jsx
@@ +458,5 @@
>        });
>        return (
>          <ul className={conversationToolbarCssClasses}>
> +          {
> +            this.props.showHangup ? 

eslint says:

content/shared/js/views.jsx
  499:36  error  Trailing spaces not allowed  no-trailing-spaces

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.3: custom buttons in titlebar and chatbox theme refresh

This patch now abstracts the window object event listeners to the conversationAppStore, so that all event listeners can be unit tested efficiently.

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.3: custom buttons in titlebar and chatbox theme refresh

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8667856 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

Raaaah! My queues are messed up! Sorry for the noise, ppl :(

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.3: custom buttons in titlebar and chatbox theme refresh

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 8667877 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

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

Looks really nice. Moving the window listeners was a good idea as well.

Comment 28

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/8e4a248e5e2da2faeb06526fe4e29006e6a5d477
Bug 1184921: remove the deprecated dark chatbox theme. r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/d44a52b0faef48ef6d38df62eda63e78d391a7e7
Bug 1184921: allow custom buttons to be added to the chatbox titlebar and implement one for Hello that closes the window when clicked. r=Standard8

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e4a248e5e2d
https://hg.mozilla.org/mozilla-central/rev/d44a52b0faef

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8650383 [details] [diff] [review]
Patch 1: remove the deprecated dark chatbox theme

Approval Request Comment
[Feature/regressing bug #]: Hello Visual refresh - final parts 
[User impact if declined]: The conversation window title bar will be inconsistent with the rest of the Hello styling in 43.
[Describe test coverage new/current, TreeHerder]: Combined patches have tests for the Hello code and the social changes.
[Risks and why]: There's some changes to the social conversation window, however Hello is the only ones using it. The other changes are lower risk to Hello.
[String/UUID change made/needed]: None

Note: Both patches on this bug need to be taken together. We'll also need the patches on the dependent bugs (bug 1210513, bug 1210707, bug 1211219) which were minor follow ups as we'd missed some of the control flow, but we've got it all right now.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 8667877 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

Approval Request Comment

See comment 30.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8650383 [details] [diff] [review]
Patch 1: remove the deprecated dark chatbox theme

Several patches to uplift to aurora, including tests.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8667877 [details] [diff] [review]
Patch 2.3: custom buttons in titlebar and chatbox theme refresh

This needs uplift to aurora for Hello theme changes.

Comment 34

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91a9dd45c071
https://hg.mozilla.org/releases/mozilla-aurora/rev/fed07352f43f

Comment 35

[Bogdan Maris, Desktop Test Engineering [OUT until 12 Jan]]

I tested the new refreshed design on Firefox Developer Edition 43.0a2 and latest Nightly 44.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit), no major issues found except bug 1218384 which will be treated separately.