Closed Bug 1188719 Opened 5 years ago Closed 4 years ago

Context menu in username field should allow filling login form

Categories

(Toolkit :: Password Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla43
Iteration:
43.1 - Aug 24
Tracking Status
firefox41 --- unaffected
firefox42 + fixed
firefox43 --- verified

People

(Reporter: rittme, Assigned: rittme)

References

(Depends on 3 open bugs, )

Details

Attachments

(2 files)

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.
Depends on: 1190626
No longer depends on: 1190626
Depends on: 1192081
Assignee: nobody → bernardo
Status: NEW → ASSIGNED
Flags: qe-verify+
Iteration: --- → 43.1 - Aug 24
Bug 1188719 - Shows login fill context menu over username field. r?MattN
Attachment #8647602 - Flags: review?(MattN+bmo)
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.
Depends on: 1194353
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?
Attachment #8647602 - Flags: review?(MattN+bmo)
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
Attachment #8647602 - Attachment description: MozReview Request: Bug 1188719 - Shows login fill context menu over username field. r?MattN → MozReview Request: Bug 1188719 - Shows login fill context menu on username field. r=MattN
Attachment #8647602 - Flags: review?(MattN+bmo)
Bug 1188719 - Tests for the username fill login context menu. r=MattN
Attachment #8648271 - Flags: review?(MattN+bmo)
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"?
Attachment #8647602 - Flags: review?(MattN+bmo)
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"`
Attachment #8648271 - Flags: review?(MattN+bmo)
Depends on: 1196851
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
Attachment #8647602 - Flags: review?(MattN+bmo)
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
Attachment #8648271 - Flags: review?(MattN+bmo)
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 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 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
This last push adds a new condition that should fix bug 1197559.
Blocks: 1197559
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
Attachment #8647602 - Flags: review?(MattN+bmo) → review+
Attachment #8648271 - Flags: review?(MattN+bmo) → review+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7d91d1ee473d
https://hg.mozilla.org/mozilla-central/rev/c8a8bd8f29e6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
QA Contact: kjozwiak
Bernardo, could you fill the uplift request to aurora/42? Thanks
Flags: needinfo?(bernardo)
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.
Flags: needinfo?(bernardo)
Attachment #8647602 - Flags: approval-mozilla-aurora?
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.
Attachment #8647602 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(bernardo)
Matthew, could you take care of the rebase of the patch? Thanks
Flags: needinfo?(MattN+bmo)
(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 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
Flags: needinfo?(MattN+bmo)
Attachment #8647602 - Flags: approval-mozilla-beta?
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.
Attachment #8647602 - Flags: approval-mozilla-beta?
Attachment #8647602 - Flags: approval-mozilla-beta+
Attachment #8647602 - Flags: approval-mozilla-aurora+
Matt, so bug 1192081 need to be rebased for beta first ?
Depends on: 1219273
Flags: needinfo?(bernardo)
Verified fixed FF 43b1 Win 7
Status: RESOLVED → VERIFIED
Depends on: 1297617
You need to log in before you can comment on or make changes to this bug.