Closed Bug 1562006 Opened 4 months ago Closed 3 months ago

[skyline] Update FxA toolbar menu for Skyline

Categories

(Firefox :: Firefox Accounts, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 70
Tracking Status
relnote-firefox --- 70+
firefox70 + fixed

People

(Reporter: vbudhram, Assigned: vbudhram)

References

(Depends on 1 open bug, Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [fxa] [skyline] feature)

Attachments

(2 files, 2 obsolete files)

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
Version: 68 Branch → 69 Branch
Version: 69 Branch → other

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

Assignee: nobody → vbudhram
Whiteboard: feature, [skyline]

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?

Flags: needinfo?(gijskruitbosch+bugs)

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?

Flags: needinfo?(rfeeley)

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.

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

(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. :-)

Flags: needinfo?(gijskruitbosch+bugs)

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?

Flags: needinfo?(rfeeley) → needinfo?(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?

Flags: needinfo?(rfkelly) → needinfo?(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.

Flags: needinfo?(rfeeley)
Priority: -- → P1
Whiteboard: feature, [skyline] → [skyline] [uj] feature
Summary: Update FxA toolbar menu for Skyline → [user journey] Update FxA toolbar menu for Skyline

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?

Flags: needinfo?(rfeeley)

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.

Flags: needinfo?(rfkelly)

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.

Flags: needinfo?(rfeeley)

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

Flags: needinfo?(rfkelly) → needinfo?(markh)

(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.

Flags: needinfo?(markh)
Depends on: 1567403
Depends on: 1567405

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

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!

Attachment #9079732 - Flags: data-review?(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.
Attachment #9079732 - Flags: data-review?(tdsmith) → data-review+
Blocks: 1568553
Blocks: 1568561

Depends on D37961

Whiteboard: [skyline] [uj] feature → [fxa] [skyline] [uj] feature

(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!

Blocks: 1568866

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
Attachment #9079732 - Attachment is obsolete: true
Attachment #9081037 - Flags: data-review?(tdsmith)

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

Summary: [user journey] Update FxA toolbar menu for Skyline → [skyline] Update FxA toolbar menu for Skyline
Whiteboard: [fxa] [skyline] [uj] feature → [fxa] [skyline] feature
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.
Attachment #9081037 - Flags: data-review?(tdsmith) → data-review+
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed

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.

Flags: needinfo?(vbudhram)
Keywords: checkin-needed
Attachment #9080397 - Attachment description: Bug 1562006 - Add support for and update links → Bug 1562006 - Add support for `Continue as` button and update links

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

Flags: needinfo?(vbudhram)
Keywords: checkin-needed

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

Keywords: checkin-needed
Backout by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6ce560581af
Backed out changeset f2b7c868c0a7 for browser chrome failures. CLOSED TREE

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

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.

Flags: needinfo?(vbudhram)

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

Flags: needinfo?(vbudhram)

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

Flags: needinfo?(mconley)

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

Attachment #9080397 - Attachment is obsolete: true

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.

Flags: needinfo?(mconley)

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

Keywords: checkin-needed

On Fri, August 2, 2019, 2:08 AM GMT+3, by apavel@mozilla.com.
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

Flags: needinfo?(vbudhram)
Keywords: checkin-needed

Hey :apavel,

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

Flags: needinfo?(vbudhram)
Keywords: checkin-needed

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

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Depends on: 1572097
Blocks: 1569620

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

Flags: needinfo?(vbudhram)

[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.

relnote-firefox: --- → ?
Flags: needinfo?(vbudhram)
Blocks: 1573959
Regressions: 1577029
You need to log in before you can comment on or make changes to this bug.