433238 - Can't fill in a password-only form when there are multiple saved logins

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Bill Gianopoulos [:WG9s]]

Bug 362576 modified autocomplete so that it will not fill in forms if autocomplete=off is specified.

Unfortunately it did NOT also modify the code to not prompt for the Master password to get the login information for the site if autocomplete=off is specified.

So, what happens now is that if you go to page which specifies autocomplete=off which also asks for a login, and you have site login information saved for the site in question, it prompts for the Master password and then does nothing with the information in the password file.

This is a security issue as well as a usability because it results in your password file being open when it need not be.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> Bug 362576 modified autocomplete so that it will not fill in forms if
> autocomplete=off is specified.

Wrong. Bug 362576 made autocomplete not _automatically_ prefill the password when autocomplete=off is specified. It will still autocomplete if you use the dropdown (i.e. the same behavior as when you have multiple passwords saved).

That makes the rest of the report invalid.

Comment 2

[Bill Gianopoulos [:WG9s]]

(In reply to comment #1)
> (In reply to comment #0)
> > Bug 362576 modified autocomplete so that it will not fill in forms if
> > autocomplete=off is specified.
> 
> Wrong. Bug 362576 made autocomplete not _automatically_ prefill the password
> when autocomplete=off is specified. It will still autocomplete if you use the
> dropdown (i.e. the same behavior as when you have multiple passwords saved).
> 
> That makes the rest of the report invalid.
> 
That does not appear to be the case for forms that contain only a password and no username (like bank sites that use sitekey where the username is entered on one page and then the password on a subsequent page).  The username field will get filled in with no prompt for the Mater Password.  When you go to the second page for which the only form field is the password, you are prompted for the Mater Password and nothing is filled in (or if it is possible to do this via the dropdown, I have no idea what he magic hot-key is to make this happen).

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Oh, maybe that patch does break password-only logins since there's no way to trigger pre-filling. That's not quite the same thing problem that's described in the summary, though.

Comment 4

[Justin Dolske [:Dolske]]

Bug 400680 covers the master password prompting problem. That fix was originally just for when the autoFillForms pref was set to false, but it's the same code path that autocomplete=off takes.

The remaining issue is what comments 2 / 3 describe here. This is basically already a known issue (although it seems we didn't have a bug on it!) for password-only forms when multiple logins are available. We don't attach an autocomplete dropdown there (what would it show, passwords? yikes!), and for password-only logins there isn't a primary selection key (username) to show the user.

Comment 5

[Bill Gianopoulos [:WG9s]]

(In reply to comment #3)
> Oh, maybe that patch does break password-only logins since there's no way to
> trigger pre-filling. That's not quite the same thing problem that's described
> in the summary, though.
> 
Yes, I should have changed the summary after you explained how it worked with a username/password form in comment #1.

Comment 7

[al_9x]

(In reply to comment #6)
> *** Bug 434094 has been marked as a duplicate of this bug. ***
> 

Perhaps multiple logins and single login with autocomplete=off should not be combined in one bug because the former can't be autofilled while the latter can be, if you provide an option to do so, and doesn't require attaching the autocomplete handler to the password field.  The solutions are potentially different.

Comment 8

[al_9x]

>The solutions are potentially different.

or to put it slightly better the multiple logins case may not have a solution but the single login password-only with autocoplete="off" certainly does, autofill, if you just allow it.

Comment 9

[al_9x]

when ff2 can fill in all your passwords but ff3 can't that's a major regression, is this bug being ignored?

Comment 10

[Justin Dolske [:Dolske]]

I disagree -- it's not a major regression. The autocomplete=off case is the same problem as the existing multiple-login problem. It should be fixed (which is why this bug is open), but that's not going to happen in the initial release.

Comment 11

[al_9x]

>The autocomplete=off case is the same problem as the existing multiple-login
>problem.

the multiple login problem has no solution you said so yourself: "what would it show, passwords? yikes!"  But the single login case has a several solutions I can think of:
 
1) make the recently disabled autofill for autocomplete=off optional.

2) even without an option you can safely autofill a password only form with one user editable field, it's a perfect match

3) attach the autocomplete handler to the single password field

as far as how major this is, I have a couple of frequently used logins at widely used sites that worked for years with ff2 and don't with ff3

>but that's not going to happen in the initial release.

please reconsider, it's not a lot of effort to fix this for the single login

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I have to administer a mailing list on lists.mozilla.org and run into this problem too after setting signon.autofillForms to false. Now that the password isn't filled in automatically anymore I have to c&p it manually. That's a bit annoying if you have to do it multiple times a day.

As I can see from the above comments it will be hard to implement. IMO a nice solution would be to be able to set a site-wide permission. So on specific and safe sites we could use the autofill feature. It will offer more control over these sensitive data to users.

Comment 13

[Max Kalashnikov]

I agree that this is a major regression, especially from a user standpoint.

If it doesn't hurt anything, even the future possibility of programming perfectionism, why not fix it?

Comment 15

[:Gijs (out for now; he/him)]

Matt/dolske, can you outline what needs to happen here? I've read the comments here and in bug 1033648 multiple times, but it's not clear to me. It seems there are at least 3 cases:

1) form with only a password field and multiple logins
2) form with a username and password field where the username is prefilled by the website, and autocomplete=off is specified (bug 1033648)
3a) form with only a password field and autocomplete=off (1 login)
3b) form with only a password field and autocomplete=off (multiple logins)

It's not clear to me what the proposed fix is (see also: comment #4). I'm going to give my thoughts and hope that you can clarify:

A) First, it seems to me that we should assume that cases (2) and (3a) are identical from a security perspective: the username is either explicitly or implicitly given by the page. If we're happy to prefill login info in an autocomplete=off form once the *user* specifies the login, perhaps we should just do the same if the webpage does so (ie there's only a password field), despite autocomplete=off? Is there a compelling reason not to do this?

B) Second, for the cases with multiple logins (1 and 3b) perhaps we could do an autocomplete popup that has the usernames as labels and the passwords as values? i.e. "password for [e-mail]", "password for "[e-mail]" ?

I don't know how much work it'd be to make the autocomplete implementation support (B). I also don't know if there are reasons not to implement (A). If we can't do (A), perhaps we could devolve to (B) even in the single-login case, and label with the username?

Comment 16

[Justin Dolske [:Dolske]]

There is no proposed fix, just a bug tracking the problem. Presumably we would need an additional form of UI to handle these cases.

This bug covers cases 1 and 3 in your comment, case 2 would be different. Password manager currently relies on the user being able to select a username from the username autocomplete dropdown (to either fill in a login when autocomplete=off is present, or to select which login to use when multiple logins are available). When there's no username field, we can't handle those two cases.

Case 2 should work today by clearing the input and entering a username / selecting a username from the resulting autocomplete. If not, that's a separate bug.

Username labels won't work when there is no username associated with a login.

Comment 17

[Chris Karlof [:ckarlof]]

Currently blocked by a technical and UX design, but we want to address it in Q1 2015.

Comment 18

[Chris Karlof [:ckarlof]]

The autocomplete=off functionality would addressed by bug 1025703.

Comment 19

[Chris Karlof [:ckarlof]]

Can likely be addressed by bug 1129596.

Comment 20

[Matthew N. [:MattN]]

The autocomplete=off aspect should have been addressed by bug 1025703.

Context menu UI is defined at https://www.lucidchart.com/documents/edit/87ab1cc8-e708-49d3-8b91-6e2e6da346fb/9

Comment 21

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Fill password field from contextual menu logins list r?MattN

Comment 22

[Bernardo Rittmeyer [:rittme]]

There is still some work to be done here, but I pushed it to review so we can check if it's the right way to go and discuss some choices I made. 

For the actual form filling part, I think we could expand on the `formFill` methods in `LoginManagerParent` and `LoginManagerContent`. They are currently being used by the fill doorhanger. Since what we want to do here is mostly the same, fill the form, I think adding a new method here would be redundant. By adding an optional `target` parameter to `formFill`, we can choose the right form/formLike to fill out, in the case of a multi-form page. This way we reuse an architecture that's already there, making it simpler and writing less code. Using this same architecture, going from filling only the password field to filling the entire form would be a simpler change (in fact, for now this patch is already filling the username too).

Comment 23

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Fill password field from contextual menu logins list r?MattN

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

https://reviewboard.mozilla.org/r/13113/#review11859

Overall this approach seems to work well. Great job on figuring out all the various pieces!

Some notes to keep in mind:
* make sure right-click fill in PW field doesn't consider username field value
* maxlength doesn't seem to be considered (ok as a follow-up). Check fillForm code to see what other attributes should be considered.

::: browser/base/content/nsContextMenu.js:506
(Diff revision 2)
> +  initPasswordManagerItems: function CM_initPasswordManagerItems() {

Nit: remove s/: function CM_initPasswordManagerItems//

::: browser/base/content/nsContextMenu.js:507
(Diff revision 2)
> +    var showFillPassword = this.onPassword && Services.logins.isLoggedIn;

s/ var / let /

::: browser/base/content/nsContextMenu.js:511
(Diff revision 2)
> +    if (showFillPassword) {

early return `if (!showFillPassword) {`?

::: browser/base/content/nsContextMenu.js:515
(Diff revision 2)
> +        target: this.target,

`target` name should be more descriptive.

::: browser/base/content/nsContextMenu.js:515
(Diff revision 2)
> +        target: this.target,
> +        browser: this.browser,

I wonder if you can just do:
`this.target,
this.browser,`
with the new syntax?

::: browser/themes/shared/contextmenu.inc.css:11
(Diff revision 2)
> +#context-navigation > .menuitem-iconic-right > .menu-iconic-right {
> +  -moz-appearance: none;
> +}

Let's keep this on the left for consistency like the origin mockups.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:451
(Diff revision 2)
> -  fillForm({ topDocument, loginFormOrigin, loginsFound, recipes }) {
> +  fillForm({ topDocument, loginFormOrigin, loginsFound, recipes, target }) {

Document the new attribute in the comment with a more descriptive name.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:468
(Diff revision 2)
> +      let form = target.form || FormLikeFactory.createFromPasswordField(target);

You shouldn''t need the \`target.form || \` part. We should always deal with FormLike instead of <form>

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:138
(Diff revision 2)
> +   * @param startDate
> +   *        The start timestamp of the period to return
> +   * @returns the timestamp in a text format of how old it is.
> +   * Example: 2 years old; 3 days old; 5 seconds old.
> +   *
> +   */
> +  _getPeriodString(startDate) {

I wish `Intl` APIs could handle this for us.
We talked about this in person and I kinda prefer leaving out "months" like Ryan had originally as it's not fine-grained enough. I also think we should show "1 year, 135 days" for > 1 year so users can distinguish password from the same year.

::: toolkit/modules/InlineSpellChecker.jsm:485
(Diff revision 2)
> +        if (!element.readOnly && element.type == "password") {
> +          flags |= this.PASSWORD;
> +        }

Can you check that this can never run when the field is disabled?

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:55
(Diff revisions 1 - 2)
> +# LOCALIZATION NOTE (loginMothsOld): Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 is the number of months passed
> +# example: 1 month old; 2 months old
> +loginMonthsOld = (#1 month old);(#1 months old)

Keep the moths away! :P

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:54
(Diff revision 2)
> +    foundLogins.forEach(login => {

Nit: forEach => for…of

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:21
(Diff revision 2)
> +  loginItems : [],
> +  contextMenu : null,

Nit: Extra spaces

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:23
(Diff revision 2)
> +  browser: null,

I'm worried about this leaking a <browser> after tab close. Also keep in mind that JSM state is shared but I guess we only have one context menu open at a time.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:84
(Diff revision 2)
> +    return Services.logins.findLogins({}, this.documentURI.prePath, "",
> +                                      null);

You can wrap to 100 instead of 80 in any password manager code if it makes things clearer.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:109
(Diff revision 2)
> +          var bunService = Cc["@mozilla.org/intl/stringbundle;1"].

`let` is the new `var`. But also: Services.strings.createBundle

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:107
(Diff revision 2)
> +  get _strBundle() {
> +      if (!this.__strBundle) {

You can use XPCOMUtils.defineLazyGetter for this.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:64
(Diff revision 2)
> +        item.setAttribute("value", login.password);

I think this is leftover.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:59
(Diff revision 2)
> +        if (!username) {
> +          let meta = login.QueryInterface(Ci.nsILoginMetaInfo);
> +          username = login.hostname + " " + this._getPeriodString(meta.timePasswordChanged);
> +        }

This pattern of `hostname + " "` shouldn't be hardcoded and should use a string substitution in case the order or separator needs to be changed in some locales.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:40
(Diff revision 2)
> +   * @returns if we found logins for this page.
> +   */
> +  addLoginsToMenu({insertAfter, target, browser, docURI}) {

addLoginsToMenu should maybe just return an array instead to make unit testing easier?

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:68
(Diff revision 2)
> +        item.setAttribute("class", "context-logins");

Nit: This class is representing a login so the variable name should be singular IMO: "context-login"

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:75
(Diff revision 2)
> +  clearLoginsFromMenu() {
> +    for (let item of this.loginItems) {
> +      this.contextMenu.removeChild(item);

In general, prefer child.remove() instead of removeChild unless you need to be strict about the parent <=> relationship.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:54
(Diff revision 2)
> +    foundLogins.forEach(login => {
> +        let item = this.contextMenu.ownerDocument.createElement("menuitem");
> +        this.loginItems.push(item);

I hope it's not possible for loginItems to get out of sync…

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:183
(Diff revision 2)
> +  },
> +}

Nit: missing semicolon at the end of the object.

::: browser/themes/shared/contextmenu.inc.css:103
(Diff revision 2)
> +#fill-password-label {
> +  list-style-image: url("chrome://mozapps/skin/passwordmgr/[e-mail]");
>  }

\#fill-password-heading seems a bit more descriptive

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:26
(Diff revision 2)
> +  /*

/\*\* to start JSDoc comments on methods. /\* is fine for files since that's what MXR uses.

Comment 25

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/13113/#review11859

> I wonder if you can just do:
> `this.target,
> this.browser,`
> with the new syntax?

Apparently you can't.

> addLoginsToMenu should maybe just return an array instead to make unit testing easier?

I'm returning a `Document Fragment` now. Should be easier to test and also more efficient.

> I hope it's not possible for loginItems to get out of sync…

This is how the inline spellchecker manages it's items. I think it should be safe, I couldn't find any filed bug related to this.

> I wish `Intl` APIs could handle this for us.
> We talked about this in person and I kinda prefer leaving out "months" like Ryan had originally as it's not fine-grained enough. I also think we should show "1 year, 135 days" for > 1 year so users can distinguish password from the same year.

As discussed, I created a separate toolkit module for the relative dates helper, that counts from seconds to days. That should be fine-grained enough for the user without breaking it's experience.

> Can you check that this can never run when the field is disabled?

This runs when the field is disabled. But I think this part should run like this, because we are only checking if it's a password field. I can add another condition to the `showFillPassword` to check if it's enabled. The curent behavior is we can see the logins at the contextual menu, but it doesn't fill the disabled field.

Comment 26

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Fill password field from contextual menu logins list r?MattN

Comment 27

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13111/#review12285

For the future, it would have been good to keep the relative time module addition in a separate patch to ease review (before and after landing).

Comment 28

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/13111/#review12285

I can still split it into two separate patches, if it would prefer.

Comment 29

[Bernardo Rittmeyer [:rittme]]

, if you prefer.

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

https://reviewboard.mozilla.org/r/13113/#review12341

Since the patch was so big I got exhausted part way so I haven't looked closely at everything yet. I will re-review the next version.

::: browser/base/content/browser-context.inc:435
(Diff revision 3)
> +      <menuitem id="fill-password-header"
> +            label="&loginFillPasswordHeader.label;"
> +            class="menuitem-iconic"
> +            disabled="true"
> +            hidden="true"/>
> +      <menuitem id="fill-password-no-logins"
> +            label="&noLoginFound.label;"
> +            disabled="true"
> +            hidden="true"/>

Fix identation of attributes

::: browser/base/content/browser-context.inc:434
(Diff revision 3)
> +      <menuseparator id="fill-password-separator" hidden="true"/>

Should we put this in a different position? I prefer above Inspect Element as extensions go below it normally.

::: browser/base/content/browser-context.inc:440
(Diff revision 3)
> +      <menuitem id="fill-password-no-logins"
> +            label="&noLoginFound.label;"
> +            disabled="true"
> +            hidden="true"/>

Ryan is fine with implementing the original design with the submenu and I think having the Saved Logins entry point makes sense so I think we should do that.

::: browser/themes/shared/contextmenu.inc.css:99
(Diff revision 3)
> +#fill-password-header {
> +  list-style-image: url("chrome://mozapps/skin/passwordmgr/[e-mail]");
> +}

We should use the 1x version at 1x and this one for 1.1x+ (at least I think that's the magic number we're using now).

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:447
(Diff revision 3)
>     *          recipes:
>     *            Fill recipes transmitted together with the original message.
>     *        }
>     */
> -  fillForm({ topDocument, loginFormOrigin, loginsFound, recipes }) {
> +  fillForm({ topDocument, loginFormOrigin, loginsFound, recipes, inputElement }) {

Document this argument

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:20
(Diff revision 3)
> +  loginItems: [],

By moving to the submenu design, I don't think we need to keep this state anymore as we can just delete everything above a specific menuitem.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:27
(Diff revision 3)
> +   * @param browser
> +   *        The browser for the document the context menu was open on.
> +   * @returns a document fragment with all the items or null if no login was found.
> +   */
> +  addLoginsToMenu(inputElement, browser) {
> +    this.loginItems = [];
> +
> +    let foundLogins = this._findLogins(browser.currentURI);

As I warned before, this is incorrect as browser.currentURI isn't necessarily the same origin as the inputElement.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:109
(Diff revision 3)
> +    let seen = new Set;
> +    let duplicates = new Set;

Nit: I've never seen us use constructors without the `()` for no arguments before so please add them.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:47
(Diff revision 3)
> +        // If login is empty or duplicate we want to append a creation date to it.

s/duplicate/duplicated/

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:57
(Diff revision 3)
> +        item.setAttribute("class", "context-login");

You could also use this class to delete all of the logins in the menu instead of keeping state.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:131
(Diff revision 3)
> +      loginFormOrigin: browser.currentURI.prePath,

This is also an incorrect assumption as I don't think each frame has its own <browser>.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:151
(Diff revision 3)
> +  return Services.strings.
> +                  createBundle("chrome://passwordmgr/locale/passwordmgr.properties");

This is unusual indentation. Can you align the createBundle with the S from Services.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:15
(Diff revision 3)
> +add_task(function* test_initialize()
> +{

Nit: no newline before {

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:18
(Diff revision 3)
> +  registerCleanupFunction(function () {

Nit: () => {

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:122
(Diff revision 3)
> +function checkMenu(contextMenu)
> +{

Nit: no newline before {

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:125
(Diff revision 3)
> +  // Make an array of menuitems for easier comparison.
> +  let menuitems = Array.prototype.slice.call(contextMenu.getElementsByClassName("context-login"));

let menuitems = [...contextMenu.getElementsByClassName("context-login")];

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:128
(Diff revision 3)
> +  Assert.equal(menuitems.length, logins.length);
> +  Assert.ok(logins.every(l => menuitems.some(m => l.username == m.label)));

The last argument to the assertion methods (providing context) should normally be used so logging from the test is clearer.

::: toolkit/components/passwordmgr/test/browser/form_basic_1pw.html:25
(Diff revision 3)
> +<!-- Simple username and password form, prefile username and password -->

Typo: prefile

::: toolkit/components/passwordmgr/test/browser/form_basic_1pw.html:18
(Diff revision 3)
> +<!-- Simple username and password form, prefiled username -->

Typo: prefiled

::: toolkit/content/xul.css:377
(Diff revision 3)
> +menuitem.menuitem-iconic-right {
> +  -moz-binding: url("chrome://global/content/bindings/menu.xml#menuitem-iconic-right");
> +}
> +

Leftover code

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:17
(Diff revision 3)
> +add_task(function* test_initialize()
> +{

No newline

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:88
(Diff revision 3)
> +function compareLoginToItem(login, item) {
> +  let regexp = new RegExp("(" + login.username + "|" + login.hostname +
> +                          ")( \\((less than a second old|a second old|[0-9]+ seconds old)\\))?");
> +  return item.label.match(regexp);
> +}

Perhaps I missed it but I don't see a test that actually ensure a time appears for duplicates.

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:59
(Diff revision 3)
> +loginHostAge = %1$S (%2$S old)

Add a localization note about the substitutions

::: toolkit/modules/RelativeTimeHelper.jsm:1
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Shipping code is MPL2, not PD.

::: toolkit/modules/RelativeTimeHelper.jsm:6
(Diff revision 3)
> +this.EXPORTED_SYMBOLS = [ "RelativeTimeHelper"];

Inconsistent whitespace

::: toolkit/modules/RelativeTimeHelper.jsm:30
(Diff revision 3)
> +    const MINUTES_IN_MS = SECONDS_IN_MS*60;
> +    const HOURS_IN_MS = MINUTES_IN_MS*60;
> +    const DAYS_IN_MS = HOURS_IN_MS*24;

Spaces around operators please

::: toolkit/modules/RelativeTimeHelper.jsm:53
(Diff revision 3)
> +    if (!endDate) {
> +      endDate = Date.now();
> +    }

Put this in the argument list instead.
`…, endDate = Date.now()) {`

::: toolkit/modules/RelativeTimeHelper.jsm:61
(Diff revision 3)
> +        let timeInPeriod = Math.floor(timePassed/period.value);

Nit: spaces around operators

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:469
(Diff revision 3)
> +      this._fillForm(form, true, true, true, true, loginsFound, recipes);
> +      return;
> +    }
> +    this._fillForm(topState.loginFormForFill, true, true, true, true, loginsFound, recipes);

The 2 calls to _fillForm only differ by the first argument so we should just have a temporary variable for the first argument that changes if there is an inputElement.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:16
(Diff revision 3)
> +/**
> + * Password manager object for the browser contextual menu.
> + */
> +let LoginManagerContextMenu = {

Nit: This was fine as-is with one \* like /\* so it is used as the description on MXR dir. listings. (Ideally MXR would look for /\*\* but it doesn't)

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:25
(Diff revision 3)
> +   * @param inputElement
> +   *        The target input element of the context menu click.
> +   * @param browser

Nit: please specify types for the arguments:

@param \{HTMLInputElement\} inputElement
…
@param \{xul:browser\} browser

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:4
(Diff revision 3)
> +/**

/\*

Comment 31

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Password manager contextual menu password field manual fill. r?MattN

Comment 32

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Bug 433238 - Tests for the password manager contextual menu. r?MattN

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

https://reviewboard.mozilla.org/r/13113/#review12913

Mostly small issues remaining:

::: browser/base/content/nsContextMenu.js:73
(Diff revisions 3 - 4)
> -    LoginManagerContextMenu.clearLoginsFromMenu();
> +    let lastLoginItem = document.getElementById("fill-login-no-logins").previousElementSibling;
> +    LoginManagerContextMenu.clearLoginsFromMenu(lastLoginItem);

This logic should be encapsulated inside `clearLoginsFromMenu` IMO.

::: browser/base/content/nsContextMenu.js:529
(Diff revisions 3 - 4)
> -    this.showItem("fill-password-no-logins", !fragment);
> +  savedPasswords: function() {

function are normally better with actions for names: How about calling this openPasswordManager too?

::: browser/base/content/nsContextMenu.js:530
(Diff revisions 3 - 4)
> +    LoginHelper.openPasswordManager(gContextMenuContentData.documentURIObject.hostPort, window);

You forgot to switch the argument order

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:460
(Diff revisions 3 - 4)
> -    if (LoginUtils._getPasswordOrigin(topDocument.documentURI) !=
> -        loginFormOrigin) {
> +    if (LoginUtils._getPasswordOrigin(topDocument.documentURI) != loginFormOrigin && (inputElement &&
> +        LoginUtils._getPasswordOrigin(inputElement.ownerDocument.documentURI) != loginFormOrigin)) {

I think this is incorrect as I think we'll now only log+return if inputElement is truthy.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:29
(Diff revisions 3 - 4)
> -   * @returns a document fragment with all the items or null if no login was found.
> +   * @param {Document}
> +   *        The context menu target document.

Please elaborate to explain the potential origin mixup that can happen if the browser's document is assumed to be the same as the document the click is in. e.g.
"The document that the context menu was activated from. This isn't the same as the browser's top-level document when subframes are involved."

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:31
(Diff revisions 3 - 4)
> +   * @returns {DocumentFragment} a document fragment with all the login items an item.

"with all the login items an item."

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:47
(Diff revisions 3 - 4)
> -        // If login is empty or duplicate we want to append a creation date to it.
> +        // If login is empty or duplicated we want to append a creation date to it.

Nit: s/creation/modification/

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:57
(Diff revisions 3 - 4)
> -        item.setAttribute("class", "context-login");
> +        item.setAttribute("class", "context-login-item");

I think this is unused now.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:59
(Diff revisions 3 - 4)
> -        // login variable is bound so we can keep the reference to each object after the loop.
> +        // login variable is bound so we can keep the reference to each object.

Nit: // `login` is bound…

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:128
(Diff revisions 3 - 4)
> -   * @param browser
> -   *        The document browser.
> +   * @param {xul:browser} browser
> +   *        The target tab browser.
>     */
> -  _fillPassword(login, inputElement, browser) {
> +  _fillPassword(login, inputElement, browser, documentURI) {

Forgot to add documentURI to the docs. Add a similar note here to clarify why we need both `browser` and `documentURI`.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:143
(Diff revisions 3 - 4)
> +   * @param {Array}

* @param {Array} formatArgs

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:146
(Diff revisions 3 - 4)
> +   * @returns {string} the localized string for the specified key,
>     * formatted with arguments if required.

The 2nd line should be aligned with "the" or "{string}"

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:151
(Diff revisions 3 - 4)
>        return this._stringBundle.formatStringFromName(key, formatArgs, formatArgs.length);
>      } else {

`else` after `return` isn't necessary. Remove the `else`.

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:30
(Diff revisions 3 - 4)
> -<!ENTITY loginFillPasswordHeader.label "Fill Password">
> +<!ENTITY fillLoginHeader.label "Fill Login">

Nit: no longer a header with the updated design.

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:31
(Diff revisions 3 - 4)
> -<!ENTITY noLoginFound.label "No saved logins">
> +<!ENTITY noLoginFound.label "No saved passwords">

I think we should leave this as "logins" to be consistent.

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:32
(Diff revisions 3 - 4)
> +<!ENTITY savedPasswords.label "Saved Passwords">

I wonder if this should be "Saved Logins" now that we're switching everything to "Logins". e.g. bug 1144856

Comment 34

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13113/#review12913

> I think this is unused now.

I see it's used in tests so I think it's fine. It also allows for easier styling/extensibility.

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

https://reviewboard.mozilla.org/r/14229/#review12915

::: toolkit/components/passwordmgr/test/browser/browser.ini:12
(Diff revision 1)
> +[browser_passwordmgr_context_menu.js]

"passwordmgr" in this context is referring to the manager interface. This test isn't related to that so "passwordmgr" should be removed.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: This should be public domain (which it gets by default now if you leave it out).

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:22
(Diff revision 1)
> +  registerCleanupFunction(() => {
> +    Services.prefs.clearUserPref("signon.autofillForms");
> +  });

Include `Services.logins.removeAllLogins();`?

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:34
(Diff revision 1)
> +add_task(function * test_context_menu_populate() {

Nit: no space before the `*` on any of these task generator functions.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:75
(Diff revision 1)
> +    }

Nit: always leave a trailing comma on arrays and object literals so blame doesn't change on additions.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:111
(Diff revision 1)
> +    });

Shouldn't this line be outdented by 2 to align with `yield`?

::: toolkit/components/passwordmgr/test/browser/browser.ini:5
(Diff revision 1)
> +  form_basic_1pw.html

Do we do the right thing if there are 2 password fields? e.g. if I have a text input followed by 2 password inputs and I use the context menu on the 2nd password field what happens:
a) If the first 2 inputs are empty
b) If the first password input is already filled
c) If the first 2 inputs are filled

We should add tests for these cases if we handle them correctly. If we don't handle them correctly, we can consider doing this in a follow-up depending on the answers.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:99
(Diff revision 1)
> +        Assert.equal(login.password , passwordInput.value, "Password filled and correct.");

Nit: extra space

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:40
(Diff revision 1)
> +      let passwordInput = browser.contentWindow.document.getElementById("test-password-1");

I guess this is fine for a test assuming it passes in e10s. ContentTask.spawn is the correct way to do this. You would need to change how openPasswordContextMenu works for that.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:115
(Diff revision 1)
> + * Check if the password field is correctly filled when it's on an iframe.

s/on/in/

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:123
(Diff revision 1)
> +    let iframe = browser.contentWindow.document.getElementById("test-iframe");

You should also test a cross-origin iframe. You can use https://example.org to load the same file in another test. If the body is the same them you can have a loop in this test and set the frame src so you don't have to duplicate all the logic. 

https://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt#103

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_context_menu.js:228
(Diff revision 1)
> +      hostname: "https://example.com",
> +      formSubmitURL: "https://example.com",

Change one of the logins to http to test that it still shows?

Comment 36

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Comment 37

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Comment 38

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/13113/#review13355

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:473
(Diff revision 5)
> +      // When bug 1164469 is landed we should have only one call for _fillForm.

I should probably file a follow up bug for this and reference it here. But I want to make sure we want to follow this path before filing it.

Comment 39

[Matthew N. [:MattN]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

https://reviewboard.mozilla.org/r/13113/#review13367

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:31
(Diff revisions 4 - 5)
> -<!ENTITY noLoginFound.label "No saved passwords">
> +<!ENTITY noPasswordFound.label "No saved password">

Was it intentional that this no longer matches the spec: "No saved passwords"?

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:57
(Diff revisions 4 - 5)
>  loginsSpielAll=Passwords for the following sites are stored on your computer:
>  loginsSpielFiltered=The following passwords match your search:
>  # LOCALIZATION NOTE (loginHostAge):
>  # This is used to show the context menu login items with their age.
>  # 1st string is the username for the login, 2nd is the login's age.
>  loginHostAge = %1$S (%2$S)
> +# LOCALIZATION NOTE (noUsername):
> +# String is used on the contexe menu, when a login doesn't have a username.
> +noUsername = No username

Be consistent about the spacing around `=`. I guess it should be removed?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:472
(Diff revisions 4 - 5)
>        form = FormLikeFactory.createFromPasswordField(inputElement);
> +      // When bug 1164469 is landed we should have only one call for _fillForm.
> +      this._fillForm(form, true, false, true, true, loginsFound, recipes, inputElement);
> +      return;
>      }
>      this._fillForm(form, true, true, true, true, loginsFound, recipes);

Why don't you do like `form` and just make a temporary variable for `clobberUsername` and `inputElement` arguments? Then we can still have one call to fillForm and the call site is more readable for the `clobberUsername` argument.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:873
(Diff revisions 4 - 5)
> +      if (inputElement &&
> +          inputElement.type == "password") {

Fix wrapping.

Actually I think we shouldn't go into unexpected behaviour if a non-password input is used:

`
if (inputElement) {
  if (inputElement.type != "password") {
    throw new Error("Unexpected input element type");
  }
  passwordField = inputElement;
  usernameField = null;
}
`

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:93
(Diff revisions 4 - 5)
> -   * @returns {Array} a login list
> +   * @returns {nsLoginInfo[]} a login list

nsILoginInfo[] (the interface name is what you want)

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:126
(Diff revisions 4 - 5)
> -   * @param {Array} loginList
> +   * @param {nsLoginInfo[]} loginList

nsILoginInfo[]

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:100
(Diff revisions 4 - 5)
> +      // Forces empty logins to be at the end
> +      if (!loginA.username) {
> +        return 1;
> +      }
> +      if (!loginB.username) {
> +        return -1;
> +      }

I think if both A and B are empty then we won't also sort by date and the return value will be incorrect.

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:32
(Diff revisions 4 - 5)
> -<!ENTITY savedPasswords.label "Saved Passwords">
> +<!ENTITY savedPasswords.label "Saved Password">

I thought we were sticking with Saved Logins?

If not, put the trailing "s" back on this unless the spec is outdated.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:460
(Diff revisions 4 - 5)
> -    if (LoginUtils._getPasswordOrigin(topDocument.documentURI) != loginFormOrigin && (inputElement &&
> +    if (LoginUtils._getPasswordOrigin(topDocument.documentURI) != loginFormOrigin && (!inputElement ||
>          LoginUtils._getPasswordOrigin(inputElement.ownerDocument.documentURI) != loginFormOrigin)) {

I think you just needed to switch the first `&&` to `||` before. I think what you have now might be correct but your extra negation makes it harder to read IMO.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:830
(Diff revisions 4 - 5)
>    _fillForm : function (form, autofillForm, clobberUsername, clobberPassword,
> -                        userTriggered, foundLogins, recipes) {
> +                        userTriggered, foundLogins, recipes, inputElement) {

I like to show that arguments are optional in the definition (as well as the JSDoc) by giving a default value e.g. `inputElement = null`.

You could have also started the conversion to an object by adding an options argument now instead of `inputElement`. The existing bug could just change to converting more optional arguments.

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:64
(Diff revisions 4 - 5)
> +# String is used on the contexe menu, when a login doesn't have a username.

Typo: contexe
Nit: I could be wrong but I don't think a comma is appropriate

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

https://reviewboard.mozilla.org/r/14229/#review13373

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:83
(Diff revisions 1 - 2)
> +    document: document,

Nit: `document,` is sufficient

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:81
(Diff revisions 1 - 2)
> -  return LoginManagerContextMenu.addLoginsToMenu(inputElement, browser, URI);
> +  return {
> +    fragment: LoginManagerContextMenu.addLoginsToMenu(inputElement, browser, URI),
> +    document: document,
> +  }

Nit: alphabetical

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:119
(Diff revisions 1 - 2)
> -    if ((login.username && !items.find(item => item.label == login.username + " (" + time + ")")) ||
> -        (!login.username && !items.find(item => item.label == login.hostname + " (" + time + ")"))) {
> +    if ((login.username &&
> +         !items.find(item => item.label == login.username + " (" + time + ")")) ||
> +        (!login.username &&
> +         !items.find(item => item.label == _stringBundle.GetStringFromName("noUsername") + " (" + time + ")"))) {
>        return false;
>      }

Nit: Can you split these into 2 conditions for readability?

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:222
(Diff revision 2)
> +  let menuitems = [... contextMenu.getElementsByClassName("context-login-item")];

Nit: remove the space after ...

Comment 41

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/13113/#review13367

> I think you just needed to switch the first `&&` to `||` before. I think what you have now might be correct but your extra negation makes it harder to read IMO.

I'm not sure this is the case here. If we use an ||, every time the first condition is met we will return. That's not exactly what we want. We should only return when neither of the origins (topDocument and inputElement) match the loginFormOrigin.

Our condition would be something like that: http://www.wolframalpha.com/input/?i=%28A+AND+NOT+B%29+OR+%28A+AND+B+AND+C%29 and that's the most readable one I found. I could split it into two different if conditions to make it more readable, but I don't know if it's a good option. What do you think?

> Be consistent about the spacing around `=`. I guess it should be removed?

I counted the occurences of both on this file and using spaces is more common. But since all the newer ones don't have spaces, I agree it makes more sense to remove them.

Comment 42

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Comment 43

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Comment 44

[Matthew N. [:MattN]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

https://reviewboard.mozilla.org/r/13113/#review13503

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:31
(Diff revisions 5 - 6)
> -<!ENTITY noPasswordFound.label "No saved password">
> +<!ENTITY noLoginFound.label "No saved logins">

Nit: Can you rename the entity name so it matches the string?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:472
(Diff revision 6)
> +    let options = {inputElement};

Nit: `{
  inputElement,
};`

so it's more readable and blame doesn't change when adding options (which doesn't seem unlikely).

Comment 45

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13113/#review13505

::: browser/base/content/browser-context.inc:421
(Diff revision 6)
> +      <menu id="fill-login"
> +            label="&fillPasswordMenu.label;"
> +            class="menu-iconic"
> +            hidden="true">
> +          <menupopup id="fill-login-popup">
> +            <menuitem id="fill-login-no-logins"

Indentation seems off. Mix of 2 and 4 spaces.

::: browser/base/content/nsContextMenu.js:509
(Diff revision 6)
> +    let showFillPassword = this.onPassword && Services.logins.isLoggedIn && !this.target.disabled;
> +    this.showItem("fill-login-separator", showFillPassword);
> +    this.showItem("fill-login", showFillPassword);

I think it may make more sense for fill-login to be disabled if `this.target.disabled`

::: browser/base/content/nsContextMenu.js:521
(Diff revision 6)
> +    if (fragment) {
> +      let popup = document.getElementById("fill-login-popup");

early return `if (!fragment) {`

::: browser/base/content/nsContextMenu.js:529
(Diff revision 6)
> +    LoginHelper.openPasswordManager(window, gContextMenuContentData.documentURIObject.hostPort);

I think we should use hostName to be consistent with the other consumer of this API (especially since your patch is currently doing strict origin matching for the menu items for now IIUC). Apologies if I told you differently before.

::: toolkit/components/passwordmgr/moz.build:42
(Diff revision 6)
>  EXTRA_JS_MODULES += [
>      'InsecurePasswordUtils.jsm',
>      'LoginHelper.jsm',
>      'LoginManagerContent.jsm',
> +    'LoginManagerContextMenu.jsm',

Put this in a `if CONFIG['MOZ_BUILD_APP'] == 'browser':` section so it doesn't get packaged on other apps not using it.

Could you also move `LoginDoorhangers.jsm` to that section since I realized I got this a bit wrong the other day with Riadh since I wasn't thinking about B2G/TB?

::: toolkit/modules/InlineSpellChecker.jsm:485
(Diff revision 6)
> +        if (!element.readOnly && element.type == "password") {
> +          flags |= this.PASSWORD;

I think the `readOnly` check would also be better with `disabled` in initPasswordManagerItems and the menu item should be disabled. That way we consistently show the menu item on all password fields where I think people would expect it.

Comment 46

[Matthew N. [:MattN]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

https://reviewboard.mozilla.org/r/14229/#review13507

Ship It!

Comment 47

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13113/#review13509

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:96
(Diff revision 6)
> +    let logins = Services.logins.findLogins({}, documentURI.prePath, "", null);

Hmm… I wonder if we should pass "" for `aHttpRealm` instead of `null`. I know some sites like adp.com have switched from HTTP auth for form auth and this would help. I guess we would need to make sure we handle capture properly but if we handle that then we get an easy improvement.

Comment 48

[Bill Gianopoulos [:WG9s]]

Hey this was my bug, and I hope you all realize that the original issue I complained about is already fixed.  I am impressed you are going the extra mile to make sure it is really doing the right thing in all cases.

Comment 49

[Matthew N. [:MattN]]

(In reply to Bill Gianopoulos [:WG9s] from comment #48)
> Hey this was my bug, and I hope you all realize that the original issue I
> complained about is already fixed.

Hello, there are some cases where it's not fixed and that's what this fix should address.
Consider a form without a username field (and therefore autocomplete):
* used in private browsing mode (where autofill is disabled for privacy reasons).
* when there are multiple matching logins for the site – we don't auto-fill since we don't know which one the user wants.

I may be missing another.

> I am impressed you are going the extra mile to make sure it is really doing the right thing in all cases.

Thanks!

Comment 50

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13113/#review13511

Great work on this! It's working well and the follow-ups will be much easier on this foundation.

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:31
(Diff revision 6)
> +<!ENTITY noLoginFound.label "No saved logins">
> +<!ENTITY savedLogins.label "Saved Logins">

"No saved logins" feels weird to me above "Saved Logins". It's like we're saying we have no saved logins but you can check yourself if you don't believe us. Maybe we should say "No matching logins" or something?

http://grab.by/JmgK

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:30
(Diff revision 6)
> +<!ENTITY fillPasswordMenu.label "Fill Password">
> +<!ENTITY noLoginFound.label "No saved logins">

I think we should do like "(No Spelling Suggestions)" and use title case (like all other menu items I see) and probably put it in braces for consistency.

Comment 51

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13113/#review13523

::: browser/base/content/nsContextMenu.js:508
(Diff revision 6)
> +  initPasswordManagerItems: function() {
> +    let showFillPassword = this.onPassword && Services.logins.isLoggedIn && !this.target.disabled;

Thinking about this more, !Services.logins.isLoggedIn should also just disable.

Comment 52

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Comment 53

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Comment 54

[Bernardo Rittmeyer [:rittme]]

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

Comment 55

[Matthew N. [:MattN]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

https://reviewboard.mozilla.org/r/13113/#review13593

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:472
(Diff revisions 6 - 7)
> -    let options = {inputElement};
> +    let options = { inputElement, };

Maybe you weren't reading the rich version but I mean with inputElenent, on its own line.

::: toolkit/locales/en-US/chrome/global/textcontext.dtd:31
(Diff revisions 6 - 7)
> -<!ENTITY noLoginFound.label "No saved logins">
> +<!ENTITY noSavedLogin.label "(No Matching Logins)">

Please update the entity name when changing the string.

Comment 56

[Matthew N. [:MattN]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

https://reviewboard.mozilla.org/r/14229/#review13595

Ship It!

Comment 57

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

Comment 58

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Comment 59

[Bernardo Rittmeyer [:rittme]]

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

Comment 60

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

Comment 61

[Bernardo Rittmeyer [:rittme]]

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

Comment 62

[Matthew N. [:MattN]]

Comment on attachment 8632600 [details]
MozReview Request: Bug 433238 - Password manager contextual menu password field manual fill. r=MattN

https://reviewboard.mozilla.org/r/13113/#review13805

Ship It!

Comment 63

[Matthew N. [:MattN]]

Comment on attachment 8639709 [details]
MozReview Request: Bug 433238 - Tests for the password manager contextual menu password fill. r=MattN

https://reviewboard.mozilla.org/r/14229/#review13807

Ship It!

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/02ff02df924a
https://hg.mozilla.org/integration/fx-team/rev/96f128a9b465

Comment 65

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/17a83ec9321b

Comment 66

[Pulsebot]

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

Comment 67

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

https://hg.mozilla.org/mozilla-central/rev/02ff02df924a
https://hg.mozilla.org/mozilla-central/rev/96f128a9b465
https://hg.mozilla.org/mozilla-central/rev/a5bde89f6829

Comment 68

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/17a83ec9321b

Comment 69

[Daniel Bodea [:danibodea]]

Hi Galvin. Considering you need this fix validated, can you help me with a test case?

Comment 70

[Matthew N. [:MattN]]

Go to any webpage with only a password field (e.g. https://mail.mozilla.org/admin/passwords-dev ) and save multiple logins for the site (use different usernames). Then reload the page and use the Fill Login context menu to fill the password field.

Comment 71

[Daniel Bodea [:danibodea]]

The test case you gave me unveils a new bug: You cannot save multiple credentials on a website that only saves passwords because the website will constantly prompt to Update the first saved password: bug 1552759.
Furthermore, if you go to the Saved Logins modal, you will notice that you cannot manually add a new credential to the list.

This being said, the fix for this bug cannot be verified by this method.

An improvised way this could be verified is by getting a login form test page with both fields and with autocomplete=off (as mentioned in comment 0), saving a few forms with both a username and a password and one form with password only. After reloading the page, the Password Manager should NOT auto-fill since there are multiple saved logins and should fill as intended when manually filling by password manager suggestions.

@Mattn: Considering that the expected results have changed since this issue was logged, I need confirmation that this fix is correctly verified. Thank you.

Comment 72

[Matthew N. [:MattN]]

(In reply to Bodea Daniel [:danibodea] from comment #71)

The test case you gave me unveils a new bug: You cannot save multiple credentials on a website that only saves passwords because the website will constantly prompt to Update the first saved password: bug 1552759.

That's expected.

Furthermore, if you go to the Saved Logins modal, you will notice that you cannot manually add a new credential to the list.

Right, only the new about:logins has that ability.

This being said, the fix for this bug cannot be verified by this method.

My test page was sufficient to verify the bug, you just need to add usernames to both login doorhangers before saving (as I hinted at).

An improvised way this could be verified is by getting a login form test page with both fields and with autocomplete=off (as mentioned in comment 0), saving a few forms with both a username and a password and one form with password only. After reloading the page, the Password Manager should NOT auto-fill since there are multiple saved logins and should fill as intended when manually filling by password manager suggestions.

@Mattn: Considering that the expected results have changed since this issue was logged, I need confirmation that this fix is correctly verified. Thank you.

If you were testing from the context menu, not autocomplete, then that would be a proper verification but you didn't specify which UI you used when you said "filling by password manager suggestions".

Comment 73

[Daniel Bodea [:danibodea]]

I have initially verified the fix by left-clicking in the password field and filling them by the Saved Logins drop-down (I assume this is "autocomplete") to make sure that all work as intended.
Afterward, I have also verified by the context menu (right-clicking in the password field, selecting "Fill Password" and then a password that has a username and then, the password that does not. Considering all of the above, I deem this bug verified in Nightly v69.0a1 (2019-05-23) (64-bit).

Unfortunately, another issue has come to the surface: when reaching this test site (https://mail.mozilla.org/admin/passwords-dev) while the browser has multiple saved logins with a username and a login without a username, the password manager will automatically auto-fill the field with the password that does not have a username. Is this intended or should I log it?

Comment 74

[Matthew N. [:MattN]]

(In reply to Bodea Daniel [:danibodea] from comment #73)

Unfortunately, another issue has come to the surface: when reaching this test site (https://mail.mozilla.org/admin/passwords-dev) while the browser has multiple saved logins with a username and a login without a username, the password manager will automatically auto-fill the field with the password that does not have a username. Is this intended or should I log it?

That is intentional.