1152385 - Change sync entries points linking to about:accounts to link to about:preferences#sync

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Ryan Feeley [:rfeeley]]

With in-content preferences launching in release, and account:preferences#sync getting a small makeover, it makes sense that the sync experiences links to account:preferences#sync instead of about:accounts.

Let's discuss.

Comment 1

[Shane Tomlinson [:stomlinson]]

Ryan, can you upload your latest mocks to give a preview of what you are thinking?

Comment 2

[Ryan Feeley [:rfeeley]]

I don't this this bug has any bearing on what about:preferences#sync looks like, just that we change all pointers in the code to it from about:accounts.

Related, we are changing the styling of the page slightly though:
https://bugzilla.mozilla.org/show_bug.cgi?id=1149721

Comment 3

[Chris Karlof [:ckarlof]]

Ryan, in content prefs are out. What pointers did you have in mind? There are lots of places that launch the login flow to Sync by opening about:accounts directly. Did you want them to visit the sync preferences pane instead?

Comment 4

[Chris Karlof [:ckarlof]]

One point I identified in "Sign in to Sync" in the hamburger menu after you create an account and don't verify.

Comment 5

[Ryan Feeley [:rfeeley]]

Yeah, the idea is that a restyled sync prefefences page (signed in or out) is a better starting point than about:accounts.

e.g. https://bug1149721.bmoattachments.org/attachment.cgi?id=8636338

Entry points include:

OS Menu Bar: Tools: Set Up Sync
Firefox Toolbar Menu:  Sign in to Sync
Marketing pages on the web

The restyled sync page is more inviting and helps the user understand where sync settings will live.

Comment 6

[Chris Karlof [:ckarlof]]

I wrote the last comment in a rush. To clarify, if you sign up for sign and don't verify, and open the hamburger menu it says "Sign in to Sync". This is already not ideal because it should probably say something like "Verify your account". If you click on "Sign in to Sync", then it takes you to about:accounts, and it should probably take you to the Sync preferences page directly.

Comment 7

[Chris Karlof [:ckarlof]]

Ryan can you advise about my observation about the hamburger menu in my last comment around when you sign up and don't verify?

Comment 8

[Edouard Oger [:eoger]]

Attachment: bug-1152385.patch

Mark, what do you think of this patch?
We lose the |entrypoint| metrics queryParam though: do we want to pass it to about:preferences#sync? (I think will look ugly, but we can).

Comment 9

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8643276 [details] [diff] [review]
bug-1152385.patch

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

The changes look OK but the lack of metrics worries me - I'll needinfo Chris.

::: browser/base/content/browser-fxaccounts.js
@@ +431,5 @@
>        // than requesting migration start immediately.
>        this.openPreferences();
>        break;
>      default:
> +      this.openPreferences();

I think you will need to change the new browser_fxaccounts test (assuming that patch sticks :)

Comment 10

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Chris, this patch would lose alot of metrics for how the user got to about:accounts (eg, via the UITour, or via the hamburger menu etc). I've forgotten who actually consumes that, but I think we should check how much of a problem that is and if we need to mitigate this some other way. Chris, do you know who that is, and if so, can you please needinfo them here?

Comment 11

[Chris Karlof [:ckarlof]]

Yeah, we definitely want to keep that "entrypoint" information in the metrics. The FxA content server consumes this information and combines it with other metrics to help evaluate the effectiveness of different entrypoints.

Comment 12

[Edouard Oger [:eoger]]

> [11:54:11]  <rfeeley>	the build is great
> [11:54:16]  <rfeeley>	works as designed!

It was a build using patches from both bug 1189842 and bug 1152385.

What do we do about the metrics?

Comment 13

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #12)
> > [11:54:11]  <rfeeley>	the build is great
> > [11:54:16]  <rfeeley>	works as designed!
> 
> It was a build using patches from both bug 1189842 and bug 1152385.
> 
> What do we do about the metrics?

The best idea I have is to dispatch a custom event at the newly opened (or switched to) pref pane and store that in memory. What is likely to be tricky is that the prefs pane may not be loaded yet (and thus will not have the event listener yet) meaning a messy check of .readyState and an extra load listener or something.

Another option may be to change openPreferences() so the extraArgs param could have an onOpened or similar callback that is called with the browser element - this would be called in the observer notification for the "new tab" case, or immediately on the "existing tab" case.

Jaws: any thoughts here? The problem we are trying to solve is to pass some data to the "Sync" prefs pane whenever our Sync code calls openPreferences("paneSync"); - the data we want to pass is for metrics when the user clicks a button in Sync prefs to take then to about:accounts. We're happy to accept that this data may be stale or not exist in edge-cases (eg, after session restore)

Comment 14

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Jaws, any chance you can get to this soonish?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mark Hammond [:markh] from comment #13)
> Jaws: any thoughts here? The problem we are trying to solve is to pass some
> data to the "Sync" prefs pane whenever our Sync code calls
> openPreferences("paneSync"); - the data we want to pass is for metrics when
> the user clicks a button in Sync prefs to take then to about:accounts. We're
> happy to accept that this data may be stale or not exist in edge-cases (eg,
> after session restore)

Sorry for the slow response. We should just add the entrypoint to the URL like we were doing for about:accounts. I don't think it is particularly ugly and it matches the direction we are taking with in-content UI and being able to take advantage of how in-content/web-pages work better for things like this. The entrypoint query string will stick around when changing categories within the in-content preferences, but it will cause no harm.

Comment 16

[Edouard Oger [:eoger]]

Attachment: bug-1152385.patch

Thanks jaws!

Comment 17

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8651897 [details] [diff] [review]
bug-1152385.patch

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

> New Binary File: python/psutil/psutil/_psutil_osx.so
> New Binary File: python/psutil/psutil/_psutil_posix.so

I'm pretty sure you don't want them!

so r=me with those removed and the test changed to check the url.

::: browser/base/content/browser-syncui.js
@@ +300,5 @@
>        window.openDialog("chrome://browser/content/sync/addDevice.xul",
>                          "syncAddDevice", "centerscreen,chrome,resizable=no");
>    },
>  
> +  openPrefs: function SUI_openPrefs(entryPoint) {

kill the explicit function name here while we are at it.

::: browser/base/content/test/general/browser_fxaccounts.js
@@ +88,2 @@
>    panelUIStatus.click();
> +  yield promisePreferencesOpened;

I think we should check what the args are here - so the code sending that test:... message should include those url params and this code should check it. It's probably OK to just check the full URL (ie, no need to parse the query string)

Comment 18

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #17)
> ::: browser/base/content/test/general/browser_fxaccounts.js
> @@ +88,2 @@
> >    panelUIStatus.click();
> > +  yield promisePreferencesOpened;
> 
> I think we should check what the args are here - so the code sending that
> test:... message should include those url params and this code should check
> it. It's probably OK to just check the full URL (ie, no need to parse the
> query string)

Actually, I don't think that comment makes sense, so ignore it.

Comment 19

[Edouard Oger [:eoger]]

Attachment: bug-1152385.patch

Thanks Mark!

Comment 20

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2e39d6e97a2

Comment 21

[Edouard Oger [:eoger]]

Try is ok.

Comment 22

[Edouard Oger [:eoger]]

Comment on attachment 8652123 [details] [diff] [review]
bug-1152385.patch

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

Messed up the r+ carry on!

Comment 23

[Pulsebot]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9cef1841ccf