1184559 - Implement the refreshed design for the conversation toolbar

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:

 - Move the conversation toolbar down, hovering the remote video stream.
 - May contain the following buttons: face mute, audio mute, screenshare, edit.

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

Comment 1

[Marina Rodríguez [:mai]]

Attachment: conversation_toolbar_panel.png

Screenshots with the conversation toolbar on the panel.

Comment 2

[Marina Rodríguez [:mai]]

Comment on attachment 8644276 [details]
conversation_toolbar_panel.png

Hi Vicky,
would you mind reviewing the image?
Best regards

Comment 3

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8644276 [details]
conversation_toolbar_panel.png

Little interaction adjustment: it would make more sense to have the whole group of buttons lightening up when the user moves the cursor over the video area.

So make it like this:

when user moves the cursor every button lightens up becoming less transparent.
After 5 secs of having the cursor static, all buttons become 75% opaque 
Make hover state it the same style as click, so there's a different feedback for when the cursor is over each individual button.

Besides that:
- Outer border should be #000000 20% opacity in all the buttons (so it helps pop out from the background image but it is not an style decoration.)
- Update the "exit" icon
- Dividing vertical line in the compound button of audio and video is missaligned, make it tall enough to leave a gap pf 3px both in bottom and top.
- No video icon looks cropped
Thanks!

Comment 4

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Hi Mark,
would you mind taking a look the patch?

Best regards

Comment 5

[Marina Rodríguez [:mai]]

Hi vicky,
would you mind reviewing the behaviour of the conversation toolbar?

https://www.youtube.com/watch?v=6mEVI0DZy_I
https://www.youtube.com/watch?v=Sae_7ZsJF8k


Best regards

Comment 6

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Updated the svg files. 
Would you mind reviewing the patch?

Comment 7

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Updated with some css changes.

Comment 8

[Marina Rodríguez [:mai]]

Attachment: conversation_toolbar_panel.png

Comment 9

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Remove unused code.

Comment 10

[Marina Rodríguez [:mai]]

Hi Vicky,
the video with the transition by inactivity.

https://www.youtube.com/watch?v=nv3kcED4b4c

Best regards

Comment 11

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8646876 [details]
conversation_toolbar_panel.png

Can you please only adjust the right side margin to be 15px? (before the exit button) Awesome job! Thanks!

Comment 12

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Updated with the last comments of Vicky

Comment 13

[Victoria Gerchinhoren [:vicky]]

(In reply to Marina Rodríguez [:mai] from comment #10)
> Hi Vicky,
> the video with the transition by inactivity.
> 
> https://www.youtube.com/watch?v=nv3kcED4b4c
> 
> Best regards

Cool!

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8646958 [details] [diff] [review]
0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

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

This looks a lot nicer overall and is heading in the right direction. There's a lot of comments here, but most of these I think are small things that shouldn't be too difficult to address. I've not been through everything yet - I thought it was worth getting these back to you for revision before going through some of the more finer detail.

The new toolbar looks a little small for my liking on the bigger views, but that's what UX asked for so we'll see how it goes ;-)

- When both screen share is active on the standalone, the toolbar buttons are displayed relative to the remote video - this can be seen on the ui-showcase.

I'll try to point out how this can be improved in the code as I go through the review.

- If you pop-out the conversation window on desktop with text chat or context, then you'll see there's a blank section near the top where the toolbar used to be.

If you search for '1184559' in conversation.css you'll find the parts that need removing to fix this. Let me know if you're not sure which ones.

- Now that the toolbar is working in RTL mode, it shows up a couple of issues, could you also add rtl versions of these rules please:

http://hg.mozilla.org/mozilla-central/annotate/7649ffe28b67/browser/components/loop/content/shared/css/conversation.css#l1202 (needs right=auto, left=0)
http://hg.mozilla.org/mozilla-central/annotate/7649ffe28b67/browser/components/loop/content/shared/css/conversation.css#l1296 (needs right=auto, left=0)

::: browser/components/loop/content/js/conversationViews.jsx
@@ +701,5 @@
>              screenShareVideoObject={this.state.screenShareVideoObject}
>              showContextRoomName={false}
> +            useDesktopPaths={true}>
> +            <loop.shared.views.ConversationToolbar
> +              audio={this.props.audio}

For some reason, the changes to the ConversationToolbar now respect audio.visible and video.visible better - I don't quite understand why. As a result, I'm not seeing the audio/video mute buttons on direct calls.

Can you add visible: true to the lines in CallControllerView#render under the case CALL_STATES.ONGOING statement, i.e.

audio={{ enabled: !this.state.audioMuted, visible: true }}
video={{ enabled: !this.state.videoMuted, visible: true }} />

(with added spaces as well please).

That should fix this.

You'll also need to do the same for the ConversationToolbar in the ui-showcase. You could probably drop the desktop + standalone versions of the ones in the showcase, and just have one set of examples.

::: browser/components/loop/content/shared/css/conversation.css
@@ +19,5 @@
> +.conversation-toolbar {
> +  z-index: 1020; /* required to have it superimposed to the video element */
> +  border: 0;
> +  left: 1.2rem;
> +  right: .7rem;

You'll need a rtl rule for these:

html[dir="rtl"] .conversation-toolbar {
  left: .7rem;
  right: 1.2rem;
}

You can check this on the ui-showcase with the rtl toggle.

@@ +42,1 @@
>    height: 100%;

Please drop the ".standalone .focus-stream" from this one, that was just a workaround needed whilst we had the toolbar separate.

There's already a ".media-wrapper > .focus-stream" providing the 100%.

@@ +48,5 @@
>    font-size: 0; /* prevents vertical bottom padding added to buttons in google
>                     chrome */
>  }
>  
> +html[dir=rtl] .conversation-toolbar > li {

nit: I prefer to have the rtl in double-quotes, though its unclear how much that really matters.

@@ +50,5 @@
>  }
>  
> +html[dir=rtl] .conversation-toolbar > li {
> +  float: right;
> +  margin-left: .7rem;

The margin-right needs to be specified as "auto" for this rule - the plain ".conversation-toolbar > li" still applies for rtl mode, so we need to override that value.

@@ +51,5 @@
>  
> +html[dir=rtl] .conversation-toolbar > li {
> +  float: right;
> +  margin-left: .7rem;
> +  font-size: 0;

You don't need the font-size: 0 as that's already specified in the non-rtl version of this.

@@ +95,5 @@
> +  float: right;
> +}
> +
> +.conversation-toolbar-btn-box.btn-edit-entry {
> +    float: right;

nit: need a two-space indent. Also you need an rtl version of this rule.

@@ +122,1 @@
>  .fx-embedded-btn-icon-video,

XXXX ?

@@ +599,5 @@
>  .fx-embedded-tiny-audio-icon,
>  .fx-embedded-tiny-video-icon {
> +  width: 26px;
> +  height: 26px;
> +  background-color: #00a9dc;

I'm not sure the changes to this rule are wanted - this has confused the accept/reject display for incoming/outgoing calls, and the icons now look too large (see AcceptCallView on the ui-showcase).

Also, it doesn't show on the ui-showcase, but there's an accept with audio-only button where the microphone is overly large, and the main part of the button is a different height to the audio-only part.

@@ +776,3 @@
>    /* This matches .fx-embedded .conversation toolbar height */
> +  top: 0;
> +  height:100%;

The comment here can be removed now.

Also, please add a space before the 100%

@@ +1028,5 @@
>  }
>  
>  .standalone-room-wrapper > .media-layout {
>    /* 50px is the header, 64px for toolbar, 3em is the footer. */
> +  height: calc(100% - 50px - 3em);

nit: please update the comment.

@@ +1167,5 @@
>      height: 100%;
>    }
>  
> +  .media-wrapper > .focus-stream > .local ~ .conversation-toolbar {
> +    max-width: calc(100% - 22px - 120px);

nit: can you add a comment to explain where the values come from please?

::: browser/components/loop/content/shared/js/views.jsx
@@ +194,5 @@
>        };
>      },
>  
> +    getInitialState: function() {
> +      this.props.userActivity = true;

We shouldn't be overriding props for this, as its local data, just 'this.userActivity = true;' should do.

@@ +195,5 @@
>      },
>  
> +    getInitialState: function() {
> +      this.props.userActivity = true;
> +      this.startIdleCountDown();

Please move the startIdleCountDown and initialisation to a componentWillMount function (or maybe actually the componentDidMount function, I don't think which of those matters much, it will mean that it definitely gets initialised every time the component is displayed).

@@ +243,5 @@
> +    componentWillUnmount: function() {
> +      document.body.removeEventListener("mousemove", this._onBodyMouseMove);
> +    },
> +
> +    _onBodyMouseMove: function() {

Please add jsdocs for the new functions.

@@ +253,5 @@
> +      }
> +    },
> +
> +    checkUserActivity: function() {
> +      this.props.inactivityPollInterval = setInterval(function() {

Again, lets keep this out of props, so as not to confuse things.

@@ +1117,5 @@
>                  posterUrl={this.props.remotePosterUrl}
>                  srcVideoObject={this.props.remoteSrcVideoObject} />
>                { this.state.localMediaAboslutelyPositioned ?
>                  this.renderLocalVideo() : null }
>                { this.props.children }

I think we should probably merge ConversationToolbar into media layout and take the hit of the extra props - but not in this bug. We've already got a lot going on here.

The issue with using the children in this way for the ConversationToolbar is that its rendered in the wrong place for the screen sharing.

I think we can get away with doing an if statement for this.props.children, i.e. in this location:

{ this.props.displayScreenShare ? this.props.children : null }

and then do the reverse within the  <div className={screenShareStreamClasses}> below.

That should get us the toolbar rendered in the right place.

Comment 15

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Hi Mark,
I think I resolve all the issues, would you mind reviewing the patch again?
Best regards

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8647728 [details] [diff] [review]
0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

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

Ok, this is basically r+. The bits that are missing are:

- the couple of comments below
- tests for the new idle functionality in ConversationToolbar. Let me know if you need help with these - one hint, look at the existing tests for sandbox.useFakeTimers and clock.tick(delay) as it saves waiting for timeouts in the tests.
- Fixing the buttons on the AcceptCallView display - see https://www.dropbox.com/s/06hoe4pov6c00vb/Screen%20Shot%202015-08-14%20at%2011.08.48.png?dl=0
- In converstion.css, in .room-context, can you apply the following:

-  /* Stretch to the maximum available space whilst not covering the conversation
-     toolbar (26px). */
-  height: calc(100% - 26px);
+  height: 100%;

It means that we're not leaving a space where the old conversation toolbar used to be.

If you need help with the tests let me know, I'll be around today until the meeting.

::: browser/components/loop/content/shared/css/conversation.css
@@ +35,5 @@
> +  transition: opacity 1.5s;
> +  -webkit-transition: opacity 1.5s;
> +  -moz-transition: opacity 1.5s;
> +  -ms-transition: opacity 1.5s;
> +  -o-transition: opacity 1.5s;

We shouldn't need any of the prefixed versions here - looking at https://developer.mozilla.org/en-US/docs/Web/CSS/transition#Browser_compatibility they were all for versions that we don't support (as they don't have webrtc).

@@ +97,5 @@
> +  background-image: url("../img/svg/media-group-right-hover.svg");
> +  background-size: cover;
> +}
> +
> +html[dir="rtl"]

This seems to have shown up accidentally.

Comment 17

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Update the pr with your comments.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8648027 [details] [diff] [review]
0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

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

::: browser/components/loop/content/shared/css/conversation.css
@@ +601,5 @@
>    vertical-align: top;
>  }
>  
>  .fx-embedded-call-detail {
>    padding-top: 1.2em;

Given the increase in height of .fx-embedded-tiny-*-icon, lets drop this down to 0.6em. Its not an idea layout in terms of divs, but I suspect this will change soon, so for now I think just adjusting the height is fine.

::: browser/components/loop/test/shared/views_test.js
@@ +277,5 @@
> +      expect(comp.getDOMNode().classList.contains("idle")).eql(false);
> +    });
> +
> +    it("should be on idle state after 6 seconds", function() {
> +      var clock = sinon.useFakeTimers();

Lets move this to the beforeEach, then we're guaranteed to have no issues with random failures if a timer decides to kick in before the tests have finished.

@@ +287,5 @@
> +      expect(comp.getDOMNode().classList.contains("idle")).eql(false);
> +
> +      clock.tick(6001);
> +      expect(comp.getDOMNode().classList.contains("idle")).eql(true);
> +      clock.restore();

This can move to the afterEach as well.

@@ +301,5 @@
> +
> +      clock.tick(6001);
> +      expect(comp.getDOMNode().classList.contains("idle")).eql(true);
> +
> +      document.body.dispatchEvent(new CustomEvent('mousemove'));

nit: this fails eslint as it should be using double-quotes.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +881,2 @@
>              <div className="fx-embedded override-position">
>                <Example style={{width: "300px", height: "26px"}} summary="Default">

Due to the changed style of the conversation toolbar, we need to make these heights 56px, which handles the absolute positioning better.

Comment 19

[Mark Banner (:standard8)]

Attachment: Implement the refreshed design for the Loop conversation toolbar.

I had to fix bitrot to test it, and given the comments were small, I addressed those as well, so this is the ready to land patch, which I'll do so in a couple of minutes if the tree is clear.

Comment 20

[Mark Banner (:standard8)]

Attachment: Implement the refreshed design for the Loop conversation toolbar.

Ok, this time with the comments really addressed.

Comment 21

[Mark Banner (:standard8)]

Attachment: Implement the refreshed design for the Loop conversation toolbar.

... and without any additional changes.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f4f36fc12c1f

Comment 23

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/290b643b5e9f

Comment 24

[Mark Banner (:standard8)]

This was backed out due to failures like:

EST-UNEXPECTED-FAIL | test_desktop_all.py TestDesktopUnits.test_units | AssertionError: build/tests/marionette/tests/browser/components/loop/test/desktop-local/index.html: 1 failure(s) encountered:
TEST-UNEXPECTED-FAIL | index.html | should long only the warnings we expect - AssertionError: expected 29 to deeply equal 28

I didn't see this locally, so my best guess is that adding the setTimeout to the componentDidMount of ConversationToolbar is being triggered in other views where the toolbar is used. So we either need to isolate it a bit more or use sandbox.useFakeTimers in more places.

The builders are likely to be a bit slower, so I suspect the extra warning is the timeout kicking in and failing.

Unfortunately I don't have time to look at this before going away - Mike should be back next week. I'd also recommend a try push for just the marionette tests with an attempted fix.

Comment 25

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

Hi Mike, 
would you mind reviewing the patch? 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bc4dd78e7e8


The changes applied to fix the test are the following:

diff --git a/browser/components/loop/test/desktop-local/conversation_test.js b/browser/components/loop/test/desktop-local/conversation_test.js
index d38cb8c..d6cc9d4 100644
--- a/browser/components/loop/test/desktop-local/conversation_test.js
+++ b/browser/components/loop/test/desktop-local/conversation_test.js
@@ -26,11 +26,14 @@ describe("loop.conversation", function() {
       },
       setLoopPref: setLoopPrefStub,
       getLoopPref: function(prefName) {
-        if (prefName === "debug.sdk") {
-          return false;
+        switch (prefName) {
+          case "contextInConversations.enabled":
+            return false;
+          case "debug.sdk":
+            return false;
+          default:
+            return "http://fake";
         }
-
-        return "http://fake";
       },
       LOOP_SESSION_TYPE: {
         GUEST: 1,
diff --git a/browser/components/loop/test/desktop-local/index.html b/browser/components/loop/test/desktop-local/index.html
index 4f30c2c..d74b997 100644
--- a/browser/components/loop/test/desktop-local/index.html
+++ b/browser/components/loop/test/desktop-local/index.html
@@ -95,7 +95,7 @@
 
     describe("Unexpected Warnings Check", function() {
       it("should long only the warnings we expect", function() {
-        chai.expect(caughtWarnings.length).to.eql(28);
+        chai.expect(caughtWarnings.length).to.eql(27);
       });
     });

Comment 26

[Mike de Boer [:mikedeboer]]

Comment on attachment 8648973 [details] [diff] [review]
0001-Bug-1184559-Implement-the-refreshed-design-for-the-c.patch

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

r=me! Thanks for adding the try-link.

Comment 27

[Mike de Boer [:mikedeboer]]

url:        https://hg.mozilla.org/integration/fx-team/rev/b83ffe470d28f8311d901bde5640b19ef3ccf92c
changeset:  b83ffe470d28f8311d901bde5640b19ef3ccf92c
user:       Marina Rodriguez Iglesias <[e-mail]>
date:       Thu Aug 20 11:25:15 2015 +0200
description:
Bug 1184559 - Implement the refreshed design for the conversation toolbar. r=Standard8,mikedeboer

Comment 28

[Mike de Boer [:mikedeboer]]

url:        https://hg.mozilla.org/integration/fx-team/rev/bbf7ab9f23651de97838e887931c042a4142761c
changeset:  bbf7ab9f23651de97838e887931c042a4142761c
user:       Mike de Boer <[e-mail]>
date:       Thu Aug 20 12:08:48 2015 +0200
description:
Bug 1184559: forgot to push newly added files. rs=bustage. CLOSED TREE.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b83ffe470d28
https://hg.mozilla.org/mozilla-central/rev/bbf7ab9f2365

Comment 30

[Ada [:adalucinet]]

Verified fixed using latest Nightly 43.0a1 and 44.0a1, across platforms [1].
One new issues found here - bug 1208047.

[1] Windows 10 32-bit, Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit