Have the "activate your FxAccount" tab switch to about:preferences#sync if opened in the browser configured with the account

VERIFIED FIXED in Firefox 46

Status

()

Firefox
Sync
P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: edwong, Assigned: stomlinson)

Tracking

Trunk
Firefox 46
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox43 affected, firefox46 fixed)

Details

(Whiteboard: fxsync, URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
6b. If opened in same browser, new tab becomes Sync prefs, Confirm Your Account becomes Account Verified
(Reporter)

Comment 1

3 years ago
related to bug 1199303. Needs a content server bug to send webChan message to enable 6b.
Iteration: --- → 43.3 - Sep 21
Whiteboard: fxsync
ni? :markh to link me to the content-server if we have one, or provide a description of what's needed if we don't; I'm not confident I remember all the details from when we discussed this in IRC.
Flags: needinfo?(markh)
(In reply to Ryan Kelly [:rfkelly] from comment #2)
> ni? :markh to link me to the content-server if we have one, or provide a
> description of what's needed if we don't; I'm not confident I remember all
> the details from when we discussed this in IRC.

We don't have a content-server issue yet. In a nutshell, we want the landing page for "please verify your mail" to send us a message as it transitions to the "much success" page. The browser would be listening for this message and switch the tab to about:preferences. Note also that this message probably shouldn't be a generic "user is/became verified" message as the semantics of that might be unclear - I guess it is closer to "user just became verified via this exact page".
Flags: needinfo?(markh)
Flags: firefox-backlog+
This isn't happening in 43.3. This is perhaps a candidate for 44.1.
Iteration: 43.3 - Sep 21 → ---
Priority: -- → P1

Updated

3 years ago
Summary: smarter logic about verification link handling → Have the "activate your FxAccount" tab switch to about:preferences#sync if opened in the browser configured with the account
See Also: → bug 1206086
(Assignee)

Updated

3 years ago
Assignee: nobody → stomlinson
Comment on attachment 8700596 [details] [diff] [review]
bug-1204714-open-sync-preferences.patch

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

I believe this will open preferences in every open window. I think we should get the "browser" which sent the message and have only that window open prefs. I think this means an ugly reach into the "sendingContext", but we can probably get away with that. Testing will be a bit of a challenge, but it might make sense to create a new browser test purely for webchannels (ie, along the lines of browser_aboutaccounts, but focusing on browser-side channel handling)
Attachment #8700596 - Flags: feedback?(markh) → feedback-
(Assignee)

Comment 8

3 years ago
Created attachment 8704630 [details] [diff] [review]
bug-1204714-open-sync-preferences.patch
Attachment #8700596 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Thanks for the feedback Mark. I have been digging in today and found another approach that may be preferable, and at first blush appears to "work" though I don't have enough system context to know for sure.

It starts with the assumption that the currently displayed tab sent the WebChannel message. This assumption may be ill founded, but it vastly simplifies things. In utilityOverlay.js, there is a function called "openUILinkIn" [1] that takes a "where" configuration option that allows a URI to be opened in one of several locations, "current" means "replace the current tab". If openPreferences [2] is updated so its extraArgs parameter also accepts a "where" option, if "where" exists, I can call openUILinkIn with "where", if "where" does not exist, use the existing behavior. Workable? I've updated the patch with this approach.

[1] - https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/base/content/utilityOverlay.js#177
[2] - https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/base/content/utilityOverlay.js#526
Flags: needinfo?(markh)
Comment on attachment 8704630 [details] [diff] [review]
bug-1204714-open-sync-preferences.patch

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

ISTM that this will still make every open window switch to preferences - browser-syncui is loaded into each window and the notification will be seen by each of them.  I expect that something like:

sendingContext.browser.loadURI(...) is all you need.

::: browser/base/content/utilityOverlay.js
@@ +736,5 @@
>    openUILinkIn(url, where);
>  }
>  
>  function openPrefsHelp() {
> +  // non-instant apply prefwindows are usually modal, so we can't open in the topmost window,

let's not touch unrelated lines.

Updated

3 years ago
Flags: needinfo?(markh)
(Assignee)

Comment 12

3 years ago
Created attachment 8705624 [details] [diff] [review]
Open sync preferences in the current tab of the WebChannel message's sendingContext

Mark, is this more along the lines of what you were thinking? The only thing that worries me about this approach is if preferences are loaded via a different mechanism in the future. The openPreferences function in utilityOverlay.js provides an abstraction to open preferences that should guard against future changes, it just doesn't provide the functionality (replace the current tab) we need.

The other thing this patch does not yet do is provide a mechanism for notifying the content server this feature is enabled. We could either bump the context from fx_desktop_v2 to fx_desktop_v3, or we could add a query parameter. I really feel like we need the capabilities mechanism we keep bringing up!

Treeherder run https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a6057568cb7
Attachment #8704630 - Attachment is obsolete: true
Attachment #8705624 - Flags: review?(markh)
Comment on attachment 8705624 [details] [diff] [review]
Open sync preferences in the current tab of the WebChannel message's sendingContext

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

I think this is fine - about:preferences can be found here via dxr if a future means we need to revisit this.

Re letting the server know - I'm fine with either a _v3 or a query param - but it's not clear to me why the server cares - at the moment that tab just stays on the verification page, and this will continue if the client doesn't understand the message - but I'm probably missing something obvious as I haven't thought much about it.
Attachment #8705624 - Flags: review?(markh) → review+
(Assignee)

Comment 14

3 years ago
(In reply to Mark Hammond [:markh] from comment #13)
> Comment on attachment 8705624 [details] [diff] [review]
> Open sync preferences in the current tab of the WebChannel message's
> sendingContext
> 
> Review of attachment 8705624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Re letting the server know - I'm fine with either a _v3 or a query param -
> but it's not clear to me why the server cares - at the moment that tab just
> stays on the verification page, and this will continue if the client doesn't
> understand the message - but I'm probably missing something obvious as I
> haven't thought much about it.


Ah, the big thing is we (plan to) show the user the "Your account has been verified!" screen with a big button that says "Sync preferences". We don't want to show the big blue button unless the browser can handle the message, correctly handling legacy browsers. Once the user clicks the button, preferences are loaded. This is the same scheme currently used by Fennec; the user is informed their account has been verified, plus they have an opportunity to continue to preferences if they wish.
(Assignee)

Comment 15

3 years ago
Created attachment 8706583 [details] [diff] [review]
Open sync preferences in the current tab of the WebChannel message's sendingContext, add context=fx_desktop_v3

This most recent patch changes the context from fx_desktop_v2 to fx_desktop_v3. This patch should land after the content server patch makes its way to prod or else Nightly users will be unable to sign in/up to Sync.

[1] https://github.com/mozilla/fxa-content-server/pull/3341
Attachment #8705624 - Attachment is obsolete: true
Attachment #8706583 - Flags: review?(markh)

Updated

3 years ago
Attachment #8706583 - Flags: review?(markh) → review+
(Assignee)

Updated

2 years ago
Attachment #8706583 - Flags: checkin?
(Assignee)

Updated

2 years ago
Attachment #8706583 - Flags: checkin?
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1573412a4ad
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 19

2 years ago
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful.

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
When already logged into Firefox account, new tab open with about:preferences#sync account becomes
logged into account.

Actual Results: 
As expected

Updated

2 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.