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

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Mark Banner (:standard8)]

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 1

[Dan Banner]

Attachment: Bug 1370218 - [Mac] Pressing backspace to remove a password in the password dialog (in preferences) should leave the dialog open.

Review commit: https://reviewboard.mozilla.org/r/154234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154234/

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

[Dan Banner]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154234/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
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

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 6

[Bogdan Maris, Desktop QA]

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?

Comment 7

[Mark Banner (:standard8)]

I've verified this reproduces on Beta 55, but then works fine on Beta 56b9.

Comment 8

[Bogdan Maris, Desktop QA]

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