1053939 - pass entry point to fxa content server when opening about:accounts

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Katie Parlante]

We'd like to know the entry points users actually take to initially set up sync: Menu item, hamburger, or home screen?

It would be useful to have this passed in on the URL to the fxa-content-server, in addition to the context & service arguments: this is the ask.

Telemetry probe or FHR data is another option.

We're also interested in knowing the number of people who see the intro screen but don't hit the "Get Started" button. (Similarly for the "Manage" screen).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to Katie Parlante from comment #0)
> We'd like to know the entry points users actually take to initially set up
> sync: Menu item, hamburger, or home screen?
> 
> It would be useful to have this passed in on the URL to the
> fxa-content-server, in addition to the context & service arguments: this is
> the ask.

This sounds reasonable.

> Telemetry probe or FHR data is another option.

This seems more complicated for the question above.

> We're also interested in knowing the number of people who see the intro
> screen but don't hit the "Get Started" button. (Similarly for the "Manage"
> screen).

I'm not sure how to best get this data, probably via a telemetry probe. Seems best handled in a separate bug.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Implementation strategy here is:
- adjust aboutaccounts.js's init() to handle a separate "entrypoint" query string parameter to about:accounts indicating the UI entry point that caused the page to open, and pass that through to wrapper.init() (which actually loads the relevant content server URL), probably just by appending to whatever URL we'd otherwise load
- adjust all UI entry points that cause about:accounts to load to pass in a unique  "entrypoint" parameter (in addition to the existing "action" parameter)

Comment 3

[Chris Karlof [:ckarlof]]

Related content server bug: https://github.com/mozilla/fxa-content-server/issues/1568

Comment 4

[Katie Parlante]

Sounds good, thanks Gavin.

> > We're also interested in knowing the number of people who see the intro
> > screen but don't hit the "Get Started" button. (Similarly for the "Manage"
> > screen).
> 
> I'm not sure how to best get this data, probably via a telemetry probe.
> Seems best handled in a separate bug.

Agreed, here: https://bugzilla.mozilla.org/show_bug.cgi?id=1054408

Comment 5

[Alex Bardas :alexbardas]

Attachment: Sync button from menu panel

Main entry points for sync are: abouthome, uitour, preferences menu, menu panel, toolbar, menubar (tools > set up sync).

There can also be a sync button added from the customizable menu to the toolbar or menu panel. I'm using the same name for that button so far ("syncbutton"), should I differentiate that entry point somehow?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Alex Bardas :alexbardas from comment #5)
> Main entry points for sync are: abouthome, uitour, preferences menu, menu
> panel, toolbar, menubar (tools > set up sync).

By "menu panel" you mean the "Sign in to sync" area there, right? What do you mean by "toolbar"? Do you mean the button mentioned below?

> There can also be a sync button added from the customizable menu to the
> toolbar or menu panel. I'm using the same name for that button so far
> ("syncbutton"), should I differentiate that entry point somehow?

I don't think it's necessary or useful to distinguish this button on the toolbar vs. this button in the menu panel.

Comment 7

[Katie Parlante]

> > There can also be a sync button added from the customizable menu to the
> > toolbar or menu panel. I'm using the same name for that button so far
> > ("syncbutton"), should I differentiate that entry point somehow?
> 
> I don't think it's necessary or useful to distinguish this button on the
> toolbar vs. this button in the menu panel.

Agreed.

Comment 8

[Alex Bardas :alexbardas]

Attachment: bug1053939_pass_entry_point_to_fxa_server_when_opening_about_accounts.diff

Comment 9

[Alex Bardas :alexbardas]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #6)
> (In reply to Alex Bardas :alexbardas from comment #5)
> > Main entry points for sync are: abouthome, uitour, preferences menu, menu
> > panel, toolbar, menubar (tools > set up sync).
> 
> By "menu panel" you mean the "Sign in to sync" area there, right? What do
> you mean by "toolbar"? Do you mean the button mentioned below?
> 

I've called the entrypoint from "Sign in to sync" menupanel, but I can easily rename it if there is a better name for that.

Comment 10

[Alex Bardas :alexbardas]

Attachment: [Patch] Pass entry point to fxa server when opening about accounts

Comment 11

[Alex Bardas :alexbardas]

Attachment: [Patch] Pass entry point to fxa server when opening about accounts

Use replaceQueryString for all switchToTabHavingURI("about:accounts...")

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8484259 [details] [diff] [review]
[Patch] Pass entry point to fxa server when opening about accounts

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

LGTM - r+ with the requested changes.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +95,5 @@
>  
>  let wrapper = {
>    iframe: null,
>  
> +  init: function (url=null, entryPoint) {

I think we should drop the default for the first param, and explicitly pass fxAccounts.getAccountsSignUpURI() in the couple of cases we rely on the default (with this patch we always pass that first param explicitly, and explicitly passing null doesn't have any real value for the 2 times we do it)

::: browser/base/content/browser-syncui.js
@@ +295,5 @@
>     *        Indicates type of wizard to launch:
>     *          null    -- regular set up wizard
>     *          "pair"  -- pair a device first
>     *          "reset" -- reset sync
>     */

We should add brief docs for this new param in the comment block

::: browser/base/content/test/general/browser_bug1025195_switchToTabHavingURI_ignoreFragment.js
@@ +43,5 @@
> +  switchTab("about:home?hello=firefoxos", false, false, false);
> +  // Remove the last opened tab to test replaceQueryString option.
> +  gBrowser.removeCurrentTab();
> +  switchTab("about:home?hello=firefoxos", false, true, true);
> +  is(tabRefAboutHome, gBrowser.selectedTab, "Selected tab should be the initial about:accounts tab");

I think this comment should read about:home

@@ +58,3 @@
>    is(tabFound, aShouldFindExistingTab,
>       "Should switch to existing " + aURI + " tab if one existed, " +
>       (aIgnoreFragment ? "ignoring" : "including") + " fragment portion");

probably want an extra message here like "and " + (aReplaceQueryString ? ... " query string");

::: browser/components/preferences/in-content/sync.js
@@ +349,5 @@
>      gSyncUtils.openToS();
>    },
>  
>    signUp: function() {
> +    this.openContentInBrowser("about:accounts?action=signup&entrypoint=preferences");

openContentInBrowser in both these sync.js files is going to need the new param to switchToTabHavingURI

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8484259 [details] [diff] [review]
[Patch] Pass entry point to fxa server when opening about accounts

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

::: browser/base/content/browser-syncui.js
@@ +297,5 @@
>     *          "pair"  -- pair a device first
>     *          "reset" -- reset sync
>     */
>  
> +  openSetup: function SUI_openSetup(wizardType, entryPoint="syncbutton") {

also, a very minor nit - even though you probably copied this style from aboutaccounts.js, I think we are starting to standardize on spaces around the "=" for default params.

Comment 14

[Katie Parlante]

A couple of questions...

1. I'm assuming "uitour" entry point is passed along in this cirumstance:
- user chooses "firefox tour" from the help menu
- user takes the tour (or doesn't), lands on https://www.mozilla.org/en-US/firefox/31.0/tour/ (and sees the sync promotion)
- user clicks "get started with sync"
- user clicks "sign in to sync" from menu
- user lands on "about:home" with the "uitour" entrypoint

I'm also assuming the same thing happens if the user gets to that "sync promotion" page from update or download (the url looks slightly different, and the text may be slightly different, but the "get started with sync" button is the same.

Do I have this right?

2. The snippet team is about to launch a sync snippet. One version directs the user to "about:home". Another version directs the user to a new sync landing page: https://www.mozilla.org/en-US/firefox/sync/, which asks the user to go to the "sign into sync" menu option. Will we/can we distinguish these entry points?

Comment 15

[Alex Bardas :alexbardas]

Attachment: [Patch] Pass entry point to fxa server when opening about accounts

I've added `uitour` entrypoint for cases where user is in uitour while trying to go to the sync page + tests.

Comment 16

[Katie Parlante]

Whoops, should be "about:accounts" in https://bugzilla.mozilla.org/show_bug.cgi?id=1053939#c14

Comment 17

[Alex Bardas :alexbardas]

Try results where the patch can be tested:

https://tbpl.mozilla.org/?tree=Try&rev=1995311eeaf6

The errors seem to be unrelated to the current patch, but I'll update my local fx-team repo and push again to try after it is r+.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

(In reply to Katie Parlante from comment #14)
> A couple of questions...

I think the most recent patch has most of these covered, but to clarify:

> 
> 1. I'm assuming "uitour" entry point is passed along in this cirumstance:
> - user chooses "firefox tour" from the help menu
> - user takes the tour (or doesn't), lands on
> https://www.mozilla.org/en-US/firefox/31.0/tour/ (and sees the sync
> promotion)

The "Sync Promotion" in the UI Tour just seems to open the hamburger menu with the blue highlight hovering over the "Signup to Sync" item - this is what you mean, right?

> - user clicks "get started with sync"
> - user clicks "sign in to sync" from menu
> - user lands on "about:home" with the "uitour" entrypoint

Even assuming you meant "about:accounts", I'm not sure what this last point refers to - but for every other point, yes, the entrypoint will be "uitour".

Note also that if the UITour is active, selecting the Sync items on the "real menu" (ie, not the hamburger menu) will also return uitour as the entrypoint, which seems like a bit of an edge case, but would be difficult to change.  In all other cases, this "real menu" is going to say "menupanel", so we really wont be able to differentiate the "real menu" from the "hamburger menu".

> I'm also assuming the same thing happens if the user gets to that "sync
> promotion" page from update or download (the url looks slightly different,
> and the text may be slightly different, but the "get started with sync"
> button is the same.

I'm not sure what this refers to - can you please expand on this?

> 2. The snippet team is about to launch a sync snippet. One version directs
> the user to "about:home".

It's not clear to me how this snippet will arrange to open about:accounts - if it does this via some snippet-specific code, this code will need to manually append the 'entrypoint' param manually.

> Another version directs the user to a new sync
> landing page: https://www.mozilla.org/en-US/firefox/sync/, which asks the
> user to go to the "sign into sync" menu option. Will we/can we distinguish
> these entry points?

If the user manually selects the menu item in this case, the entry-point is going to be 'menupanel'.

It would be great if you can clarify the above and/or confirm what I described is acceptable.  Also, Alex just sent a link to a try build - it would be great if you can try this out and verify it does what you need.

Comment 19

[Katie Parlante]

I tested out the try build, and it works great. Further clarification:

> The "Sync Promotion" in the UI Tour just seems to open the hamburger menu
> with the blue highlight hovering over the "Signup to Sync" item - this is
> what you mean, right?

Yes, that is what I meant.

> Even assuming you meant "about:accounts", I'm not sure what this last point
> refers to - but for every other point, yes, the entrypoint will be "uitour".

Yeah, my bad, I meant "about:accounts" -- the result of the user clicking on the "Signup to Sync" with the blue highlight circle. I tested it out and it works as expected.

> Note also that if the UITour is active, selecting the Sync items on the
> "real menu" (ie, not the hamburger menu) will also return uitour as the
> entrypoint, which seems like a bit of an edge case, but would be difficult
> to change.  In all other cases, this "real menu" is going to say
> "menupanel", so we really wont be able to differentiate the "real menu" from
> the "hamburger menu".

Thats fine.

> > I'm also assuming the same thing happens if the user gets to that "sync
> > promotion" page from update or download (the url looks slightly different,
> > and the text may be slightly different, but the "get started with sync"
> > button is the same.
> 
> I'm not sure what this refers to - can you please expand on this?

There are different ways to get to the "uitour", looks like it all works as expected.

> > 2. The snippet team is about to launch a sync snippet. One version directs
> > the user to "about:home".
> 
> It's not clear to me how this snippet will arrange to open about:accounts -
> if it does this via some snippet-specific code, this code will need to
> manually append the 'entrypoint' param manually.

When testing I happened to get this snippet, looks like it sends the "uitour" entrypoint. I'll follow up with the snippet team to see how they invoke this.

> > Another version directs the user to a new sync
> > landing page: https://www.mozilla.org/en-US/firefox/sync/, which asks the
> > user to go to the "sign into sync" menu option. Will we/can we distinguish
> > these entry points?
> 
> If the user manually selects the menu item in this case, the entry-point is
> going to be 'menupanel'.

Yup, makes sense. The snippet team wants to be able to do A/B testing of the two approaches (invoking the about:accounts page directly or sending the user to a sync landing page); I think they need to do some tweaking on their end to make that happen. 

> It would be great if you can clarify the above and/or confirm what I
> described is acceptable.  Also, Alex just sent a link to a try build - it
> would be great if you can try this out and verify it does what you need.

The current patch meets the original intent of the bug. We'll create a separate bug for the snippets A/B testing, as that probably involves the snippet code and choices about the design of the sync landing page.

Thanks for the work!

Comment 20

[Alex Bardas :alexbardas]

The try push looks almost good, but there is a problem on Windows Debug with bc1:

https://tbpl.mozilla.org/?tree=Try&rev=d6e06d96f96d

Mark, do you see any obvious solution to fix the test that is failing?

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8484517 [details] [diff] [review]
[Patch] Pass entry point to fxa server when opening about accounts

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

I think this patch looks good, but I'm deferring review until we hear back from mihaela.velimiroviciu.  In the meantime you might like to try adding that param back in and retrigger bc1 tests a number of times to ensure the orange goes away.

::: browser/components/customizableui/test/browser_967000_button_sync.js
@@ +28,5 @@
>    ok(syncButton, "The Sync button was added to the Panel Menu");
>    syncButton.click();
>  
>    newTab = gBrowser.selectedTab;
> +  yield promiseTabLoadEvent(newTab);

I'm a little confused by this and the test orange:

* The orange mentioned in comment 20 is caused by an error thrown in promiseTabLoadEvent - at face value, that would be because you are not passing the second param - the URL to load.

* Confusingly though, I re-triggered the job on try and the orange is indeed intermittent - how this works at all without that second param is strange to me.

* Even more confusingly, promiseTabLoadEvent explicitly loads the requested URL - but IIUC, the point of this test is to ensure the syncButton.click() causes about:accounts to load - so why we then explicitly load it is a mystery to me.

needinfo mihaela.velimiroviciu to see if we can get some insight here - it may be that simply re-adding that second param causes the orange to go away, but the test still seems wrong, or I'm missing something.

Comment 22

[Mihaela Velimiroviciu (:mihaelav)]

I'm not 100% sure, but I think it may be because browser_aboutHome.js test uses synthesizeMouseAtCenter, which seems to be faulty (see bug 1027181 comment 241). May try to do a <element>.click() instead...

Comment 23

[Alex Bardas :alexbardas]

I've taken a closer look and I realized that I've removed the second argument (the uri to load) because I was used with this method http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#568 , which is in /base/content and is not used here. 

For me that method looks like what we need here, because we need to make sure that the tab is loaded after the syncButton.click() happened. 

I'd rather fix promiseTabLoadEvent from customizableui/test/head.js and make that argument optional. It should not affect the current behavior from other tests at all.

Comment 24

[Alex Bardas :alexbardas]

Attachment: Pass entry point to fxa server when opening about accounts

I've fixed promiseTabLoadEvent to allow an optional aURL. I'm waiting for the new try results here: 

https://tbpl.mozilla.org/?tree=Try&rev=03c7990f2109

Comment 25

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8486255 [details] [diff] [review]
Pass entry point to fxa server when opening about accounts

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

I don't like this version as the timing could be wrong - the click is what should load the tab, so adding the event handler after the thing that causes the tab to load might go orange.  This is the same reason promiseTabLoadEvent adds the event handler before loading the URL.  So making the URL optional doesn't make sense IMO - anyone using that must already have triggered the tab load and thus might be too late to get the event.

I think we should just fix the test to instead not use this function and manually add a handler before it does the .click() - I suspect that will solve the problem.

Comment 26

[Alex Bardas :alexbardas]

Attachment: Pass entry point to fxa server when opening about accounts

After pressing the sync button, generally a new tab opens and loads the about:accounts page (so adding an event before is impossible because the new tab does not exist yet). If the user is on about:newtab, then about:accounts is loaded in the same tab. I can make sure the user is on about:newtab page every time, but if this behaviour will change, then the test must be rewritten.

I found a middle way, not using an event and testing for contentDocument.readyState === "complete". I had to use waitForCondition for this approach. What do you think?

Comment 27

[Mark Hammond [:markh] [:mhammond]]

(In reply to Alex Bardas :alexbardas from comment #26)
> Created attachment 8486533 [details] [diff] [review]
> Pass entry point to fxa server when opening about accounts
> 
> After pressing the sync button, generally a new tab opens and loads the
> about:accounts page (so adding an event before is impossible because the new
> tab does not exist yet). If the user is on about:newtab, then about:accounts
> is loaded in the same tab.

In that case we can put the load event directly on the tab.

> I can make sure the user is on about:newtab page
> every time, but if this behaviour will change, then the test must be
> rewritten.

If that's a concern the event could be put directly on gBrowser - but note that in that case the existing code in the test would need changing anyway - it already assumes the current tab is replaced as this is the tab is passes into promiseTabLoadEvent.

eg, the following patch works when the handler it put on the tab and directly on gBrowser, but I think keeping the assumption the current tab is replaced is fine, and I prefer this to loops and readyState checking.

--- a/browser/components/customizableui/test/browser_967000_button_sync.js
+++ b/browser/components/customizableui/test/browser_967000_button_sync.js
@@ -19,10 +19,16 @@ add_task(function() {

   let syncButton = document.getElementById("sync-button");
   ok(syncButton, "The Sync button was added to the Panel Menu");
+  let deferredCreated = Promise.defer();
+  let handler = event => {
+    deferredCreated.resolve();
+    gBrowser.selectedTab.removeEventListener("load", handler, true);
+  }
+  gBrowser.selectedTab.addEventListener("load", handler, true);
   syncButton.click();
+  yield deferredCreated.promise;

   newTab = gBrowser.selectedTab;
-  yield promiseTabLoadEvent(newTab, "about:accounts");

   is(gBrowser.currentURI.spec, "about:accounts", "Firefox Sync page opened");
   ok(!isPanelUIOpen(), "The panel closed");

Comment 28

[Alex Bardas :alexbardas]

Attachment: Pass entry point to fxa server when opening about accounts

It is much better to listen for an event, indeed. I've also added a comment that we expect the about:accounts page to open in the current tab.

The test was broken before because about:accounts was opened every time, regardless the sync button was clicked or not.

Thank you for helping me with this, Mark :).

Comment 29

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8486944 [details] [diff] [review]
Pass entry point to fxa server when opening about accounts

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

thanks!

Comment 30

[Shane Tomlinson [:stomlinson]]

:markh, :alexbardas, both `syncButton` and `syncbutton` are used for the entryPoint. Can you update these to be consistent?

+  openSetup: function SUI_openSetup(wizardType, entryPoint = "syncbutton") {

+  openSignInAgainPage: function (entryPoint = "syncButton") {

Comment 31

[Alex Bardas :alexbardas]

(In reply to Shane Tomlinson [:stomlinson] from comment #30)
> :markh, :alexbardas, both `syncButton` and `syncbutton` are used for the
> entryPoint. Can you update these to be consistent?
> 
> +  openSetup: function SUI_openSetup(wizardType, entryPoint = "syncbutton") {
> 
> +  openSignInAgainPage: function (entryPoint = "syncButton") {

Yes, good catch. I'll convert everything to lowercase.

Comment 32

[Alex Bardas :alexbardas]

Attachment: Pass entry point to fxa server when opening about accounts

Used "syncbutton" instead of "syncButton" for an entrypoint. Try looks good:

https://tbpl.mozilla.org/?tree=Try&rev=89a6bf8c507d

There are some errors, but they have nothing to do with this bug.

Comment 33

[Carsten Book [:Tomcat]]

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

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08ba579779b1

Comment 35

[Chris More [:cmore]]

Outside of having source information, will we have any information related to different campaigns within a source? For example, imagine we did two different snippets and from the about:home source, could be differentiate those two campaigns and determine which one is more successful on signup or will they both look the same in this data? Is there any way to pass parameters or campaign IDs to this data source to be able to understand the effectiveness of campaigns or is this bug solely about the source?

Comment 36

[Alex Bardas :alexbardas]

(In reply to Chris More [:cmore] from comment #35)
> Outside of having source information, will we have any information related
> to different campaigns within a source? For example, imagine we did two
> different snippets and from the about:home source, could be differentiate
> those two campaigns and determine which one is more successful on signup or
> will they both look the same in this data? Is there any way to pass
> parameters or campaign IDs to this data source to be able to understand the
> effectiveness of campaigns or is this bug solely about the source?

It wasn't in the scope of this bug which is only about the source and it doesn't pass campaign IDs right now (only "&entrypoint=abouthome"). 

That can be solved in a new bug. If you want to file it, please cc me and mark to it.

Comment 37

[Chris More [:cmore]]

(In reply to Alex Bardas :alexbardas from comment #36)
> (In reply to Chris More [:cmore] from comment #35)
> > Outside of having source information, will we have any information related
> > to different campaigns within a source? For example, imagine we did two
> > different snippets and from the about:home source, could be differentiate
> > those two campaigns and determine which one is more successful on signup or
> > will they both look the same in this data? Is there any way to pass
> > parameters or campaign IDs to this data source to be able to understand the
> > effectiveness of campaigns or is this bug solely about the source?
> 
> It wasn't in the scope of this bug which is only about the source and it
> doesn't pass campaign IDs right now (only "&entrypoint=abouthome"). 
> 
> That can be solved in a new bug. If you want to file it, please cc me and
> mark to it.

Thanks for the clarification.

Comment 38

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified this in Firefox 35 Beta 6 (BuildID=20141222200458) on Windows 7 x64, Mac OS X 10.9.5 and Ubuntu 14.04 x64. I got the following entry points as expected: abouthome, uitour, preferences, menupanel, menubar, syncbutton.

Note that the "uitour" entry point does not work with e10s in Nightly. This was filed as bug 1115345.