1107255 - Clicking on a room, or starting a direct conversation should close the panel

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

When clicking on a room, or starting a direct video/audio call, the panel should be closed automatically.

Other actions, e.g. blocking a contact, copying a room url, should keep the panel open.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Close panel on call conversation start

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Use WindowCloseMixin for existing, tested versions of window.close

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8533348 [details] [diff] [review]
Close panel on call conversation start

This causes conversation entry URL clicks from the panel as well as audio and video call starts from the contact list to close the panel, but nothing else.

It uses the WindowCloseMixin that we paired on last week.  This patch is based on fx-team, and once it lands on mozilla-central, it probably wants to be uplifted.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8533349 [details] [diff] [review]
Use WindowCloseMixin for existing, tested versions of window.close

This patch must be applied on top of the previous one, and converts all existing _tested_ calls (and their tests) to window.close to use the mixin. There are a bunch of untested calls which need to have tests written before they're converted.  I'm happy to file a spin-off bug to do that, if you wish.  I don't think this wants to be uplifted after it lands.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Note that I'm happy (and would even prefer!) to do these as pair reviews, if you're inclined and available.

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8533348 [details] [diff] [review]
Close panel on call conversation start

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

LGTM, r=nperriault.

::: browser/components/loop/content/shared/js/mixins.js
@@ +69,5 @@
> +   * calling window.close() directly, use this mixin and call
> +   * this.closeWindow from your component.  In tests, use
> +   * loop.shared.mixins.setRootObject to substitute a fake window
> +   * beforeEach() and replace the real one for use by the framework
> +   * afterEach(.

Nit: afterEach()

Also, this last sentence would make a very good jsdoc for the whole module.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +34,5 @@
>      };
>  
> +    fakeWindow = {
> +      close: sandbox.stub(),
> +      //document: { addEventListener: function(){} }

Nit: should be deleted.

@@ +51,5 @@
> +
> +
> +  describe("ContactsList", function () {
> +    var listView;
> +    beforeEach(function() {

Nit: missing empty line above beforeEach.

@@ +56,5 @@
> +      navigator.mozLoop.calls = {
> +        startDirectCall: sandbox.stub(),
> +        clearCallInProgress: sandbox.stub()
> +      };
> +      navigator.mozLoop.contacts = {getAll: sandbox.stub()};

Nit: How about:

navigator.mozLoop = {
  calls: {…},
  contacts: {…}
};

@@ +62,5 @@
> +      listView = TestUtils.renderIntoDocument(loop.contacts.ContactsList());
> +    });
> +
> +    afterEach(function() {
> +      listView = null;

Nit: we should probably delete navigator.mozLoop here.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8533349 [details] [diff] [review]
Use WindowCloseMixin for existing, tested versions of window.close

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

LGTM, r=nperriault.

::: browser/components/loop/content/js/conversation.jsx
@@ +223,5 @@
>     * At the moment, it does more than that, these parts need refactoring out.
>     */
>    var IncomingConversationView = React.createClass({
> +    mixins: [sharedMixins.AudioMixin,
> +      sharedMixins.WindowCloseMixin],

Nit: Better indentation needed (make sure tis matches the style applied in similar situation).

::: browser/components/loop/content/js/conversationViews.jsx
@@ +193,5 @@
>     * Call failed view. Displayed when a call fails.
>     */
>    var CallFailedView = React.createClass({
> +    mixins: [Backbone.Events, sharedMixins.AudioMixin,
> +      sharedMixins.WindowCloseMixin],

Nit: indentation.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8533348 [details] [diff] [review]
Close panel on call conversation start

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

::: browser/components/loop/content/shared/js/mixins.js
@@ +69,5 @@
> +   * calling window.close() directly, use this mixin and call
> +   * this.closeWindow from your component.  In tests, use
> +   * loop.shared.mixins.setRootObject to substitute a fake window
> +   * beforeEach() and replace the real one for use by the framework
> +   * afterEach(.

Fixed.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +34,5 @@
>      };
>  
> +    fakeWindow = {
> +      close: sandbox.stub(),
> +      //document: { addEventListener: function(){} }

Done in the patch destined for nightly but not uplift.

@@ +51,5 @@
> +
> +
> +  describe("ContactsList", function () {
> +    var listView;
> +    beforeEach(function() {

Fixed.

@@ +56,5 @@
> +      navigator.mozLoop.calls = {
> +        startDirectCall: sandbox.stub(),
> +        clearCallInProgress: sandbox.stub()
> +      };
> +      navigator.mozLoop.contacts = {getAll: sandbox.stub()};

I thought I could do this while we were pairing, but I was wrong: navigator.mozLoop is actually defined in a higher scope in the tests.  I guess I could hoist or merge the objects, but that seems like more trouble than it's worth.

@@ +62,5 @@
> +      listView = TestUtils.renderIntoDocument(loop.contacts.ContactsList());
> +    });
> +
> +    afterEach(function() {
> +      listView = null;

Because the object itself is defined in an enclosing scope, the things that make sense to delete here are navigator.mozLoop.calls and navigator.mozLoop.contacts, so I've done that.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Close panel on call conversation start (landed fx-team; needs uplift to 35 after it clears m-c]

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8533861 [details] [diff] [review]
Close panel on call conversation start (landed fx-team; needs uplift to 35 after it clears m-c]

With review comments addressed; carrying forward Niko's r+.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: [landed fx-team; should NOT uplift after it clears m-c] Fix tested Loop callers of window.close to use WindowCloseMixin, r=NiKo`

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8533900 [details] [diff] [review]
[landed fx-team; should NOT uplift after it clears m-c] Fix tested Loop callers of window.close to use WindowCloseMixin, r=NiKo`

review comments fixed; carrying forward r+

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13c5e6f5657d
https://hg.mozilla.org/mozilla-central/rev/bcc39a19cd51

Comment 16

[Maire Reavy [:mreavy]]

Dan -- Is this supposed to close the panel when the user clicks "start a conversation" (and creates a new room)?  Or does the panel close only when the user clicks on an existing room?

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8533861 [details] [diff] [review]
Close panel on call conversation start (landed fx-team; needs uplift to 35 after it clears m-c]

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: UI annoyance (have to click to dismiss the panel after selecting a room)

[Describe test coverage new/current, TBPL]: on m-c for a few days; includes extensive tests (most of the patch)

[Risks and why]: low risk - just closes the panel on certain operations, includes extensive tests.

[String/UUID change made/needed]: none

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

mreavy: only when the user clicks an existing conversation.  The reason for this is that "Start a conversation" only adds another entry to the list, it doesn't actually open the conversation window.

And now that I'm thinking about it a little harder, I suspect that most people, most of the time when they press "Start a Conversation" would be happier if the conversation window opened for them without requiring an extra click (in which case closing the panel then would make sense).  I'd suggest pinging Sevaan and/or filing a bug on that...

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/abdf900b595d