Closed Bug 1463797 Opened Last year Closed Last month

hitting <enter> in password update UI does nothing

Categories

(Toolkit :: Password Manager, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: dietrich, Assigned: sfoster, Mentored)

References

Details

Attachments

(2 files)

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.
Component: General → Password Manager
Product: Firefox → Toolkit
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.
Mentor: sfoster
Priority: -- → P3
(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.
Assignee: nobody → alexandersonone
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.
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
I haven't added the test yet, but here's my working code so far.
: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.
Flags: needinfo?(MattN+bmo)
Sorry for the delay, I'll get to this as soon as I can.
Flags: needinfo?(MattN+bmo)
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
Attachment #8989604 - Flags: review?(MattN+bmo)
Re-assigning as Alex let me know he wont be able to follow up on this any time soon.
Assignee: alexandersonone → sfoster
Attachment #9087874 - Attachment description: Bug 1463797 - Added a method to hit enter to save passwords while password box is focused and test. r?MattN → Bug 1463797 - (WIP) Added a method to hit enter to save passwords while password box is focused and test. r?MattN
Attachment #9087874 - Attachment description: Bug 1463797 - (WIP) Added a method to hit enter to save passwords while password box is focused and test. r?MattN → Bug 1463797 - Added a method to hit enter to save passwords while password box is focused and test. r?MattN

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.

Pushed by sfoster@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.