1562006 - [skyline] Update FxA toolbar menu for Skyline

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, enhancement, P1)

Description

[Vijay Budhram]

Hello all,

This is a tracking bug for the upcoming Skyline work on the FxA toolbar menu. We are currently still flushing out the final UX flows and feature document but will attach links when complete.

At a high level we plan on updating the toolbar menu to do the following

• Show a list of approved services that a user can log into with their Firefox Account
  • The list does not change depending on login state, ie it is static
• Update the menu to be less sync specific but still provide access to sync settings
• Add telemetry to new menu buttons
• Update entrypoint value for new menu

Comment 1

[Vijay Budhram]

The UX team has approved and finalized the UX that will be shipped in Firefox 70. This version does not have Sync and Firefox Accounts decoupled.

https://mozilla.invisionapp.com/share/V6ST9IFJMXP#/screens/372045396_Skyline_FxA_Toolbar

Comment 2

[Vijay Budhram]

Attachment: Bug 1562006 - Update FxA toolbar menu for skyline

Comment 3

[Ryan Kelly [:rfkelly]]

Gijs, from a conversation at Whistler, I recall you saying there were some aspects of the current Accounts Toolbar Menu implementation that you would have liked to see done differently if we had the time. This might be an opportunity to address some aspects incrementally, if you're happy to provide some guidance. Anything specific you'd like to see us take into account here?

Comment 4

[Ryan Kelly [:rfkelly]]

I have some questions about the details of linking out to Monitor and Send, particularly in the case where the button says "Continue as ...".

I'm assuming that these buttons will open monitor.firefox.com or send.firefox.com in a new tab, but the mocks don't specify what should happen in that new tab. We need to consider a little state matrix where the browser is either signed in or not, and the website is either not signed in, signed in as the same user, or signed in as a different user. What is the desired experience in each of these cases?

My guess at the optimal desired experience is:

	Browser not signed in	Browser signed in as UserA
Website not signed in	Opens FxA signin page for the website	Opens website with UserA automagically signed in and ready to go
Website signed in as UserA	Opens website in existing state, signed in as UserA	Opens website in existing state, signed in as UserA
Website signed in as UserB	Opens website in existing state, signed in as UserA	Opens website with UserB automagically signed out, UserA automagically signed in and ready to go

It's not trivial to build that experience based on what we have available today, but if that's where we want to be then we can figure out how to work towards it. It will probably require us to spin off a couple of dependent bugs for making changes in Monitor and Send.

A slightly jankier experience, but which could be a lot quicker to ship, would be the following:

	Browser not signed in	Browser signed in as UserA
Website not signed in	Opens FxA signin page for the website	Opens FxA signin page for the website, prefilled for one-click signin as UserA
Website signed in as UserA	Opens website in existing state, signed in as UserA	Opens website in existing state, signed in as UserA
Website signed in as UserB	Opens website in existing state, signed in as UserA	Opens FxA signin page for the website, prefilled for one-click signin as UserA

Ryan, thoughts? Would you be happy for us to do the second first as an MVP, then the first "automagical" version as a followup?

Comment 5

[Ryan Kelly [:rfkelly]]

In either case, we'll want to consider exposing a dedicated URI on both Monitor and Send that these toolbar items can link out to, so + :fzzzy and :groovecoder for early context.

Comment 6

[Ryan Kelly [:rfkelly]]

we'll want to consider exposing a dedicated URI on both Monitor and Send that these toolbar items can link out to

This might also overlap with what we need to do to support activating Monitor from the protection report, so linking for context:

https://docs.google.com/document/d/15hgk0UJlLdpWv3iJQyqew2gZ_OfiFZ52dYo6HtW-gkE/edit?ts=5d251317#bookmark=id.t1c7e0p3ceuf

Comment 7

[:Gijs (he/him)]

(In reply to Ryan Kelly [:rfkelly] from comment #3)

Gijs, from a conversation at Whistler, I recall you saying there were some aspects of the current Accounts Toolbar Menu implementation that you would have liked to see done differently if we had the time. This might be an opportunity to address some aspects incrementally, if you're happy to provide some guidance. Anything specific you'd like to see us take into account here?

No, I think the current state in-tree is fine, I think my comments in Whistler were more about some of the process of how we got there. :-)

Comment 8

[Ryan Feeley [:rfeeley]]

A++ on your markdown chops. Does this help clarify the intent? I don't think we should ever take users directly to FxA sign-in. Rather let the site sell the benefit of using the account, which is often optional, rather than getting them to commit before seeing.

	Browser not signed in	Browser signed in as UserA
Website not signed in	Straight visit to domain (e.g. monitor.firefox.com)	Allow relier to choose, but ideally they opt for instant sign-in, or messaging the user welcoming them with a Continue as… button
Website signed in as UserA	Opens website in existing state (signed in as UserA)	Opens website in existing state (signed in as UserA)
Website signed in as UserB	Opens website in existing state (signed in as UserB)	Opens website with UserB automagically signed out, UserA automagically signed in and ready to go

Will this require coordination with Monitor? Does sign-in equate to getting emails ASAP?

Comment 9

[Ryan Kelly [:rfkelly]]

A++ on your markdown

Thanks, I did a lot of googling...

Will this require coordination with Monitor?

I think we should get both Monitor and Send to add a dedicated URL that we can use for this handoff; I'll think a bit more on the details and come back with something more concrete, so keepin the ni? on myself here.

Does sign-in equate to getting emails ASAP?

Sorry, I don't understand this question, please elaborate?

Comment 10

[Ryan Feeley [:rfeeley]]

Sorry, I don't understand this question, please elaborate?

Probably more for Monitor, but it is an email alert service related to breaches. I wonder if it's possible to sign-in to Monitor without immediately subscribing to these emails. They might want to change this behaviour if they are are auto-enrolling people into emails they lack context for.

Comment 11

[Ryan Kelly [:rfkelly]]

What should happen to the "Send" and "Monitor" links for users who are using a non-default FxA server, such as those running the Mozilla China repack, or those hosting their own FxA+Sync setup?

Should we just link out to the websites anyway, and they'll have to create an account with accounts.firefox.com in order to use them? Or would it make more sense to hide the links in this case since we can't offer a smooth "continue as XYZ" experience?

Comment 12

[Ryan Kelly [:rfkelly]]

I'll think a bit more on the details and come back with something more concrete, so keepin the ni? on myself here.

Restoring my ni? since I haven't followed up on this bit yet.

Comment 13

[Ryan Feeley [:rfeeley]]

Should we just link out to the websites anyway, and they'll have to create an account with accounts.firefox.com in order to use them? Or would it make more sense to hide the links in this case since we can't offer a smooth "continue as XYZ" experience?

Great catch. If you can manage to hide them then that would be be ideal.

Comment 14

[Ryan Kelly [:rfkelly]]

Mark, what's the best pref we should check to see whether a non-default account server is in use?

Comment 15

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

(In reply to Ryan Kelly [:rfkelly] from comment #14)

Mark, what's the best pref we should check to see whether a non-default account server is in use?

identity.fxaccounts.remote.root - although prefs code is a little messy, so we should remove the old way of configuring things as this comment suggests. I opened bug 1567376 for that.

Comment 16

[Ryan Kelly [:rfkelly]]

I filed Bug 1567405 and Bug 1567403 to dig into the details of linking out to Monitor and Send.

Comment 17

[Vijay Budhram]

Attachment: FxA toolbar menu data collection review for Skyline

Hey Tim,

Mind doing a data review for this? This patch adds new events to the existing feature which had a data review in https://bugzilla.mozilla.org/show_bug.cgi?id=1542334.

Please let me know if you are unable to do this or have a recommendation on someone who might. Thanks!

Comment 18

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Comment on attachment 9079732 [details]
FxA toolbar menu data collection review for Skyline

Thanks!

1) Is there or will there be **documentation** that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, in Events.yaml.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, the Firefox telemetry opt-out.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?

n/a

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, interaction data.

5) Is the data collection request for default-on or default-off?

Default-on.

6) Does the instrumentation include the addition of **any *new* identifiers**?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No)

Yes, Vijay and Leif are responsible for renewing the collections before they expire in 71 and 75.

9) Does the data collection use a third-party collection tool? **If yes, escalate to legal.**

No.

Comment 19

[Vijay Budhram]

Attachment: Bug 1562006 - Add support for `Continue as` button and update links

Depends on D37961

Comment 20

[Hector Zhao [:hectorz]]

(In reply to Ryan Feeley [:rfeeley] from comment #13)

(In reply to Ryan Kelly [:rfkelly] from comment #11)

What should happen to the "Send" and "Monitor" links for users who are using a non-default FxA server, such as those running the Mozilla China repack, or those hosting their own FxA+Sync setup?

Should we just link out to the websites anyway, and they'll have to create an account with accounts.firefox.com in order to use them? Or would it make more sense to hide the links in this case since we can't offer a smooth "continue as XYZ" experience?

Great catch. If you can manage to hide them then that would be be ideal.

Thanks for taking our situation into consideration!

Comment 21

[Vijay Budhram]

Attachment: Updated FxA toolbar menu data collection review for Skyline

Hey Tim,

I have attached the updated data review for the FxA toolbar menu. The only change from this review and the previous is that now we emit a telemetry event for when the clicks the Login and Passwords button from the menu.

Telemetry event for user clicking “Login and Passwords” in avatar menu (Category 2)
pwmgr.fxamenu

Comment 22

[Thomas Elin [:relaas]]

Removed User Journey references in this bug per Julie McCracken. She stated there are no dependencies on that team.

Comment 23

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Comment on attachment 9081037 [details]
Updated FxA toolbar menu data collection review for Skyline

Thanks for the update! I agree `pwmgr.fxamenu` is cat 2; the response at comment 18 is still complete.

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The patch fails to apply:

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpc5O5KW toolkit/components/telemetry/Events.yaml Hunk #2 FAILED at 542. Hunk #3 FAILED at 566. 2 out of 3 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Events.yaml.rej abort: patch command failed: exited with status 256

Please fix this conflict and also mark the issue raised by Pike as solved.

Comment 25

[Vijay Budhram]

Thanks :aryx, I've resolved the conflicts and fixed the issue that was brought up by Pike (marked it Done in phabricator).

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2b7c868c0a7
Update FxA toolbar menu for skyline r=eoger,Gijs,fluent-reviewers,flod

Comment 27

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6ce560581af
Backed out changeset f2b7c868c0a7 for browser chrome failures. CLOSED TREE

Comment 28

[Alexandru Michis [:malexandru]]

Backed out changeset f2b7c868c0a7 for browser chrome failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d6ce560581af17b5aadc7957d5c0472faa27ccb6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=258861598&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&tochange=d6ce560581af17b5aadc7957d5c0472faa27ccb6&fromchange=f2b7c868c0a7064ce6bd58565e210a67acbf349e&searchStr=browser%2Cchrome

Failures logs:

• TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_duplicateIDs.js | appMenu-logins-button should be unique -
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258861600&repo=autoland&lineNumber=9058
• multiple failures in browser_sync.js:
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258861598&repo=autoland&lineNumber=4563

Comment 29

[Alexandru Michis [:malexandru]]

Also seems to fail in browser_parsable_css.js: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258884926&repo=autoland&lineNumber=1634

[task 2019-07-29T22:27:11.770Z] 22:27:11 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-marker-anon-child’. Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua|pluginproblem)\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true}
[task 2019-07-29T22:27:11.770Z] 22:27:11 INFO - Buffered messages finished
[task 2019-07-29T22:27:11.770Z] 22:27:11 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for chrome://browser/skin/customizableui/panelUI.css: Expected attribute name or namespace but found ‘"disabled"’. Ruleset ignored due to bad selector. -
[task 2019-07-29T22:27:11.770Z] 22:27:11 INFO - Stack trace:
[task 2019-07-29T22:27:11.770Z] 22:27:11 INFO - chrome://mochikit/content/browser-test.js:test_ok:1576
[task 2019-07-29T22:27:11.770Z] 22:27:11 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:294
[task 2019-07-29T22:27:11.770Z] 22:27:11 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:515

Comment 30

[Vijay Budhram]

Hey :malexandru,

I've pushed updates to fix the browser sync and css parsing tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b34a4e44c7588425521b27ad33ba1e9c1eb81457 should hopefully verify they are fixed.

Comment 31

[Cosmin Sabou [:CosminS]]

Vijay, please take a look over these other failures: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b34a4e44c7588425521b27ad33ba1e9c1eb81457&selectedJob=258937474

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258937427&repo=try&lineNumber=5146
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258939689&repo=try&lineNumber=2141

Comment 32

[Vijay Budhram]

Hey :cosmins,

I resolved the duplicate id test failure and pushed another try which looks to have passed successfully.

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

Comment 33

[Vijay Budhram]

I've pushed a couple more changes to fix failing tests. The treeherder [1] build shows everything is good except unexpected flickering? :mconley in https://bugzilla.mozilla.org/show_bug.cgi?id=1561547#c11 you mentioned a similar type issue. Do you believe these are related or maybe this is an intermittent error? It also fails for me when testing from mozilla-central tip.

The test file allows for exceptions, so that might be an option here. Or maybe it is ok to land it? Thoughts?

[1] - https://treeherder.mozilla.org/testview.html?repo=try&revision=7071e4c460eaba76c247d33f8c27baf122a567e8

Comment 34

[Vijay Budhram]

In previous comment, I didn't do a pull to get latest mozilla-central code. I've done that and rebased the patch. I also pushed another try job [1]. Is it possible that I was running into this known issue [2].

[1] - https://treeherder.mozilla.org/#/jobs?repo=try&revision=38fcbc80a68050abf791e444de4c4cdaf5502063
[2] - https://bugzilla.mozilla.org/show_bug.cgi?id=1569371

Comment 35

[Mike Conley (:mconley) (:⚙️)]

No test jobs were triggered on the push in comment 34, and only Linux 32 was built. I've taken the liberty of retriggering with mochitest-browser tests, and running the win32, win64, linux64 and macOS platforms as well.

Thanks for updating browser_appmenu.js - I think in this case, since the paint in the FxA menu item (the removal of the dot) is expected upon reaching a particular subview, this is a fine change.

Comment 36

[Vijay Budhram]

Thanks :mconley,

I have went ahead and added the exceptions to the browser_appmenu tests and repushed a try [1]. There are a couple different mochitest errors, but those appear unrelated to any browser menu tests. Setting the checkin-needed flag since I think this is ready to land.

[1] - https://treeherder.mozilla.org/testview.html?repo=try&revision=435ab36ba8da16f0fc6832995427e7710bb5f642

Comment 37

[Andreea Pavel [:apavel]]

On Fri, August 2, 2019, 2:08 AM GMT+3, by [e-mail].
Revisions: D38806 diff 140217 ← D38807 diff 140218 ← D38808 diff 140219 ← D38809 diff 140220 ← D38810 diff 140221 ← D38811 diff 140222 ← D38812 diff 140223
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpHhWSvc toolkit/components/resistfingerprinting/nsRFPService.cpp Hunk #4 FAILED at 546. Hunk #5 FAILED at 559. Hunk #7 succeeded at 790 with fuzz 1 (offset 15 lines). 2 out of 8 hunks FAILED -- saving rejects to file toolkit/components/resistfingerprinting/nsRFPService.cpp.rej modules/libpref/init/StaticPrefList.yaml Hunk #1 FAILED at 5602. 1 out of 1 hunk FAILED -- saving rejects to file modules/libpref/init/StaticPrefList.yaml.rej abort: patch command failed: exited with status 256

Comment 38

[Vijay Budhram]

Hey :apavel,

I have rebased and resolved the merged conflicts. This should be good now.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5313c2e63f09
Update FxA toolbar menu for skyline r=eoger,Gijs,fluent-reviewers,flod

Comment 40

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/5313c2e63f09

Comment 41

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Vijay, could you make a release note addition request for our Nightly channel please? thanks
https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

Comment 42

[Vijay Budhram]

[Why is this notable]: This updates a highly visible component in browser.
[Suggested wording]: The Firefox Accounts toolbar menu has been updated and reorganized to give faster access to account features and services.