Confirm your account pref screen should no longer redirect to Sync prefs

RESOLVED FIXED in Firefox 58

Status

()

P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: rfeeley, Assigned: lina)

Tracking

50 Branch
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [fxa])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8858805 [details]
1080p.mov

Now that the Firefox Accounts team is providing a "Connect Another Device" experience on the web, instead of redirecting just-verified users to Sync prefs, we should instead give the web control.

Current experience shown.

Comment 1

2 years ago
Could you provide some ideas of what a revised flow might look like?
Flags: needinfo?(rfeeley)
I'm fairly sure that this patch just wants us to remove http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/browser/base/content/aboutaccounts/aboutaccounts.js#213 - some tests are also likely to be affected.
Flags: needinfo?(rfeeley)
(Assignee)

Comment 3

2 years ago
From triage: for Desktop, we should do this soon. For Android, Alex isn't sure if FxA supports this (driving users who sign up on mobile to download Firefox Desktop) yet.
Priority: -- → P2
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-8) from comment #3)
> From triage: for Desktop, we should do this soon. For Android, Alex isn't
> sure if FxA supports this (driving users who sign up on mobile to download
> Firefox Desktop) yet.

We do not, signups have always been so lopsided in favor of desktop, that we
never put in the effort to go the other way.
(Assignee)

Comment 5

2 years ago
(In reply to Shane Tomlinson [:stomlinson] from comment #4)
> We do not, signups have always been so lopsided in favor of desktop, that we
> never put in the effort to go the other way.

Makes sense. Thanks, Shane!
Assignee: nobody → kit
(In reply to Mark Hammond [:markh] from comment #2)
> I'm fairly sure that this patch just wants us to remove
> http://searchfox.org/mozilla-central/rev/
> 4bd7a206dea5382c97a8a0c30beef668cc449f5b/browser/base/content/aboutaccounts/
> aboutaccounts.js#213 - some tests are also likely to be affected.

Most openPrefs call in that file are suspect.
Hey, do you have any update on this or should the FxA team try to fix this? (We found that you are assigned to this bug)
Flags: needinfo?(kit)
(Assignee)

Comment 8

2 years ago
Hi Vlad! I started looking at this, and I'm wondering if we'll need to change the content server to send an `fxaccounts:sync_preferences` WebChannel command after verification or sign-in confirmation. Simply removing the `openPrefs` calls in comment 6 causes us to get stuck at the "Email Sent" screen (or "Confirm your account" for new accounts), even though the "Sync Enabled" notification is shown and the browser starts syncing.
Flags: needinfo?(kit) → needinfo?(vlad)
> Simply removing the `openPrefs` calls in comment 6 causes us to get stuck at the "Email Sent"
> screen (or "Confirm your account" for new accounts)

Right, I believe we'll also need a content-server patch to transition to showing something useful here.  Right now we don't transition the screen because we expect Firefox to navigate away from it entirely.  I believe the intention is to instead show the connect-another-device screen.  Shane, can we safely make such a change without causing backwards-compatibility problems?
Flags: needinfo?(vlad) → needinfo?(stomlinson)
(In reply to Ryan Kelly [:rfkelly] from comment #9)
> > Simply removing the `openPrefs` calls in comment 6 causes us to get stuck at the "Email Sent"
> > screen (or "Confirm your account" for new accounts)
> 
> Right, I believe we'll also need a content-server patch to transition to
> showing something useful here.  Right now we don't transition the screen
> because we expect Firefox to navigate away from it entirely.  I believe the
> intention is to instead show the connect-another-device screen.  Shane, can
> we safely make such a change without causing backwards-compatibility
> problems?

This is already taken care of: [1]. We did this as part of the "Sync suggestion".

[1] - https://github.com/mozilla/fxa-content-server/blob/ce4b91116a0cb80f96ca8dd7c9e41c39e07a2afa/app/scripts/models/auth_brokers/fx-desktop-v2.js#L66:L76
Flags: needinfo?(stomlinson)
(In reply to Shane Tomlinson [:stomlinson] from comment #10)
> (In reply to Ryan Kelly [:rfkelly] from comment #9)
> > > Simply removing the `openPrefs` calls in comment 6 causes us to get stuck at the "Email Sent"
> > > screen (or "Confirm your account" for new accounts)
> > 
> > Right, I believe we'll also need a content-server patch to transition to
> > showing something useful here.  Right now we don't transition the screen
> > because we expect Firefox to navigate away from it entirely.  I believe the
> > intention is to instead show the connect-another-device screen.  Shane, can
> > we safely make such a change without causing backwards-compatibility
> > problems?
> 
> This is already taken care of: [1]. We did this as part of the "Sync
> suggestion".
> 
> [1] -
> https://github.com/mozilla/fxa-content-server/blob/
> ce4b91116a0cb80f96ca8dd7c9e41c39e07a2afa/app/scripts/models/auth_brokers/fx-
> desktop-v2.js#L66:L76

Sorry, more context here after I reloaded it myself - we added the referenced code above 
~ 10 months ago to help users who browse directly to "https://accounts.firefox.com" 
(without ?service=sync... query params). In Firefox Desktop, we present a little 
blue tooltip that says "Looking for Sync? Sign in here". If the user clicks on the 
"Sign in here" link, the page redirects to 
https://accounts.firefox.com/signup?service=sync&context=fx_desktop_v3. Because this page
isn't a child of about:accounts and we can't depend upon the browser to transition once
it detects the account is verified, we have to poll and progress ourselves. Whenever
the user verifies their email, we progress from /confirm to /signup_confirmed (or signin).

The mention of "getting stuck" I'd like to hear more about, it sounds like there are 
some cases we haven't considered.
Flags: needinfo?(kit)
(Assignee)

Comment 12

2 years ago
Ah, I see. I didn't go through https://accounts.firefox.com; instead, I opened about:preferences#sync, and clicked "Create Account" or "Sign In". The URL is `about:accounts?action={signup, signin}&entrypoint=preferences`. In both cases, I got stuck at the confirmation screen if I removed the `openPrefs` calls from `aboutaccounts.js`.
Flags: needinfo?(kit)
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #12)
> Ah, I see. I didn't go through https://accounts.firefox.com; instead, I
> opened about:preferences#sync, and clicked "Create Account" or "Sign In".
> The URL is `about:accounts?action={signup, signin}&entrypoint=preferences`.
> In both cases, I got stuck at the confirmation screen if I removed the
> `openPrefs` calls from `aboutaccounts.js`.

Oh... I misunderstood! I thought my dream of removing about:accounts was 
about to become a reality!

Since we are still within about:accounts, we could return a capability in 
the fxaccounts:fxa_status response [1] that indicates "The browser won't 
transition to about:preferences#sync afterwards, you take care of this".

I'm not sure if this capability would be sent all the time or only
if FxA is opened within about:accounts. I'm also stumped for what 
this would be named, naming abstract concepts is hard.

[1] - https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsWebChannel.jsm#405
From IRC

> re bug 1357085 - until we do kill about:accounts, can't about:accounts itself just transition to a better a.f.c page instead of a more complex capabilities solution?
> (ie, hard-code a new url instead of about:prefs)
...
> I'm probably missing something subtle, but Kit writes "In both cases, I got stuck at the confirmation screen if I removed the `openPrefs` calls from `aboutaccounts.js`." - and openPrefs is literally |window.location = "about:preferences#sync"| - so I'm thinking |window.location = "https://accounts.firefox.com/something-better"|


I'm slightly uncomfortable with that approach because it reduces the flexibility we have on the FxA side to determine the next steps, though I'm mulling over whether we can retain the flexibility. For example, we recently made changes to the signup flow in the firstrun page to encourage users to connect a second device - the UI that is displayed to the user depends in part on the user's country. Users in the US, Canada, and GB can send themselves an SMS, users in other parts of the world cannot. This decision making currently occurs in the /confirm page when verification polling has indicated the user verified their account.

If the user can send themselves an SMS, we send them to /sms, if the user is ineligible to send an SMS, we try to send them to /connect_another_device, and if they aren't eligible for /connect_another_device either, we send them to /signup_confirmed. It's a nasty decision tree. We could move that decision tree logic elsewhere, onto an endpoint you could point at, though I suspect we'd need an endpoint for each of signin, signup, and password reset. This seems to just shift the complexity. I suggested the "capability" approach because we already have a "cadAfterSignUpConfirmationPoll" capability internally [1], I was hoping to hook into that because it's pretty dead simple on our end.

[1] - https://github.com/mozilla/fxa-content-server/blob/d22f4cde9c8c9b54938e8f956b00ded1caf77a24/app/scripts/models/auth_brokers/fx-firstrun-v1.js#L30
(Assignee)

Comment 15

2 years ago
I think the mailing list discussion is crystallizing into a handshake to indicate if the content server should handle this. Does that sound good to y'all? Should we open a separate content server issue?
Flags: needinfo?(stomlinson)
Flags: needinfo?(markh)
IIUC, Shane is advocating for a new "capability" in the existing "fxaccounts:fxa_status" message, but in general, I'm fine with whatever he decides is the best way forward.
Flags: needinfo?(markh)
(In reply to Mark Hammond [:markh] from comment #16)
> IIUC, Shane is advocating for a new "capability" in the existing
> "fxaccounts:fxa_status" message, but in general, I'm fine with whatever he
> decides is the best way forward.

Some other alternatives:

* Bump the fx_desktop_vN number to fx_desktop_v4, though I'm not sure that offers
  any real advantages to a capability.
* UA sniff and set a capability on the front end - perfectly acceptable but spreads 
  around knowledge and requires deployment coordination between the teams.
* Append a query parameter when opening signin, signup, force_auth, or settings, 
  something that indicates "FxA should watch for email address verifications as well." 
  A fairly simple approach that we have used in the past.
* Have the browser notify the FxA frontend that an email verification has occurred. 
  The FxA frontend would not polling but would transition upon receipt of the message. 
  This would reduce polling pressure on the servers, but the amount of polling from 
  about:accounts is relatively low compared to the firstrun page.
* Ditch about:accounts and open FxA in its own tab.

I have to admit that I'm partial to ditching about:accounts. Less browser code. 
localStorage isolation in e10s evaporates. No work needed on the content server.

That said, I understand there is some hesitation to remove about:accounts, esp
w.r.t. providing a "You are offline" error message. I've never been sold on that
page's utility, though I understand that since FxA is being opened from within 
about:preferences, there is a case that users should never have a bad experience.

None of the other options really appeal all that much. UA sniffing is the simplest 
alternative, one we rely on a lot, but it's fragile to UA munging addons and 
capability knowledge is spread to FxA when that knowledge is best maintained at 
the source of truth - the browser itself.
Flags: needinfo?(stomlinson)
(In reply to Shane Tomlinson [:stomlinson] from comment #17)
> (In reply to Mark Hammond [:markh] from comment #16)
> > IIUC, Shane is advocating for a new "capability" in the existing
> > "fxaccounts:fxa_status" message, but in general, I'm fine with whatever he
> > decides is the best way forward.
> 
> Some other alternatives:

Thanks Shane.

> I have to admit that I'm partial to ditching about:accounts.

I'm totally fine with that. I think we just need rfeeley to sign off on that plan first (and specifically the error handling you mention) - when he and I chatted in SF about this, he seemed nervous about losing that error capability.

(FWIW, there's also a different error page shown when the FxA-related preferences are wrong (eg, not https etc), but I'd guess it's much easier to argue that is OK to kill.

> No work needed on the content server.

For my information, I'd have thought that *some* effort on the content server would be needed as it would then need to transition where it doesn't now due to the "flashing" mentioned above? Or maybe the content server already behaves differently when it knows it is in about:accounts?

> None of the other options really appeal all that much. UA sniffing is the
> simplest 
> alternative, one we rely on a lot, but it's fragile to UA munging addons and 
> capability knowledge is spread to FxA when that knowledge is best maintained
> at 
> the source of truth - the browser itself.

It's not clear from the above how exactly Kit should move this forward. I'd be all for closing this bug as WONTFIX and working on "remove about:accounts" if we can get rfeeley on board, but this bug has already been open 4 months and I'd put money on the fact really really like *something* in 57, even if it is short term :)
Flags: needinfo?(stomlinson)
(In reply to Mark Hammond [:markh] from comment #18)
> 
> For my information, I'd have thought that *some* effort on the content
> server would be needed as it would then need to transition where it doesn't
> now due to the "flashing" mentioned above? Or maybe the content server
> already behaves differently when it knows it is in about:accounts?

You are correct that we have two different behaviors depending on whether
FxA is embedded in about:accounts or not [1]. We did this a few months ago
to support an improved "Sync suggestion" tooltip [2][3]. What this means
is that from our point of view, we are already ready for about:accounts 
to be removed, and in theory, this requires no changes on our side.

> It's not clear from the above how exactly Kit should move this forward. I'd
> be all for closing this bug as WONTFIX and working on "remove
> about:accounts" if we can get rfeeley on board, but this bug has already
> been open 4 months and I'd put money on the fact really really like
> *something* in 57, even if it is short term :)

I suspect that continuing with Kits already started work instead of removing
about:accounts will be simpler/less risky path for Fx 57. 

How about a two phase approach:

1. Fix the immediate problem by completing Kit's work.
2. Convince Ryan that removing about:accounts is a worthwhile idea, and once
we have him on board, rip it all out.

If that seems reasonable, to complete Kit's work, let's go with the 
simplest path - UA sniff on the content server. We'll check for Fx >= 57, 
and if so, do our own screen transitions when embedded in about:accounts. 
I still think a capability provides a clean coordination path, but I just 
spent an hour trying to think of capability names I'm not embarrassed by. 
I failed.

As far as coordination, I reckon the FxA portion should land first or 
else FxA will transition to the "confirm your email" screen and nobody 
will update the UI upon email verification. There may be a window after
we deploy and before Kit's patch lands where Fx 57 users see a quick
flash of "Your account is confirmed", but this seems acceptable
if we know Kit's patch will land before 57 is released.

Kit and Mark, does this approach work for you?


[1] - https://github.com/mozilla/fxa-content-server/blob/ce4b91116a0cb80f96ca8dd7c9e41c39e07a2afa/app/scripts/models/auth_brokers/fx-desktop-v2.js#L66:L76
[2] - https://github.com/mozilla/fxa-content-server/issues/4605
[3] - https://github.com/mozilla/fxa-content-server/pull/4798
Flags: needinfo?(stomlinson)
Flags: needinfo?(markh)
Flags: needinfo?(kit)
(In reply to Shane Tomlinson [:stomlinson] from comment #19)
> Kit and Mark, does this approach work for you?

Sold! Thanks Shane, I opened bug 1395460 to remove about:accounts.
Flags: needinfo?(markh)
(Assignee)

Comment 21

2 years ago
This sounds like a good plan to me. I'll plan on removing all automatic `openPrefs` calls in that file, and waiting until you have the UA sniffing in place before landing.
Flags: needinfo?(kit)
Comment hidden (mozreview-request)
Kit, unfortunately I have a million things going on and won't get to this until train-96 instead of train-95 (which was cut today). Unless we issue a point release, our side will be in prod in ~ 3 weeks. Will a 3 week delay be problematic?
Flags: needinfo?(kit)
(Assignee)

Comment 25

2 years ago
(In reply to Shane Tomlinson [:stomlinson] from comment #24)
> Kit, unfortunately I have a million things going on and won't get to this
> until train-96 instead of train-95 (which was cut today). Unless we issue a
> point release, our side will be in prod in ~ 3 weeks. Will a 3 week delay be
> problematic?

Not at all! I don't think there's any rush from the Desktop side, and this patch is small enough to uplift to Beta if we don't make it before the merge.
Flags: needinfo?(kit)

Comment 26

2 years ago
mozreview-review
Comment on attachment 8905115 [details]
Bug 1357085 - Don't automatically redirect to Sync prefs after confirming an FxA sign-in or sign-up.

https://reviewboard.mozilla.org/r/176918/#review182176
Attachment #8905115 - Flags: review?(markh) → review+
Whiteboard: [fxa]
We just landed our portion into master, this should be released to prod on ~ 25th of Sept.
:kitcambridge - shippit! Our portion has landed in prod and has stuck.
Flags: needinfo?(kit)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2d9f733b1c71 -d a2a7b3161ac3: rebasing 423655:2d9f733b1c71 "Bug 1357085 - Don't automatically redirect to Sync prefs after confirming an FxA sign-in or sign-up. r=markh" (tip)
merging browser/base/content/aboutaccounts/aboutaccounts.js
warning: conflicts while merging browser/base/content/aboutaccounts/aboutaccounts.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)

Comment 31

a year ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0e17fa9eca1
Don't automatically redirect to Sync prefs after confirming an FxA sign-in or sign-up. r=markh
https://hg.mozilla.org/mozilla-central/rev/b0e17fa9eca1
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Updated

a year ago
Flags: needinfo?(kit)
:kitcambridge, :markh - will a request be made to uplift this patch to 57? 
If not, I'll update the content server portion to change behavior starting with 58.
Flags: needinfo?(markh)
Flags: needinfo?(kit)
>  will a request be made to uplift this patch to 57?

Word is that the bar for requesting uplift to 57 is now "if we discovered this after release, we'd roll a point release of 57".  I think this falls squarely squarely below that bar and the right thing to do is wait for 58.
(In reply to Ryan Kelly [:rfkelly] from comment #34)
> >  will a request be made to uplift this patch to 57?
> 
> Word is that the bar for requesting uplift to 57 is now "if we discovered
> this after release, we'd roll a point release of 57".  I think this falls
> squarely squarely below that bar and the right thing to do is wait for 58.

Agreed.
Flags: needinfo?(markh)
Flags: needinfo?(kit)
Thanks for the update, I kinda feared this. This is where a capability instead of UA sniffing would have shone.
(In reply to Shane Tomlinson [:stomlinson] from comment #36)
> Thanks for the update, I kinda feared this. This is where a capability
> instead of UA sniffing would have shone.

Since we have to wait for 58 for this already, could we add a capability 
so we can remove the UA sniffing on the FxA side? I could even write the
patch if folks agree.
Flags: needinfo?(markh)
Flags: needinfo?(kit)
I don't think we'd roll out another point release just for this patch, but it's arguably an improvement to our onboarding flow, especially if the goal is to increase multi-device users. We can't experiment with driving new Desktop users to mobile, because Desktop straight-up redirects to preferences.

If we want to make it easier for people to get the mobile apps and set up Sync (text a download link, scan a QR code to sign in without retyping your password, and so on), we'd need to wait until 58. That's 6 weeks we could have spent improving the experience for new users. We could make the case that this patch warrants riding along with a higher-priority patch, that would be cause for a point release.

I know that's a stretch, though, so a capability is probably a safer bet. Shane, would this be as simple as adding another field to https://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/services/fxaccounts/FxAccountsWebChannel.jsm#405-407, and changing the content server in https://github.com/mozilla/fxa-content-server/issues/5556?
Flags: needinfo?(stomlinson)
Flags: needinfo?(markh)
Flags: needinfo?(kit)
(In reply to Shane Tomlinson [:stomlinson] from comment #36)
> Thanks for the update, I kinda feared this. This is where a capability
> instead of UA sniffing would have shone.

For my info, how does the fact it is in 58 instead of 57 change anything around this?
There's an `if (firefox_version >= 57)` in fxa-content-server that will have to change to an `if (firefox_version >= 58)`.
(In reply to Ryan Kelly [:rfkelly] from comment #40)
> There's an `if (firefox_version >= 57)` in fxa-content-server that will have
> to change to an `if (firefox_version >= 58)`.

(In reply to Mark Hammond [:markh] from comment #39)
> (In reply to Shane Tomlinson [:stomlinson] from comment #36)
> > Thanks for the update, I kinda feared this. This is where a capability
> > instead of UA sniffing would have shone.
> 
> For my info, how does the fact it is in 58 instead of 57 change anything
> around this?

Yeah, it's mostly in the tests. The actual UA sniff is pretty easy, the difficulty
comes because I have a lot of tests that test for one behavior <= Fx 56, and
another behavior >= 57. Those will need to change to be <= Fx 57 and >= 58.
With a capability approach, the tests would be very similar, but instead of
gating on Fx version, the tests would gate on "does not have capability" and
"has capability". That way the feature/tests are independent of browser 
version #. In this case, it's not critical make the change to capabilities, 
my thinking is that since we have to make changes to the tests already, 
we might as well use capabilities because that's what we'll most likely
use in the future.
Flags: needinfo?(stomlinson)
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #38)

> I know that's a stretch, though, so a capability is probably a safer bet.
> Shane, would this be as simple as adding another field to
> https://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/services/fxaccounts/
> FxAccountsWebChannel.jsm#405-407, and changing the content server in
> https://github.com/mozilla/fxa-content-server/issues/5556?

That's exactly it. In the content-server `browserTransitionsAfterEmailVerification: true` is set by default, 
and `browserTransitionsAfterEmailVerification: false` is set for Fx 57. It's not the best name, but workable.
You need to log in before you can comment on or make changes to this bug.