Closed Bug 1093787 Opened 10 years ago Closed 9 years ago

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

Categories

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

defect
Points:
8

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
backlog Fx36+

People

(Reporter: dmosedale, Assigned: standard8)

References

Details

(Whiteboard: [standalone][todo: see user story][good first verify])

User Story

Expected end result:

* when I click the "start" button
** it switches to the view with the red cancel button
** the "Ready to start your conversation" text is replaced by "Please give your browser permission to share your camera and microphone" 

Things to test: 

* how does this behave when users have said "always grant permissions" to Hello?
* "Never allow"?

Attachments

(1 file, 9 obsolete files)

38.67 KB, patch
NiKo
: review+
Details | Diff | Splinter Review
+++ 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.
No longer blocks: 1039224, 1047040, 1069031, 1091316
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.
User Story: (updated)
Assignee: nobody → 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?)
Points: --- → 8
Attached patch Fix up gum edge cases, WIP (obsolete) — Splinter Review
Attachment #8518566 - Attachment is obsolete: true
Attachment #8520652 - Attachment is obsolete: true
Attachment #8520824 - Attachment is obsolete: true
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.
Attachment #8521063 - Attachment description: Fix standalone start button brokenness, WIP → Fix standalone start button brokenness, seems to work
Attachment #8521063 - Flags: review+
Attachment #8521063 - Flags: review+ → feedback?(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.
Attachment #8521063 - Flags: feedback?(standard8) → feedback+
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.
Attachment #8522592 - Attachment is obsolete: true
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
Attachment #8523365 - Attachment is obsolete: true
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...
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.
Assignee: dmose → standard8
Attachment #8523369 - Attachment is obsolete: true
backlog: --- → Fx36+
Attached patch WIP - Drop the cancel button (obsolete) — Splinter Review
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).
Attachment #8521063 - Attachment is obsolete: true
Attachment #8523370 - Attachment is obsolete: true
Blocks: 1047040
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.
Attachment #8527774 - Attachment is obsolete: true
Attachment #8527883 - Flags: review?(nperriault)
Iteration: --- → 36.3
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.
Attachment #8527883 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/mozilla-central/rev/51428a851d5d
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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)
Attachment #8527883 - Flags: approval-mozilla-aurora?
Flags: qe-verify-
Flags: in-moztrap-
Whiteboard: [standalone][todo: see user story] → [standalone][todo: see user story][good first verify]
Attachment #8527883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.