1463797 - hitting <enter> in password update UI does nothing

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Dietrich Ayala (:dietrich)]

Sorry, I can't find a component for permissions UI in Firefox, so putting this in General.

62.0a1 (2018-05-22) (64-bit) Mac OS X

STR:

1. go to site and reset password
2. Firefox opens "update password?" panel
3. enter new password
4. hit <enter> key

Expected: panel closes

Actual: nothing happens. have to use mouse to click button.

Comment 1

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

Looks like extending the input handler at  https://dxr.mozilla.org/mozilla-central/rev/47e81ea1ef10189ef210867934bf36e14cf223dc/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#895-898 to watch for ENTER should fix this.

Comment 2

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

(In reply to Sam Foster [:sfoster] from comment #1)
> Looks like extending the input handler at 
> https://dxr.mozilla.org/mozilla-central/rev/
> 47e81ea1ef10189ef210867934bf36e14cf223dc/toolkit/components/passwordmgr/
> nsLoginManagerPrompter.js#895-898 to watch for ENTER should fix this.

We don't get the 'key' property on the input event, so this will need a new keyup listener to complete the action when we see 'Enter'. 

This new behavior will need a test. It should be enough to have a single test that verifies the 'enter' key on the password field has the same effect as clicking the main action button.

ISTM we should be able to call doCommand() on something here to fully complete the main action and dismiss the popup in the same way clicking on the "save" button do. But I'm not yet sure where/how.

Comment 3

[Alex]

I did get the enter to save working, but I have yet to get a test along with it.

To fix it, you have to create a new function in mozilla-central/toolkit/components/passwordmgr/nsLoginManagerPrompter.js below 
the onInput function (line 898). The function needs to look something like this:

  let handleSubmit = (e) => {
    if (e.keyCode == 13) {
      this.mainActionButton.click()
    }
  };


Then you have to add an event listener inside the "showing" case around line 1000. This should look as follows:

  chromeDoc.getElementById("password-notification-password")
           .addEventListener("keyup", handleSubmit);

Finally, to be able to have scope of the button, a line needs to be added beneath 830.

  this.mainActionButton = mainActionButton


I have no clue of how to add a test though, and I'm confused to as if this is the right way to go about fixing it.

Comment 4

[Matthew N. [:MattN]]

I think you're on the right track but it would be easier to see if you submit the changes to the bug as a patch/commit.

For tests you can look at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js and add to it.

Comment 5

[Alex]

Attachment: bug 1463797 - Added a method to hit enter to save passwords while password box is focused and test.

Added a method to hit enter to save passwords while password box is focused.

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

Comment 6

[Alex]

I haven't added the test yet, but here's my working code so far.

Comment 7

[Alex]

Comment on attachment 8989604 [details]
bug 1463797 - Added a method to hit enter to save passwords while password box is focused and test.

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

Comment 8

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

:MattN, this is ready for an initial review. Alex won't be able to spend much more time on this but has said he can make any changes to get it landed.

Comment 9

[Matthew N. [:MattN]]

Sorry for the delay, I'll get to this as soon as I can.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8989604 [details]
bug 1463797 - Added a method to hit enter to save passwords while password box is focused and test.

https://reviewboard.mozilla.org/r/254622/#review264922

Looks good! Just some minor fixes and adding support on the username field.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:839
(Diff revision 2)
>  
>      let currentNotification;
>  
>      let updateButtonStatus = (element) => {
>        let mainActionButton = element.button;
> +      this.mainActionButton = mainActionButton;

Can't you use `currentNotification.button.click()` instead of having to use this new variable (which may get out of sync)?

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:910
(Diff revision 2)
>        readDataFromUI();
>        updateButtonLabel();
>      };
>  
> +    let handleSubmit = (e) => {
> +      if (e.keyCode == 13) {

As mentioned on https://developer.mozilla.org/en-US/docs/Web/Events/keyup, .keyCode is deprecated in favor of .key:
```js
if (e.key == "Enter") {
```

Please double-check the above though.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1037
(Diff revision 2)
> +              chromeDoc.getElementById("password-notification-password")
> +                       .addEventListener("keyup", handleSubmit);

I would guess this behaviour should happen on the username field too? Add a test for that too please.

::: toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js:587
(Diff revision 2)
> +    await EventUtils.synthesizeKey("KEY_Enter");
> +
> +  });

Nit: delete the empty line

::: toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js:598
(Diff revision 2)
> +  info("Make sure Remember took effect and we don't prompt for an existing login");
> +  await testSubmittingLoginForm("subtst_notifications_1.html", function(fieldValues) {
> +    is(fieldValues.username, "notifyu1", "Checking submitted username");
> +    is(fieldValues.password, "notifyp1", "Checking submitted password");
> +    let notif = getCaptureDoorhanger("password-save");
> +    ok(!notif, "checking for no notification popup");
> +  });
> +
> +  logins = Services.logins.getAllLogins();
> +  is(logins.length, 1, "Should only have 1 login");
> +  login = logins[0].QueryInterface(Ci.nsILoginMetaInfo);
> +  is(login.username, "notifyu1", "Check the username used");
> +  is(login.password, "notifyp1", "Check the password used");
> +  is(login.timesUsed, 2, "Check times used incremented");
> +
> +  checkOnlyLoginWasUsedTwice({ justChanged: false });

I don't think you need to duplicate this part which is covered by other tests

Comment 11

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

Re-assigning as Alex let me know he wont be able to follow up on this any time soon.

Comment 12

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

Attachment: Bug 1463797 - Added a method to hit enter to save passwords while password box is focused and test. r?MattN

Comment 13

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

Can't you use currentNotification.button.click() instead of having to use
this new variable (which may get out of sync)?

currentNotification has the mainAction callback, but doesnt give us a way to get to the button and the command. In the latest patch I use e.target.closest("popupnotification").button to get to it.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6b8e557ccdf
Added a method to hit enter to save passwords while password box is focused and test. r=MattN

Comment 15

[Andreea Pavel [:apavel]]

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