1093787 - if gUM dialog is hidden, the standalone "start" button does nothing until it is unhidden and clicked

Status: RESOLVED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

+++ This bug was initially created as a clone of Bug #1073410 +++

Because the dialog can easily become inadvertently hidden (eg by giving focus to something other than Firefox), this is a super frustrating situation for the user.

The long-term fix is to improve the dropdown (handled in other bugs), and the medium term workaround is to use something image-like to direct the user how to do this (tracked in bug 1047040).

However, we need to do _something_ about this before Fx34 hits release if at all possible, because when it happens, the appearance to many novice is just "this is unfixably broken", because they don't find the icon to unhide the dialog.

So, as an interim, we want to do the simplest thing we can to make it clear that the user needs to take some action.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

So we _could_ choose to display one of the images adam proposes in bug 1073410 comment 14 as part of this bug, but I think we need to do the things in the user story anyway, and those things are already big enough.  So my gut is that we should leave that out, and either file a separate bug, or just wait for bug 1047040.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix standalone start button brokenness, WIP

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Spike patch is attached.  It mostly works, but things continue to be in a bug whack-a-mole stage.  Clicking on the cancel button in a few situations has various different problems that needed to be debugged/fixed.  At least one of them results in a multiplexGum infinite loop.  Cancel button stuff to poke at next:

          // * fast click and we've lost some race (still exists?)
          // * doorhanger hidden or "Not Now" clicked (doesn't re-ask on start)
          // * "Don't Share" clicked (works?)

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix up gum edge cases, WIP

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix standalone start button brokenness, WIP

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix standalone start button brokenness, seems to work

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8521063 [details] [diff] [review]
Fix standalone start button brokenness, seems to work

Here's a slightly cleaned up version of what we were poking at today.  I'd love your thoughts on moving forward with this.  Note that the unit tests aren't currently passing.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8521063 [details] [diff] [review]
Fix standalone start button brokenness, seems to work

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

Yeah, this is complicated, but I don't see that we can do much better with the APIs that we have.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix standalone start button brokenness, WIP, updated but flaky

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Yuck.  The flakiness is because there was a bad merge that caused some stuff from the .js file to get merged into the .jsx file.  Next step is to unhork that.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix standalone start button brokenness, WIP

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

What looked like a bad merge (as suggested in comment 10) was actually my editor being overzealous in reformatting.  It's not totally clear to me what the flakiness problems with the now-obsoleted version were, but I couldn't really test that today due to the push server unreliability issues.

This is a cleaned up version of the patch, which doesn't have all the unnecessary reformatting, and, I hope, will be fairly stable.  All the tests now pass, and some new ones have been written, but there are still more to write.

There list of things to do currently looks something like this:

* fix case of granting "allow access" to gUM prompt, call state goes to connecting, link-clicker presses "cancel", the views reset, but the camera is still shared (orange icon still up).  Not sure what's going one here; Chrome doesn't seem to have an analogous issue.  Best bet for debugging this is likely to add more logging.
* factor out most logging to some utility function so that it's off by default
* write abortCall tests
* write infinite recursion test for mGUM
* review patch to see what other changed code needs tests & write them
* make sure everything still works in Google Chrome
* add any necessary jsdoc comments

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix standalone start button brokenness, WIP

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Patch is against fx-team. The reason I've left the earlier feedback+ patch un-obsoleted is because I haven't been able to test whether this current patch is reliably, rather than flaky.  If the current patch turns out to be flaky, it may be a useful test to try the older patch, which hasn't been flaky in the past.  I'll be surprised if this turns out to be necessary, but...

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Re-assigning to Standard8, as I'm turning into a pumpkin.  He may well not be the right owner for this, given the other things on his plate, but I'm confident he can find a good one if he's not.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix standalone start button brokenness, WIP

Comment 17

[Mark Banner (:standard8)]

Attachment: WIP - Drop the cancel button

Simplified patch that drops the cancel button to remove complexity - i.e. the user has to either accept or deny the prompt to progress (or reload).

Comment 18

[Mark Banner (:standard8)]

Attachment: Insert an additional view for Loop standalone calls to prompt the user to accept the microphone and camera permissions before starting the call.

The short summary of this patch, is that we're inserting a new state and view into the flow for the standalone. This handles the getting of the media permissions and appropriate display.

Dropping the cancel button means that we don't need to deal with a lot of the flow of the previous patches (and browser edge cases). It also in some ways feels a nicer flow now.

Some of the flows in this patch are a bit messy - that's because we're in the non-flux code, so we can't do much about that. However, I think this patch is quite safe.

Comment 19

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

Comment on attachment 8527883 [details] [diff] [review]
Insert an additional view for Loop standalone calls to prompt the user to accept the microphone and camera permissions before starting the call.

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

Looks good, and improves UX. Small nits to be optionally addressed.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +269,5 @@
>        );
>      }
>    });
>  
>    var PendingConversationView = React.createClass({

Nit: Missing short jsdoc.

@@ +277,5 @@
> +      cancelCallback: React.PropTypes.func
> +    },
> +
> +    render: function() {
> +      // XXX Hide cancel

This can be removed.

@@ +317,5 @@
> +      );
> +    }
> +  });
> +
> +  var GumPromptConversationView = React.createClass({

Nit: Missing short jsdoc.

@@ +323,5 @@
> +      var callState = mozL10n.get("call_progress_getting_media_description",
> +                                  {clientShortname: mozL10n.get("clientShortname2")});
> +      document.title = mozL10n.get("standalone_title_with_status",
> +                                   {clientShortname: mozL10n.get("clientShortname2"),
> +                                    currentStatus: mozL10n.get("call_progress_getting_media_title")});

Nit: I'd rather format these that way:

mozL10n.get("standalone_title_with_status", {
  clientShortname: mozL10n.get("clientShortname2"),
  currentStatus: mozL10n.get("call_progress_getting_media_title")
});

@@ +327,5 @@
> +                                    currentStatus: mozL10n.get("call_progress_getting_media_title")});
> +
> +      return (
> +        <PendingConversationView callState={callState}/>
> +      );

Nit: I think you can directly return <PendingConversationView callState={callState} />;

@@ +331,5 @@
> +      );
> +    }
> +  });
> +
> +  var WaitingConversationView = React.createClass({

Nit: Missing short jsdoc.

::: browser/components/loop/test/standalone/webapp_test.js
@@ +364,5 @@
>  
>        describe("session:ended", function() {
> +        it("should call multiplexGum.reset", function() {
> +          var multiplexGum = new standaloneMedia._MultiplexGum();
> +            standaloneMedia.setSingleton(multiplexGum);

Nit: indentation

@@ +477,5 @@
>              conversation.set("loopToken", "");
>            });
>  
>            it("should display the FailedConversationView", function() {
> +            conversation.trigger("call:outgoing:setup");

It's strange we don't call the tested method anymore here (#setupOutgoingCall)… Could this move to some Events test section? Or at least rename the section to avoid confusing the reader too much.

Comment 20

[Mark Banner (:standard8)]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51428a851d5d

Comment 22

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

Comment on attachment 8527883 [details] [diff] [review]
Insert an additional view for Loop standalone calls to prompt the user to accept the microphone and camera permissions before starting the call.

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Not a lot other than possible merge pain on future bugfixes.  This is largely standalone code used on the server by people joining a room from a link.

[Describe test coverage new/current, TBPL]: Largely manually tested

[Risks and why]: almost no risk, as the changes are to the standalone code.

[String/UUID change made/needed]: none (that are in the build)

Comment 23

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/390a34a40ea4