Closed Bug 1080953 Opened 10 years ago Closed 9 years ago

UITour: tell page when first incoming call is received and if that room window is open

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
8

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: Dolske, Assigned: MattN)

References

Details

Attachments

(1 file, 1 obsolete file)

The Hello tour guides a user through creating their first Hello room and sharing that link. When the person receiving the link initiates a call (I.E. the tour user's first incoming call is received), the tour should resume. 

* If the tour page is still open, that tab should be selected. If the tour page is not open, a tab should be opened with the tour (and resumed at the proper point, mechanism TBD).

* If the room view for the call was already open, the call connects and everything is obvious. But if the view isn't open, the tour needs to know so that it can open the Hello panel, indicate how to click an active room, and know when the user has opened the room so the tour can continue. [This bug needs to tell the tour if the incoming call's room is open, those other steps will be implemented by other bugs.]

* Something will need to track tour status (probably the bug implementing the First Time Experience (FTE) panel for Hello), as we only want this bug to do things when finishing a tour. (So, a tristate flag: Tour not started, Tour waiting for incoming call, Tour Completed).

* In particular, we'd like to avoid annoying QA/developers who are not interested in a tour, and don't want the first call in testing profiles to be resuming a tour they never started.
No longer blocks: 1098620
This is probably a lower priority for now as it's not part of the reduced-scope tour.
After reviewing the reduced-scope wire frames during the PDX work-week, it looks like this bug is still required as part of the overall FTE flow.
(In reply to Alex Gibson [:agibson] from comment #2)
> After reviewing the reduced-scope wire frames during the PDX work-week, it
> looks like this bug is still required as part of the overall FTE flow.

IMO the wireframes from the PDX work week went back to the longer, mostly original, flow.

This is my proposal:

== Tour tab is closed when someone else joins a room for the first time after opening the tour ==
* no "incomingConversation" query parameter – User is opening the tour to start from the beginning.
* &incomingConversation=waiting – First incoming conversation after opening the tour and the 
                                  conversation window is closed. Point to the conversation list to tell 
                                  the user to open the conversation.
* &incomingConversation=open – First incoming conversation after opening the tour and the conversation 
                               window is already open. Don't show room list. Instead go to success page 
                               (end of tour).

== Tour tab is opened when someone else joins a room for the first time after opening the tour ==
One of the following will happen:
* "Loop:IncomingConversation" notification with details `conversationOpen: false`
* "Loop:IncomingConversation" notification with details `conversationOpen: true`


To clarify, the page will either get the query parameter with a page load in a new tab (if the URL wasn't open) XOR the event will be fired at the page.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 8
Flags: qe-verify-
Flags: firefox-backlog+
Attached file MozReview Request: bz://1080953/MattN (obsolete) —
/r/1421 - Bug 1080953 - UITour: tell page when first incoming call is received and if that room window is open.

Pull down this commit:

hg pull review -r a83c24ee066adc35197be5d6c117db2904b46ba0
Depends on: 1111013
What should we use for the `utm_campaign` query parameter when we resume the tour from the first conversation but the user had already closed the existing tour tab? My proposal was "resume-with-conversation"
Flags: needinfo?(cprice)
To be clear, you are referring to the 2nd row in the wireframe (http://cl.ly/image/3j2w0i0T0c1b) - "User clicks 'active' Hello icon in chrome", correct?

The value for utm_campaign was "getting-started" from the button (bug 1099462), and "settings-menu" from the gear menu (bug 1101286).

I recommend keeping with the convention of tagging the source, and use "hello-icon-resume".

Thanks
Flags: needinfo?(cprice)
(In reply to Cory Price [:ckprice] from comment #7)
> To be clear, you are referring to the 2nd row in the wireframe
> (http://cl.ly/image/3j2w0i0T0c1b) - "User clicks 'active' Hello icon in
> chrome", correct?

There isn't a single source/element entry point for opening a new tab:
* The user isn't in the room when someone else joins for the first time. We re-open the tab when the hello panel is opened next.
* The user is in the room when someone else joins for the first time. We open the tab immediately.
(In reply to Matthew N. [:MattN] from comment #8)
> There isn't a single source/element entry point for opening a new tab:
> * The user isn't in the room when someone else joins for the first time. We
> re-open the tab when the hello panel is opened next.
> * The user is in the room when someone else joins for the first time. We
> open the tab immediately.

For the purposes of this bug, "resume-with-conversation" will be sufficient.

The wireframe shows two different states depending on if the initiating user has the room open already vs if the initiating user has closed the room/tab and it has to be re-opened via the Hello icon. If the URL will be the same for these scenarios, we'll need to ensure we can detect which state to raise some other way.

I will continue this conversation in bug 1109132.

Thanks
Attachment #8535428 - Flags: review?(standard8)
Attachment #8535428 - Flags: review?(bmcbride)
/r/1421 - Bug 1080953 - UITour: tell page when first incoming call is received and if that room window is open.

Pull down this commit:

hg pull review -r d5a1089c742ae409b51dbed0a2c516a81393e79e
I didn't have time to make tests yet but there is pressure to get this landed ASAP so I'm putting the patch up for review without tests.
Try push: https://tbpl.mozilla.org/?tree=Try&rev=60b022dc08b3

Alex, you can test out this patch with the downloads from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@noorenberghe.ca-60b022dc08b3/ in a few hours.
Flags: needinfo?(agibson)
(In reply to Matthew N. [:MattN] from comment #12)
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=60b022dc08b3
> 
> Alex, you can test out this patch with the downloads from
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/
> mozilla@noorenberghe.ca-60b022dc08b3/ in a few hours.

Thanks, Matt!
Flags: needinfo?(agibson)
https://reviewboard.mozilla.org/r/1419/#review927

::: browser/base/content/browser-loop.js
(Diff revision 2)
> +        let roomsWithNonOwners = yield new Promise(resolve => {

I think this chunk wants to be its own function for ease of unit testing.  That said, if you'd prefer to make that change when you actually write the tests, that's fine.

::: browser/base/content/browser-loop.js
(Diff revision 2)
> +      }).then(Task.async(function* () {

This has made openCallPanel get really giant, to the point where a reader or maintainer is required to carry a lot of mental state.  If it's easy, I'd propose hoisting the anonymous function here into it's own non-anonymous function, but to ease that problem, and to improve testability.

::: browser/components/loop/MozLoopService.jsm
(Diff revision 2)
> +      // The particpant that joined isn't necessarily included in room.participants so concat

It would be nice if this comment explain how that's possible.

::: browser/components/loop/MozLoopService.jsm
(Diff revision 2)
> +    let hadExistingTab = win.switchToTabHavingURI(url, true, {

A comment explaining why the ignore stuff is in use would be helpful.

::: browser/components/loop/MozLoopService.jsm
(Diff revision 2)
> +        conversationOpen: aIncomingConversationState == "open",

nit: ===
https://reviewboard.mozilla.org/r/1419/#review931

Looks good; r=dmose with the various bits addressed.
Attachment #8535428 - Flags: review?(standard8)
Attachment #8535428 - Flags: review?(bmcbride)
(In reply to Dan Mosedale (:dmose) - use needinfo? (not reading bugmail regularly) from comment #14)
>
> This has made openCallPanel get really giant, to the point where a reader or
> maintainer is required to carry a lot of mental state.  If it's easy, I'd
> propose hoisting the anonymous function here into it's own non-anonymous
> function, but to ease that problem, and to improve testability.

"both to ease that problem".
(In reply to Matthew N. [:MattN] from comment #3)
> == Tour tab is opened when someone else joins a room for the first time
> after opening the tour ==
> One of the following will happen:
> * "Loop:IncomingConversation" notification with details `conversationOpen:
> false`
> * "Loop:IncomingConversation" notification with details `conversationOpen:
> true`

Testing on the build in Comment 12, I'm not seeing the "Loop:IncomingConversation" event fire (along with details `conversationOpen: false`) if I have closed the rooms view. Nothing happens in this case, until I open the hello panel and click to re-open the conversation. At this point, I get a ""Loop:IncomingConversation" event with the details `conversationOpen: true`?
(In reply to Alex Gibson [:agibson] from comment #18)
> (In reply to Matthew N. [:MattN] from comment #3)
> > == Tour tab is opened when someone else joins a room for the first time
> > after opening the tour ==
> > One of the following will happen:
> > * "Loop:IncomingConversation" notification with details `conversationOpen:
> > false`
> > * "Loop:IncomingConversation" notification with details `conversationOpen:
> > true`
> 
> Testing on the build in Comment 12, I'm not seeing the
> "Loop:IncomingConversation" event fire (along with details
> `conversationOpen: false`) if I have closed the rooms view. Nothing happens
> in this case, until I open the hello panel and click to re-open the
> conversation. At this point, I get a ""Loop:IncomingConversation" event with
> the details `conversationOpen: true`?

Further to this, I'm not seeing the event regardless to if the tab is or isn't visible when the other person connects (if that helps at all).
I just also got a `conversationOpen: true` event even though the conversation view wasn't open, which resulted in the page going into the wrong state?
After some investigation it seems the event with `conversationOpen: false` will fire when clicking the Hello icon, but *only* if I access the page from either the "Get Started" button or the "Tour" link. If I refresh the page to take the tour again, it no longer seems to work.

Is this the intended behavior? It seems inconsistent to if I leave the conversation view open, which behaves differently. Looking at the code, I assume this is due to `gettingStarted.resumeOnFirstJoin` being only set when first loading the tab?

I still have no idea how I got an event with `conversationOpen: true` when the view was not open.
Sorry for all the comments, but I notice another oddity: 

If I try the tour several times by going through the full flow in a single session, the browser seems to open progressively more and more tabs each time it reconnects the conversation, e.g. the following URL got opened 4 times each in a separate tab:

http://127.0.0.1:8000/en-US/firefox/37.0a1/hello/start/?incomingConversation=open&utm_source=firefox-browser&utm_medium=firefox-browser&utm_campaign=resume-with-conversation
Alex and I sorted out the issues and solutions in our weekly dev. meeting.

* Part of the problem was misunderstandings/miscommunication on how the events work.
* I had a TODO about moving the pref setting to another time to avoid nagging users who bail before the email/copy step but figured it could wait. Alex points out in comment 21 that setting the pref from the tour entrypoints can cause problems if someone visits the tour via a link shared from a friend or if they reload the tab and want the tour to continue.
** I will allow the page to set the pref when it wants (probably after email/copy are clicked)
* I will look into the issue with multiple tabs opening
Attachment #8535428 - Flags: review+
/r/1421 - Bug 1080953 - UITour: tell page when first incoming call is received and if that room window is open. r=dmose,dolske

Pull down this commit:

hg pull review -r d7500ef2a0921ad0d5d3a9e35bd8f80e8436d289
Attachment #8535428 - Flags: review?(dolske)
/r/1421 - Bug 1080953 - UITour: tell page when first incoming call is received and if that room window is open. r=dmose,dolske

Pull down this commit:

hg pull review -r d7500ef2a0921ad0d5d3a9e35bd8f80e8436d289
I'm writing some quick tests now. I wasn't able to repro the multiple tabs opening yet.  It would be good to know exactly which of the cases that happened in (e.g. conversation open/closed in advance and the entry point that lead to the tab opening).
/r/1421 - Bug 1080953 - UITour: tell page when first incoming call is received and if that room window is open. r=dmose,dolske

Pull down this commit:

hg pull review -r 1f2a1bc6dc11be5c0b2945bb9899d08654367287
Attachment #8535428 - Flags: review?(dolske) → review+
Comment on attachment 8535428 [details]
MozReview Request: bz://1080953/MattN

[Triage Comment]

Needed for Fx35 Hello tour. No unusual risk.
Attachment #8535428 - Flags: approval-mozilla-beta+
Attachment #8535428 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/fx-team/rev/8a4a29b0c603
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
Alex, you can download an updated build of what landed plus bug 1112565 at http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-macosx64/1418877969/ since I'm not sure if it will make it in today's Nightly.
On(In reply to Matthew N. [:MattN] from comment #31)
> Alex, you can download an updated build of what landed plus bug 1112565 at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-
> macosx64/1418877969/ since I'm not sure if it will make it in today's
> Nightly.

An issues notice with the latest changes:

- I leave the tab open after sharing the link, but close the conversation view:

When the other connects, the hello icon turns blue as expected. I go to click it, but the hello panel doesn't open. At this point I get sent *lots* of "Loop:IncomingConversation" events to the point where it starts to slow down the browser.

This also seems to effect all scenarios where the conversation connects. The next time the hello icon is clicked, I get hundreds of events one after the other?
Also I wonder if there's a risk of making the hello panel sticky automatically when the tab re-opens - if the web page doesn't land in a state where the panel can be dismissed again (e.g. the conversation connects successfully), then could the user be stuck with a panel they can't close?
https://hg.mozilla.org/mozilla-central/rev/8a4a29b0c603
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Depends on: 1113247
Filed Bug 1113247 as a follow up to some possible issues mentioned in Comment 32
(In reply to Alex Gibson [:agibson] from comment #33)
> Also I wonder if there's a risk of making the hello panel sticky
> automatically when the tab re-opens - if the web page doesn't land in a
> state where the panel can be dismissed again (e.g. the conversation connects
> successfully), then could the user be stuck with a panel they can't close?

I'm not sure I understand but the page doesn't need to call hideMenu("loop") anymore since bug 1112565 as it will close on its own before switching tabs so I'm not sure how the hello panel would stay sticky. I'm looking into why clicking the hello button after a showMenu("loop") doesn't allow manually closing the panel like it does for "appMenu".
(In reply to Matthew N. [:MattN] from comment #36)
> I'm not sure I understand but the page doesn't need to call hideMenu("loop")
> anymore since bug 1112565 as it will close on its own before switching tabs
> so I'm not sure how the hello panel would stay sticky. I'm looking into why
> clicking the hello button after a showMenu("loop") doesn't allow manually
> closing the panel like it does for "appMenu".

The page isn't calling hideMenu("loop") on load. I think I was hitting a side effect of where I was going wrong in Bug 1113247. It was causing the hello button to behave incorrectly. But it's a good point that showMenu("loop") does seem to prevent the user from closing the panel again.
(In reply to Alex Gibson [:agibson] from comment #37)
> But it's a good point that showMenu("loop") does seem to prevent the user from closing the panel again.

It's not hard to fix but I'm not sure it's worth it since switching away from a tour tab will also dismiss the panel. This is only an issue with UITour since normally clicking outside the panel (on its anchor) would dismiss the panel anyways.
Depends on: 1113739
Attachment #8535428 - Attachment is obsolete: true
Attachment #8618402 - Flags: review+
You need to log in before you can comment on or make changes to this bug.