1188719 - Context menu in username field should allow filling login form

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Bernardo Rittmeyer [:rittme]]

The context menu opened from a username field should show a list of logins from the password manager to allow manual form fill. This should have a similar interface to the one from bug 433238.

Comment 1

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

Bug 1188719 - Shows login fill context menu over username field. r?MattN

Comment 2

[Bernardo Rittmeyer [:rittme]]

This is not a final version. I'm pushing it only for some feedback right now. 
Mainly on the `getFieldContext` method and it's results. I could return the actual field Elements, but I don't know if passing them to the parent would be recommended. I would assume we should try not to do that, but I'm not sure.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

https://reviewboard.mozilla.org/r/16015/#review14343

I think this approach is fine.

::: browser/base/content/nsContextMenu.js:513
(Diff revision 1)
>      let showFillPassword = this.onPassword;

This alias adds indirection without much benefit AFAICT.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1053
(Diff revision 1)
> +    let [usernameField, newPasswordField, oldPasswordField] =
> +          this._getFormFields(form, false);
> +
> +    return {
> +      usernameField: {
> +        found: !!usernameField,
> +        disabled: usernameField && (usernameField.disabled || usernameField.readOnly),

Why are we looking at the username field from `_getFormFields` instead of the field the user chose?

Comment 4

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b667e6949667

Comment 5

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6085bf15721

Comment 6

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

Bug 1188719 - Shows login fill context menu on username field. r=MattN

Comment 7

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1188719 - Tests for the username fill login context menu. r=MattN

Bug 1188719 - Tests for the username fill login context menu. r=MattN

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

https://reviewboard.mozilla.org/r/16015/#review14617

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:479
(Diff revision 2)
> +      if (inputElement.type == "password") {
> -      clobberUsername = false;
> +        clobberUsername = false;

I just wrote:
"Doesn't `clobberUsername = false;` mean that we will abort filling the password if the wrong username exists in the username field? If so, I don't think that's what we want."
but I see that you handle this with `usernameField = null;` in `_fillForm`. I wonder if we should move the clobberUsername overriding inside `_fillForm` or have `_fillForm` honour clobberUsername = true with an inputElement. As-is one can pass arguments that don't do what the argument says. I don't know the right answer to whether the check should be moved but I think there should be a comment here indicating that the found username field will be totally ignored. 

We should come up with better documentation on the arguments and/or with assertions to document some cases like how we handle username values and empty saved usernames.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1058
(Diff revision 2)
> +    let [usernameField, newPasswordField, oldPasswordField] =
> +          this._getFormFields(form, false);

Recipes should be passed for all new code. In this case it can be a problem when right-clicking on a username field as the detected password field can be wrong without the recipe. I'm fine with a sync message for now if async is hard. It's better than no recipes (which I want to make required in the API).

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1062
(Diff revision 2)
> +    // to use parameter element as the username field.

"parameter element"?

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8648271 [details]
MozReview Request: Bug 1188719 - Tests for the username fill login context menu. r=MattN

https://reviewboard.mozilla.org/r/16167/#review14621

Looks good overall but please cleanup some of the major duplication in the test file. Even better if you use head.js and split into 2 tests.

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:57
(Diff revision 1)
> +    url: TEST_HOSTNAME + "/browser/toolkit/components/" +
> +         "passwordmgr/test/browser/multiple_forms.html",

Use a const at the top for this now that you're using it many times.

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:180
(Diff revision 1)
> +add_task(function* test_context_menu_login_fill() {

indicate in the name that this test is for the username field.

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:185
(Diff revision 1)
> +      popupStatus: "hidden",

I thought this meant that the popup shouldn't appear. Can you find a more accurate name? Maybe menuitemStatus?

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:250
(Diff revision 1)
> +  yield BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: TEST_HOSTNAME + "/browser/toolkit/components/" +
> +         "passwordmgr/test/browser/multiple_forms.html",
> +  }, function* (browser) {
> +    for (let testCase of testSet) {
> +      info("Testing username field: " + testCase.usernameInput);
> +      let usernameInput = browser.contentWindow.document.getElementById(testCase.usernameInput);
> +
> +      // Synthesize a right mouse click over the username input element.
> +      let contextMenuShownPromise = BrowserTestUtils.waitForEvent(window, "popupshown");
> +      let eventDetails = {type: "contextmenu", button: 2};
> +      BrowserTestUtils.synthesizeMouseAtCenter(usernameInput, eventDetails, browser);

…

There's a lot of duplication here from the existing test that this feels hard to maintain. Could you refactor some of it to re-use code?

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:290
(Diff revision 1)
> +      let unchangedFieldsValues = null;
> +      if (testCase.unchangedFields) {
> +        unchangedFieldsValues = [];
> +        for (let fieldId of testCase.unchangedFields) {
> +          unchangedFieldsValues[fieldId] = browser.contentWindow.document.getElementById(fieldId).value;

Is unchangedFieldsValues an array or an object? Why aren't you initializing on the declaration line? I think you want a Map instead anyways.

::: toolkit/components/passwordmgr/test/browser/multiple_forms.html:12
(Diff revision 1)
> +<form id='form-1-1'>

Is this ID even used? Using number in testcase isn't normally good idea because of problems like this when you want to reorder. If you need an ID, use a descriptive one or get rid of it.

::: toolkit/components/passwordmgr/test/browser/multiple_forms.html:69
(Diff revision 1)
> +    <input id='test-username-9' type='text'     name='uname' value='' disabled='true'>

Boolean attributes in HTML (not XUL) use the attribute name as the value so this should be either just `disabled` or `disabled="disabled"`

Comment 10

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

Bug 1188719 - Shows login fill context menu on username field. r=MattN

Comment 11

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8648271 [details]
MozReview Request: Bug 1188719 - Tests for the username fill login context menu. r=MattN

Bug 1188719 - Tests for the username fill login context menu. r=MattN

Comment 12

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/16015/#review14617

> I just wrote:
> "Doesn't `clobberUsername = false;` mean that we will abort filling the password if the wrong username exists in the username field? If so, I don't think that's what we want."
> but I see that you handle this with `usernameField = null;` in `_fillForm`. I wonder if we should move the clobberUsername overriding inside `_fillForm` or have `_fillForm` honour clobberUsername = true with an inputElement. As-is one can pass arguments that don't do what the argument says. I don't know the right answer to whether the check should be moved but I think there should be a comment here indicating that the found username field will be totally ignored. 
> 
> We should come up with better documentation on the arguments and/or with assertions to document some cases like how we handle username values and empty saved usernames.

The way I adressed this is by adding a bit more documentation to the clobberUsername parameter and doing `usernameField = null;` only if clobberUsername is false. So now it should follow the following behaviour:
```
clobberUsername  inputElement[type=password]  Action
true             true                         Clobber the username.
false            true                         Fill only the password field. Ignore username field value.
true             false                        Clobber the username.
false            false                        Fill only the password field. If the already present username is not present in foundLogins, do nothing.
```

I also filed Bug 1196851 to make a more complete investigation there.

Comment 13

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

Bug 1188719 - Shows login fill context menu on username field. r=MattN

Comment 14

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8648271 [details]
MozReview Request: Bug 1188719 - Tests for the username fill login context menu. r=MattN

Bug 1188719 - Tests for the username fill login context menu. r=MattN

Comment 15

[Bernardo Rittmeyer [:rittme]]

This last push adds a new condition that should fix bug 1197559.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

https://reviewboard.mozilla.org/r/16015/#review15997

Sorry for the delay

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8648271 [details]
MozReview Request: Bug 1188719 - Tests for the username fill login context menu. r=MattN

https://reviewboard.mozilla.org/r/16167/#review16003

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7d91d1ee473d
https://hg.mozilla.org/integration/fx-team/rev/c8a8bd8f29e6

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d91d1ee473d
https://hg.mozilla.org/mozilla-central/rev/c8a8bd8f29e6

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Bernardo, could you fill the uplift request to aurora/42? Thanks

Comment 21

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

Approval Request Comment
[Feature/regressing bug #]: Allow filling login from username field. This expands on the features from bug 433238.
[User impact if declined]: Users will be able to fill password at password field but not login from login field. This also fixes the regression from bug 1197559.
[Describe test coverage new/current, TreeHerder]: The patch landed on central 10 days ago. We have good automated testing coverage and manual testing.
[Risks and why]: Medium-low risk, build over feature already present, adds a new object to the context menu global.
[String/UUID change made/needed]: none.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

New feature, we want to polish it.

Comment 23

[Carsten Book [:Tomcat]]

Hi, this doesnot apply cleanly to aurora

3 files to edit
merging toolkit/components/passwordmgr/LoginManagerContent.jsm failed!
abort: unresolved conflicts, can't continue

can you take a look, thanks!

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Matthew, could you take care of the rebase of the patch? Thanks

Comment 25

[Matthew N. [:MattN]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Matthew, could you take care of the rebase of the patch? Thanks

The problem is that bug 1192081 isn't on 42.

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

42 is now beta.
Approval Request Comment: see comment 21

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647602 [details]
MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN

OK, as been in aurora for a month, polish a new feature, taking it.

Comment 28

[Carsten Book [:Tomcat]]

Matt, so bug 1192081 need to be rebased for beta first ?

Comment 29

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b0aa8d67c934
https://hg.mozilla.org/releases/mozilla-beta/rev/645e5fe8f354

Comment 30

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 43b1 Win 7