1534447 - Add an entry point to access saved logins from the main menu

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement, P1)

Description

[Matthew N. [:MattN]]

Attachment: Design from Bryan Bell

Provide easier access to existing saved logins from the panel UI.

This entry point should call LoginHelper.openPasswordManager(…) so that the correct UI is opened after bug 1529729.

[Tracking Requested - why for this release]: New entrypoint to support a Lockbox desktop extension release.

Bryan Bell, can you provide the key asset for the icon or should we used the existing assets we have for the doorhanger (login.svg and login-detailed.svg)?

Comment 1

[Pascal Chevrel:pascalc]

This is soft code freeze week, no new features should land. I am not seeing any Trello card tracking this for 67 or for planned off train releases in 2019 H1. This bug, bug 1529729 and bug 1534442 seem to want landing unplanned features just before the merge, what is the QA story around this lockbox/Firefox Desktop integration?

Comment 2

[Matthew N. [:MattN]]

(In reply to Pascal Chevrel:pascalc from comment #1)

This is soft code freeze week, no new features should land. I am not seeing any Trello card tracking this for 67 or for planned off train releases in 2019 H1.

The extension is only targeting June and isn't a system add-on. Users will manually install it when it launches.

This bug, bug 1529729 and bug 1534442 seem to want landing unplanned features just before the merge, what is the QA story around this lockbox/Firefox Desktop integration?

Unfortunately, this request only came in yesterday due to a delay in UX involvement.

Bug 1529729 isn't a feature or risky change so I don't think it falls under the soft code freeze.

I wasn't planning to land this new menu item during the soft code freeze. I was just going to land the string and then request uplift on using it.

IIUC, Lockbox has their own QA separate from Firefox where they would test things like bug 1529729.

I updated the Trello card at https://trello.com/c/pZMDSciK/349-lockbox-desktop-extension

Comment 3

[Sam Foster [:sfoster] (he/him)]

I'll do this is a couple of parts: the string in its own commit, then the menu item itself.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

MattN and I just had a quick meeting to discuss this code change and related uplifts. It seems like there are two parts to enabling "Lockbox extension in Firefox". The first is to add various entry points: 1) additional menu for logins and passwords in the hamburger menu, 2) saved logins from username/password field on a form, etc. The second part is the QA and risk assessment of the Lockbox extension itself, which is TBD.

Until the second part lands, all the new entry points that MattN and team are adding will open the existing "saved logins" UI (about:preferences#privacy -> Logins & Passwords -> Saved Logins).

Given that, my own assessment of the fix from this bug and bug 1534442 (pref flip only) is that it is low risk and can land today/tomorrow. MattN should decide what works best for his team.

I am hoping Flod and team can localize the one new string being added this week and plan accordingly.

Comment 5

[Matthew N. [:MattN]]

Thanks Ritu!

:flod, is there any issue with adding one new string ("Logins & Passwords") to m-c in the next day or two? It's the same as the login-header[1] string just used in a different context.

[1] https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/browser/locales/en-US/browser/preferences/preferences.ftl#685

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1534447 - Add new string for application menu item to access Logins/Passwords. r?flod,gijs

Comment 7

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #6)

Created attachment 9050447 [details]
Bug 1534447 - Add new string for application menu item to access Logins/Passwords. r?flod,gijs

Note, I used the guidelines on bug 1530771 (from mheubusch) to make this string "Logins and Passwords" rather than "Logins & Passwords".

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #6)

Created attachment 9050447 [details]
Bug 1534447 - Add new string for application menu item to access Logins/Passwords. r?flod,gijs

I went through all potential access keys for this string, and found none not already in use, and some already stacked. Should I just leave it without one for now or is this good as-is?

Comment 9

[bbell]

This mock is incomplete. I'll need to supply a recommendation of where we expect users to land when they click on that.

Comment 10

[Matthew N. [:MattN]]

(In reply to bbell from comment #9)

This mock is incomplete. I'll need to supply a recommendation of where we expect users to land when they click on that.

I think it makes sense to open the dialog like we do now from the other non-preferences entry points (autocomplete, context menu, Page Info) so the user doesn't lose the context of the page they're on.

When do you think you'll have a decision and the key icon asset?

Comment 11

[Sam Foster [:sfoster] (he/him)]

Another question: Do we want a menubar entry for Logins and Passwords too? E.g. under Tools.

Comment 12

[bbell]

This is a living document, I'll be adding more details soon. https://mozilla.invisionapp.com/share/VGQYY71FEMH#/351804149_Artboard_1

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #5)

:flod, is there any issue with adding one new string ("Logins & Passwords") to m-c in the next day or two?

No. Soft freeze period is not string frozen, you can land strings until the final merge day.

Comment 14

[Matthew N. [:MattN]]

(In reply to Francesco Lodolo [:flod] from comment #13)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #5)

:flod, is there any issue with adding one new string ("Logins & Passwords") to m-c in the next day or two?

No. Soft freeze period is not string frozen, you can land strings until the final merge day.

Thanks. That's what I thought but relman wanted me to confirm.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd3b527094a9
Add new string for application menu item to access Logins/Passwords. r=flod,Gijs

Comment 16

[Oana Pop-Rus]

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

Comment 17

[Matthew N. [:MattN]]

Opening the filtered subdialog in about:preferences#privacy is a lot more work and more risky than opening the dialog window (what I proposed in comment 0 and comment 10) so I don't think we should rush that into 67.

I also have concerns about the tab switching as we currently "switch-to-tab" when opening about:preferences meaning that if an existing about:preferences tab is open then we will switch to that tab (which may be far away from the tab the user was on if they have many tabs) and re-use it. We would have to pass the dialog to open, the domain to filter, along with the section of about:preferences to select. All of this would be avoided if we use the existing pattern of opening the dialog in its own window, at least for now.

It doesn't seem worthwhile to put all the effort into deeplinking to a subdialog when Lockbox will replace this within the year.

Comment 18

[Sandy Sage]

I agree with Matt on this. Due to the additional effort required to instrument the proposed flow with deep linking into a subdialog, we're going to press forward where the 'saved logins' link in the menu opens the dialog window (what we already have exiting)

I realize that is not an ideal user experience. We're going to continue to provide users a better flow with the Lockbox integration, starting with the extension.

Comment 19

[bbell]

Attachment: login-16.svg

Here is a production-ready icon for this menu item. Please use this to replace the existing logins icon globally.

Comment 20

[Sam Foster [:sfoster] (he/him)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=297f28b9f2bc12a768dffcf31000524178748f18

IIRC we don't have reftests for the main menu. I'm not sure if there's any other gotchas to landing this which I'm forgetting.

We have test coverage for LoginHelper.openPasswordManager already at toolkit/components/passwordmgr/test/browser/browser_openPasswordManager.js, so I'm not adding any new tests in this patch.

I opened bug 1535758 to cover replacing the old with the new icon globally as that seemed orthogonal to this bug, but I can land that first if we're worried about getting into a state where we have 2 icons representing the same thing.

Comment 21

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1534447 - Add application menu item to show Logins & Passwords. r?Gijs

Comment 22

[Sam Foster [:sfoster] (he/him)]

(In reply to bbell from comment #19)

Created attachment 9051116 [details]
login-16.svg

Here is a production-ready icon for this menu item. Please use this to replace the existing logins icon globally.

I'm assuming we want to get rid of the detailed icon ( chrome://browser/skin/notification-icons/login-detailed.svg ) and use this in its place also?

Comment 23

[Matthew N. [:MattN]]

I would guess so.

Comment 24

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1534447 - Replace login and login-detailed icon with new icon. r?Gijs

Comment 25

[Sam Foster [:sfoster] (he/him)]

Attachment: new-login-autocomplete-icon-bug-1534447.png

Comment 26

[Sam Foster [:sfoster] (he/him)]

Attachment: new-login-appmenu-icon-bug-1534447.png

Comment 27

[Sam Foster [:sfoster] (he/him)]

Attachment: new-login-doorhanger-icon-bug-1534447.png

Comment 28

[Sam Foster [:sfoster] (he/him)]

Attachment: new-login-autocomplete-icon-bug-1534447.png

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecfd8c4af301
Replace login and login-detailed icon with new icon. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6514c64754a5
Add application menu item to show Logins & Passwords. r=Gijs

Comment 30

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ecfd8c4af301
https://hg.mozilla.org/mozilla-central/rev/6514c64754a5

Comment 31

[Timea Cernea [:tbabos][inactive]]

Verified - Fixed on latest Nightly 68.0a1 (2019-03-20) (64-bit) on Windows 7/10 x64, Mac OS 10.13 and Ubuntu 16.04.

• Logins and Passwords option is now displayed in the Firefox main menu. Clicking on it will open the password manager.
• The Save or Update password doorhanger is also displayed as in the attached screenshot
• The new password icon is displayed in the autofill dropdown menu for each suggested user password

Waiting for a potential uplift request before closing the issue.

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 9051412 [details]
Bug 1534447 - Add application menu item to show Logins & Passwords. r?Gijs

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: Users won't have easy access to their saved logins and won't have a Photon icon. This will also support the desktop Lockbox extension release in June.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial menu item addition and icon swap. I talked with Ritu about this change during soft code freeze and she agreed it's low risk.
• String changes made/needed: None. Strings are already in 67.
• Do you want to request approval of these patches as well?: on

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 9051429 [details]
Bug 1534447 - Replace login and login-detailed icon with new icon. r?Gijs

See comment 32

Comment 34

[Pascal Chevrel:pascalc]

Comment on attachment 9051412 [details]
Bug 1534447 - Add application menu item to show Logins & Passwords. r?Gijs

New menu item in the hamburger menu, already verified on Nightly and the translation of the string already started in 67. Approved for 67 beta 5, thanks.

Note to Sheriffs: there are 2 patches to uplift.

Since this is adding a new item in our main menu, we need a release note to expose the feature, Matthew can you propose one please? https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

Comment 35

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bda705ea201f
https://hg.mozilla.org/releases/mozilla-beta/rev/4f154c1d9d9f

Comment 36

[Timea Cernea [:tbabos][inactive]]

Verified - Fixed on latest Beta 67.0b5 on Windows 7/10, Mac OS 10.13 and Ubuntu 16.04.

Comment 37

[Matthew N. [:MattN]]

(In reply to Pascal Chevrel:pascalc from comment #34)

Since this is adding a new item in our main menu, we need a release note to expose the feature, Matthew can you propose one please? https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

I would like to combine the relnote with the one for bug 1534442 but that one isn't ready to uplift yet.

Comment 38

[Matthew N. [:MattN]]

(In reply to Pascal Chevrel:pascalc from comment #34)

Since this is adding a new item in our main menu, we need a release note to expose the feature, Matthew can you propose one please? https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

Release Note Request (optional, but appreciated)
[Why is this notable]: Along with bug 1534442 we are providing easier access to the saved login list on desktop.
[Affects Firefox for Android]: No
[Suggested wording]: Easier access to your list of saved logins from the main menu and login autocomplete.
[Links (documentation, blog post, etc)]: All we have is https://twitter.com/mnoorenberghe/status/1108447892702789632 since Lockbox will talk more about this area later in the year.

Comment 39

[Pascal Chevrel:pascalc]

Added to release notes with beta 7, thanks.

Comment 40

[Chris H-C :chutten]

It seems as though this might have resulted in a sudden and prolonged change in the PWMGR_MANAGE_OPENED probe in Nightly 68 and Beta 67.

Is this true? Is this expected? Is this probe still measuring something useful?

Comment 41

[Sam Foster [:sfoster] (he/him)]

(In reply to Chris H-C :chutten from comment #40)

It seems as though this might have resulted in a sudden and prolonged change in the PWMGR_MANAGE_OPENED probe in Nightly 68 and Beta 67.

Is this true? Is this expected? Is this probe still measuring something useful?

That is actually great news! I'll check if there's any chance these are false signals, but at face value it would appear to tell us that a previously-buried feature is now seeing more use - which was the goal of this bug.

Comment 42

[Matthew N. [:MattN]]

(In reply to Chris H-C :chutten from comment #40)

It seems as though this might have resulted in a sudden and prolonged change in the PWMGR_MANAGE_OPENED probe in Nightly 68 and Beta 67.

Is this true? Is this expected? Is this probe still measuring something useful?

The change in ratio between 0 and 1 would be from bug 1534442 since it accumulates in the 1 bucket. The probe description should be updated but this bug's UI would count in the 0 bucket and increase the volume of accumulations… it's too bad you can't see the number of accumulations by probe value…

Comment 43

[Chris H-C :chutten]

You can, just not in this view. Looking at the distribution view for beta67 with the build from 20190331 we see that we're seeing 32.4k samples in the 1 bucket and 2.37k in the 0 bucket: https://mzl.la/2TWdP2g

Compared to 20190328 with 2.3k samples in the 1 bucket and 15.02k in the 0 bucket: https://mzl.la/2TSoTh6

(( You can see the number of samples and pings in the Evolution view (the bottom two graphs of my links from comment #40) but you need to do the math yourself to turn that into absolute numbers ))

Comment 44

[Matthew N. [:MattN]]

Yeah, I realize I could do it from this page but it would have been really useful to have that on the evolution page in the tooltip.

Comment 45

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9051438 [details]
new-login-doorhanger-icon-bug-1534447.png

Removing old ui-review flag

Comment 46

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9051437 [details]
new-login-appmenu-icon-bug-1534447.png

Removing old ui-review flag