1529025 - Fill Login/password context menu doesn't respect the signon.rememberSignons preference

Status: UNCONFIRMED

(Toolkit :: Password Manager, defect, P5)

Description

[Matthew N. [:MattN]]

If you set signon.rememberSignons to false via about:config or about:preferences then the Fill Login/password context menu is still displayed. Currently all other fill UI is disabled in that case.

I know there is a bug where users want that pref to just be about prompts to save but we haven't made a decision on that yet and this bug makes us inconsistent.

You need to fix [1] to use LoginHelper.enabled and add

this.enabled = Services.prefs.getBoolPref("signon.rememberSignons");

to updateSignonPrefs. You should also add a test to toolkit/components/passwordmgr/test/browser/browser_context_menu.js and you'll want to use the third argument to openPasswordContextMenu to check that the menu item isn't visible and prevent that helper from trying to open the menu. See the other tests that used that third argument for examples.

[1] https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/browser/base/content/nsContextMenu.js#721-727
[2] https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/toolkit/components/passwordmgr/test/browser/browser_context_menu.js#272-283

Comment 1

[SresthaSrivastava [:srestha]]

Hey, I am an outreachy applicant interested in the project "Implement a new certificate viewer for Firefox". Can I take up this issue as a part of my contribution?

Comment 2

[Matthew N. [:MattN]]

Sure thing! Feel free to submit a diff on Phabricator and then we'll assign the bug. Let me know if you have questions.

Comment 3

[PhoenixAbhishek]

Hi, I am an Outreachy applicant.
Can I take this up?
Do we need compete build of Firefox for this, or artefact build can be used..???

Comment 4

[SresthaSrivastava [:srestha]]

Hey, I am already working on this

Comment 5

[Aashna]

Hey, can I take up this issue?

Comment 6

[SresthaSrivastava [:srestha]]

Hey, I am already working on this

Comment 7

[SresthaSrivastava [:srestha]]

Hey @Matthew N., can you tell me the steps to reproduce this?

Comment 8

[SresthaSrivastava [:srestha]]

Okay, so I understood it now.

Comment 9

[SresthaSrivastava [:srestha]]

Attachment: [WIP]: Bug 1529025- used LoginHelper.enabled r=Matthew N.[:MattN]

Add test file.

Add test file.

Add test file.

Comment 10

[SresthaSrivastava [:srestha]]

@Matthew N.[:MattN], please review

Comment 11

[Matthew N. [:MattN]]

Hello, did you see my review comments on Phabricator?

Comment 12

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

:srestha, let us know if you want to continue work on this bug. I'm setting unassigned for now.

Comment 13

[Peter Nguyen]

Hi :MattN , I am working on fixing this issue. Can you assign me this bug?

Comment 14

[Peter Nguyen]

Attachment: Bug 1529025 - Fill Login/password context menu doesn't respect the signon.rememberSignons preference. r?sfoster

Comment 15

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

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

Comment 16

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

:MattN, do you want to also review attachment 9069418 [details] or just delegate to my review?

Comment 17

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

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

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

Peter, it looks like it might be worth looking at browser/base/content/test/contextMenu/browser_contextmenu_input.js to see if we need the same fix to ensure the signon.rememberSignons pref is set when the test are run.

Comment 18

[Peter Nguyen]

I see :sfoster. I'm working on it.

Comment 19

[Matthew N. [:MattN]]

We talked about this and due to bug 1451864 I think we may want to revisit my initial report.

Comment 20

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

Peter, it seems there is some inconsistency in what the signon.rememberSignons preference should mean and control. Historically, we have used it to enable/disable saving of logins, but in a recent change last years, the string (text) for the preference was changed to read "Ask to save logins and passwords for websites" which doesnt mean quite the same thing. If the pref is only about asking to save - not about supporting the saving of logins, that casts the value of this bug and your patch into doubt as perhaps the user would want to fill logins from the context menu even with this pref turned off. Feel free to get your tests passing and update the patch here if you want to wrap it up, but we'll likely not merge this patch unless the situation changes.

Comment 21

[Peter Nguyen]

Oh, I see, Sam. So do you mean we will revert our code to its initial state and leave this feature intact?

Comment 22

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

(In reply to Peter Nguyen from comment #21)

Oh, I see, Sam. So do you mean we will revert our code to its initial state and leave this feature intact?

Yeah, you can push your latest changes to keep a permanent record so we can pick this back up if this decision changes in the future.

Comment 23

[Matt Coomber]

Wondering if a about:config option could be added to disable this if it's not going to be integrated with the main saving of logins option?

Comment 24

[Matthew N. [:MattN]]

(In reply to Matt Coomber from comment #23)

Wondering if a about:config option could be added to disable this if it's not going to be integrated with the main saving of logins option?

That adds additional complexity for development in order to keep it working so we don't want to add a preference unless it's user-facing.