Open Bug 1529025 Opened 5 years ago Updated 2 years ago

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

Categories

(Toolkit :: Password Manager, defect, P5)

defect

Tracking

()

UNCONFIRMED

People

(Reporter: MattN, Unassigned)

References

Details

Attachments

(2 files)

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

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?

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

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

Hey, I am already working on this

Hey, can I take up this issue?

Hey, I am already working on this

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

Okay, so I understood it now.

Add test file.

Add test file.

Add test file.

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

@Matthew N.[:MattN], please review

Assignee: nobody → sresthasrivastava.ss
Status: NEW → ASSIGNED

Hello, did you see my review comments on Phabricator?

Flags: needinfo?(sresthasrivastava.ss)

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

Assignee: sresthasrivastava.ss → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(sresthasrivastava.ss)

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

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

Assignee: nobody → nguyenvandung02051999
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)

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

Flags: needinfo?(nguyenvandung02051999)

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

Flags: needinfo?(nguyenvandung02051999)

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

Flags: needinfo?(MattN+bmo)

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.

Flags: needinfo?(nguyenvandung02051999)

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

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

Flags: needinfo?(nguyenvandung02051999)
Mentor: MattN+bmo
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
Priority: P3 → P5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee: nguyenvandung02051999 → nobody
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false

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?

Assignee: nobody → sresthasrivastava.ss

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

Assignee: sresthasrivastava.ss → nobody
Keywords: good-first-bug
Whiteboard: [lang=js]
Severity: minor → S4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: