Closed Bug 1370218 Opened 5 years ago Closed 5 years ago

[Mac] Pressing backspace to remove a password in the password dialog (in preferences), also removes the dialog

Categories

(Firefox :: Preferences, defect)

53 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox56 --- verified

People

(Reporter: standard8, Assigned: dbugs)

Details

Attachments

(1 file)

This is a similar issue to bug 1342472 and bug 1365957, but slightly different effect.

STR

1) Save a password on a site.
2) Open preferences
3) Select privacy & security
4) Forms & Passwords -> Saved Logins
5) Select the password added in step 1
6) Press Backspace.

Actual Results

-> The password is removed and the dialog is closed.

Expected Result

-> The password is removed, the dialog stays open.

Note: there's an example test in bug 1342940 that could probably be used to write a test for this.
Comment on attachment 8883337 [details]
Bug 1370218 - [Mac] Pressing backspace to remove a password in the password dialog (in preferences) should leave the dialog open.

https://reviewboard.mozilla.org/r/154234/#review159716

Thanks Dan, this looks good. Please make the small test changes to the duplicate test file as well, where applicable.

Cheers

::: commit-message-5bed7:1
(Diff revision 1)
> +Bug 1370218 - [Mac] Pressing backspace to remove a password in the password dialog (in preferences), also removes the dialog. r?MattN

Nit: Generally the bug summary should indicate the problem and the commit message should indicate the solution. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment

::: browser/components/preferences/in-content/tests/browser_password_management.js:6
(Diff revision 1)
> +"use strict";
> +const PM_URL = "chrome://passwordmgr/content/passwordManager.xul";
> +
> +var passwordsDialog;
> +
> +add_task(async function test_setup() {

Nit: Normally the `test_` prefix is for test tasks, not setup tasks. `setup` is a good enough name.

::: browser/components/preferences/in-content/tests/browser_password_management.js:7
(Diff revision 1)
> +  let pwmgr = Cc["@mozilla.org/login-manager;1"].
> +                getService(Ci.nsILoginManager);

Nit: You could use `Services.logins` directly in order to hide some XPCOM goop.

::: browser/components/preferences/in-content/tests/browser_password_management.js:12
(Diff revision 1)
> +  let nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1",
> +                                                 Ci.nsILoginInfo, "init");

Nit: Align the `Ci` with the double-quote of the line above.
Attachment #8883337 - Flags: review?(MattN+bmo) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf0bcf30a672
[Mac] Pressing backspace to remove a password in the password dialog (in preferences) should leave the dialog open. r=MattN
https://hg.mozilla.org/mozilla-central/rev/cf0bcf30a672
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Flags: qe-verify+
I tried to reproduce this bug using Nightly from 2017-06-05 on Mac OS X 10.11 and macOS 10.12 but I couldn't reproduce it. 
Maybe there is something that I am not doing right or I missed something, but I did follow the steps that are written in the comment 0. 

Maybe, if you have the time, can you please check and see if it's still reproducing on latest Firefox 56 beta?
Flags: needinfo?(standard8)
I've verified this reproduces on Beta 55, but then works fine on Beta 56b9.
Status: RESOLVED → VERIFIED
Flags: needinfo?(standard8)
(In reply to Mark Banner (:standard8) from comment #7)
> I've verified this reproduces on Beta 55, but then works fine on Beta 56b9.

Thanks Mark!
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.