Pages can use JS to handle logins instead of using form submission events and we should try to capture the saved values in those cases. We can likely use pagehide or pageshow events for this but we should consider some edge cases: * Do we want to capture if the user closes a tab? What about the page doing window.close (e.g. Persona)? These may fire pagehide? * Do we want to capture if the users navigates back/forward with typed text? What about the page navigating back/forward? * We probably don't want to cause a second capture doorhanger to appear if the values were already captured via onbeforesubmit. We probably still want onbeforesubmit around to handle a submission with event.preventDefault() (no pagehide).

Bug 1166947 - WIP: Run login capture code upon page navigation if there are password fields present

Comment on attachment 8627686 [details] Bug 1166947 - Run login capture code upon page navigation if there are password fields present. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/12263/diff/1-2/

Comment on attachment 8627686 [details] Bug 1166947 - Run login capture code upon page navigation if there are password fields present. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/12263/diff/2-3/

Comment on attachment 8627686 [details] Bug 1166947 - Run login capture code upon page navigation if there are password fields present. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/12263/diff/3-4/

I'm still working on browser_formless_submit_chrome.js but that is pretty straightforward and wanted to get your eyes on the code changes sooner.

Comment on attachment 8627686 [details] Bug 1166947 - Run login capture code upon page navigation if there are password fields present. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/12263/diff/4-5/

browser_formless_submit_chrome.js is now finished so there are no remaining TODOs and this is ready for review.

https://reviewboard.mozilla.org/r/12263/#review57966 ::: modules/libpref/init/all.js:4279 (Diff revision 5) > # AIX > #endif > > // Login Manager prefs > pref("signon.rememberSignons", true); > +pref("signon.rememberSignons.formless", true); I'd suggest giving this a name independent of rememberSignons, so there's no confusion about what happens if signon.rememberSignons is |false| but the latter is true. signon.formless.enabled? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:356 (Diff revision 5) > > + try { > + this.setupProgressListener(window); > + } catch (ex) { > + // Ignore NS_ERROR_FAILURE if the progress listener was already added > + } Move the try/catch into setupProgressListener? Caller doesn't need to care about this. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:415 (Diff revision 5) > // Always record the most recently added form with a password field. > - this.stateForDocument(form.ownerDocument).loginForm = form; > + let state = this.stateForDocument(form.ownerDocument); > + if (!state.loginFormRootElements) { > + state.loginFormRootElements = new Set(); > + } > + state.loginFormRootElements.add(form.rootElement); What is loginFormRootElements? Can't stateForDocument() handle creating the Set()? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:434 (Diff revision 5) > .then(null, Cu.reportError); > }, > > onPageShow(event, window) { > + if (window.location == "about:blank") > + return; Why is this needed now? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:474 (Diff revision 5) > > // Returns the first known loginForm present in this window or in any > // same-origin subframes. Returns null if no loginForm is currently present. > let getFirstLoginForm = thisWindow => { > - let loginForm = this.stateForDocument(thisWindow.document).loginForm; > - if (loginForm) { > + let loginForms = this.stateForDocument(thisWindow.document).loginFormRootElements; > + if (loginForms && [...loginForms][0]) { if (loginForms && loginForms.size) I'm a little unclear on what "getFirstLoginForm" means if we're randomly picking one from this new Set... ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:495 (Diff revision 5) > > // Returns true if this window or any subframes have insecure login forms. > let hasInsecureLoginForms = (thisWindow, parentIsInsecure) => { > let doc = thisWindow.document; > let isInsecure = parentIsInsecure || !this.isDocumentSecure(doc); > - let hasLoginForm = !!this.stateForDocument(doc).loginForm; > + let hasLoginForm = !![...(this.stateForDocument(doc).loginFormRootElements || [])][0]; This... really isn't readable. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:813 (Diff revision 5) > > log("Password field (new) id/name is: ", newPasswordField.id, " / ", newPasswordField.name); > - if (oldPasswordField) > + if (oldPasswordField) { > log("Password field (old) id/name is: ", oldPasswordField.id, " / ", oldPasswordField.name); > + } else { > + log("Password field (old):", oldPasswordField); I don't understand this. You're adding an |else| when oldPasswordField is falsy?

https://reviewboard.mozilla.org/r/12263/#review57966 > I'd suggest giving this a name independent of rememberSignons, so there's no confusion about what happens if signon.rememberSignons is |false| but the latter is true. > > signon.formless.enabled? I used "signon.formlessCapture.enabled" instead since formless fill is already shipping so this only controls capture (which is why I put it under "rememberSignons"). > What is loginFormRootElements? Can't stateForDocument() handle creating the Set()? > What is loginFormRootElements? It's basically a reference to all of the login forms in a document so that we don't have to go searching for them upon page navigation and possibly re-calculating what the root of a formless FormLike is. These are root elements of the form instead of FormLike objects themselves so that we can get the benefits of a `Set` since two FormLike created for the same "form" won't return the same JS object. > Can't stateForDocument() handle creating the Set()? Yeah, I did this now for `oginFormRootElements` but I'm not sure why this wasn't done before. > Why is this needed now? Sorry, it's not related to this bug so I shouldn't have bundled it in the same commit, it's just comething that has bothered me for a while. Basically we a console warning for each tab if signon.debug is on due to trying to parse about:blank and exract some property that isn't on about URIs. Do you want me to remove it? > if (loginForms && loginForms.size) > > I'm a little unclear on what "getFirstLoginForm" means if we're randomly picking one from this new Set... The behaviour after is equivalent to before since `Set`s are ordered: "you can iterate its elements in insertion order" from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set. i.e. I'm still returning the first login form found. As discussed before, I don't like using the `getFirstLoginForm` idea but luckily it's only used by the experimental fill doorhanger. > This... really isn't readable. I've improved this now. Let me know if you prefer something more verbose yet. > I don't understand this. You're adding an |else| when oldPasswordField is falsy? Yeah, I think the thought was that it would allow distinguishing between falsy values e.g. `null` vs. `undefined` but I'm not sure if there will ever be more than one falsy value in practice. I can remove the `else` block if you prefer.

Comment on attachment 8627686 [details] Bug 1166947 - Run login capture code upon page navigation if there are password fields present. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/12263/diff/5-6/

Comment on attachment 8627686 [details] Bug 1166947 - Run login capture code upon page navigation if there are password fields present. https://reviewboard.mozilla.org/r/12263/#review58504 Ugh. Sorry this took so long. :( It's pretty painful to swap in context for everything that's going on. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:74 (Diff revision 6) > gEnabled = Services.prefs.getBoolPref("signon.rememberSignons"); > gAutofillForms = Services.prefs.getBoolPref("signon.autofillForms"); > gStoreWhenAutocompleteOff = Services.prefs.getBoolPref("signon.storeWhenAutocompleteOff"); > }, > + > + // nsIWebProgressListener The code here seems sensible enough, although the complexity of webprogresslistener makes me feel like I probably don't understand it well enough to know is there's anything missing. :/ I'm going to r+ anyway, as worst case is an extra/missing onNavigation call -- just a harmless bug to fix. But if you want more detailed feedback/review, ask someone who's more actively using this stuff? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:412 (Diff revision 6) > - this.stateForDocument(form.ownerDocument).loginForm = form; > + let state = this.stateForDocument(form.ownerDocument); > + state.loginFormRootElements.add(form.rootElement); > + log("adding", form.rootElement, "to loginFormRootElements for", form.ownerDocument); Can we just move this section (adding the rootElement to the state) into FormLikeFactory.createFrom[Field|Form]? It might seem like a minor thing, but I keep tripping over this. As-is, it's unclear why we're suddenly deciding right here to stash this into the state object, when it seems unrelated to the task at hand (geting logins to fill the form). I think it would make a lot more sense in the context of "hey, we're creating a formlike, and that's a thing we want to track as part of the page state". ...in fact, this is exactly what's being done with the LoginManagerContent._formLikeByRootElement.set() call that you're adding in this patch. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:431 (Diff revision 6) > .then(this.loginsFound.bind(this)) > .then(null, Cu.reportError); > }, > > onPageShow(event, window) { > + if (window.location == "about:blank") Maybe just add a comment here noting what you said in comment 9? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:473 (Diff revision 6) > - let loginForm = this.stateForDocument(thisWindow.document).loginForm; > - if (loginForm) { > - return loginForm; > + let loginForms = this.stateForDocument(thisWindow.document).loginFormRootElements; > + if (loginForms && [...loginForms][0]) { > + return [...loginForms][0]; I think loginForms will always be non-null now (since the Set is created on init)? Better to check for |loginForms.size| instead of creating |[...loginForms][0]| just to throw it away (and then create it again to return it). IE, just: if (loginForms.size) return [...loginForms][0]; ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:495 (Diff revision 6) > - let hasLoginForm = !!this.stateForDocument(doc).loginForm; > + let loginFormRoots = [...(this.stateForDocument(doc).loginFormRootElements || [])]; > + let hasLoginForm = !!loginFormRoots[0]; I don't really find this any more readable than the previous patch. :) It's compact, but the problem is it takes too long to decipher. Luckly this can also be simplified by loginFormRootElements always being non-null now. Further, if you're just checking to see if loginFormRoots[0] exists, can't this whole thing shrink down to: let hasLoginForm = this.stateForDocument(doc).loginFormRootElements.size > 0 ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:845 (Diff revision 6) > + if (loginFormRootElements.size == 0) { > + log("_onNavigation: No login form root elements for the navigating document"); > + return; > + } I don't object to keeping this block if you think it's useful, but I'd point out that you already log .size right above this, and the for-of below this will just be a nop if it's empty.

https://reviewboard.mozilla.org/r/12263/#review58504 > Can we just move this section (adding the rootElement to the state) into FormLikeFactory.createFrom[Field|Form]? > > It might seem like a minor thing, but I keep tripping over this. As-is, it's unclear why we're suddenly deciding right here to stash this into the state object, when it seems unrelated to the task at hand (geting logins to fill the form). I think it would make a lot more sense in the context of "hey, we're creating a formlike, and that's a thing we want to track as part of the page state". > > ...in fact, this is exactly what's being done with the LoginManagerContent._formLikeByRootElement.set() call that you're adding in this patch. Yeah, good idea. The comment explains that the old code just cared about the last one encountered so that's why it was done here. I don't think the ordering should matter so I will move this (x2) and remove the comment. > Maybe just add a comment here noting what you said in comment 9? I'm just going to remove this as I realized it may cause problems if a user opens about:blank > I don't really find this any more readable than the previous patch. :) It's compact, but the problem is it takes too long to decipher. > > Luckly this can also be simplified by loginFormRootElements always being non-null now. Further, if you're just checking to see if loginFormRoots[0] exists, can't this whole thing shrink down to: > > let hasLoginForm = this.stateForDocument(doc).loginFormRootElements.size > 0 Yep, good idea. At one point in my past revisions loginFormRootElements could be null. > I don't object to keeping this block if you think it's useful, but I'd point out that you already log .size right above this, and the for-of below this will just be a nop if it's empty. True, I removed the whole block

Comment on attachment 8627686 [details] Bug 1166947 - Run login capture code upon page navigation if there are password fields present. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/12263/diff/6-7/

Verification steps: 1) Load https://www.metlife.com 2) From the login form on the homepage, type a (fake) username and password 3) Click Log in Expected result with this patch: The doorhanger to save the password appears (even if the login fails [bug 558178]) Without this patch: The doorhanger wouldn't appear Similar steps can be repeated on any site where there is an <input type=password> which is not contained within a <form>. Some other examples: * hulu.com (from the homepage) but this requires a real account (free) * https://vnet.one/login (bug 1279417)

Other sites this fixes: * TP-Link router administration login

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/fx-team/rev/ad4e4ea6550a Run login capture code upon page navigation if there are password fields present outside a <form>. r=dolske

backed out for test failures like https://hg.mozilla.org/integration/fx-team/rev/59488654684d

This is passing now on https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5f719d807c66820e6353cc5c484eaccebeca453 with the fix from bug 1291060.

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/fx-team/rev/1e3c438085ec Run login capture code upon page navigation if there are password fields present. r=dolske

Verified fixed on hulu.com, vnet.one on FX 51.0a1 (2016-08-30) Win 7, Ubuntu 14.04, OS X 10.12.

Release Note Request (optional, but appreciated) [Why is this notable]: Firefox will now ask to save logins on sites which don't use a <form> submit event (which accounts for about 18% of all password fields: https://mzl.la/2dcxirY) [Suggested wording]: Support saving logins on up to 18% more pages (Please check with marketing about what kind of claims we want. I say "up to" since it's an upper bound) [Links (documentation, blog post, etc)]: None. I can put something on my personal blog if you would like.

Added to Fx51 (Aurora) release notes.

In the release notes of 51 with "Support saving passwords for forms without 'submit' events" as wording.