1311301 - Improve discoverability of login autocompletion autofocused inputs

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Jonathan Kingston [:jkt] he/him]

Following on from Bug 376668 sites that autofocus user fields on initial load don't always show their dropdown due to the event happening before the code is bound.

As a user I expect the autocomplete dropdown to be visible on gmail when I first visit the site and I have more than one account on that domain.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1311301 - making login fields show autocomplete on autofocused inputs

Review commit: https://reviewboard.mozilla.org/r/87632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87632/

Comment 2

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8803331 [details]
Bug 1311301 - making login fields show autocomplete on autofocused inputs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87632/diff/1-2/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8803331 [details]
Bug 1311301 - making login fields show autocomplete on autofocused inputs

https://reviewboard.mozilla.org/r/87632/#review87850

::: browser/base/content/content.js:89
(Diff revision 2)
> +  if (event.target instanceof Ci.nsIDOMHTMLInputElement) {
> +    AutoCompletePopup.closePopup();
> +  }

Is this still needed if you rebase on m-c? There were some fixes to ac popups recently related to closing.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1017
(Diff revision 2)
>  
>        // Attach autocomplete stuff to the username field, if we have
>        // one. This is normally used to select from multiple accounts,
>        // but even with one account we should refill if the user edits.
>        if (usernameField)
> -        this._formFillService.markAsLoginManagerField(usernameField);
> +        this._formFillService.markAsLoginManagerField(usernameField, userTriggered);

What is the `userTrigged` useful for? It sounds like this approach would fix bug 1120037 too if we don't consider userTriggered.

Comment 4

[Dale Harvey (:daleharvey)]

If I edit it to:

        nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(node);
        MaybeStartControllingInput(input);
        ShowPopup();

then https://bugzilla.mozilla.org/show_bug.cgi?id=1120037 is fixed, the popup is attached on load and editing the field shows it, the |MaybeStartControllingInput| seems required to set |mFocusedInput| which is relied a lot within nsFormFillController

However at least with the test case @ https://bug1120037.bmoattachments.org/attachment.cgi?id=8546961 and the above patch + ammendments I do not see the popup on load, only after editing the input

Comment 5

[Tanvi Vyas[:tanvi]]

Dale, any update here?

Comment 6

[Dale Harvey (:daleharvey)]

Hey, since there is a working patch I was figuring I would do other work and wait on https://bugzilla.mozilla.org/show_bug.cgi?id=376668 to land, however as its getting close to merge day I will do this in parallel, will get a patch in for review this evening

Comment 7

[Tanvi Vyas[:tanvi]]

(In reply to Dale Harvey (:daleharvey) from comment #6)
> Hey, since there is a working patch I was figuring I would do other work and
> wait on https://bugzilla.mozilla.org/show_bug.cgi?id=376668 to land, however
> as its getting close to merge day I will do this in parallel, will get a
> patch in for review this evening

Yes, please work in parallel.  You can view the latest patches on 376668 for reference if that helps, but that bug doesn't have to land first.  Especially because we are running out of days before merge.

Comment 8

[Dale Harvey (:daleharvey)]

As mentioned elsewhere that bug does have to land first, this is based on it however now there is a solution for the tests in https://bugzilla.mozilla.org/show_bug.cgi?id=376668, Have a working patch here just writing the tests nowand pushed to try to check if there is anything else I will need to look into fixing before review https://treeherder.mozilla.org/#/jobs?repo=try&revision=2768eb7513b80870567c5f62dd3566b6339ef818

Comment 10

[Dale Harvey (:daleharvey)]

Attachment: Ensure autocomplete is shown on autofocus

This is based on top of https://bug376668.bmoattachments.org/attachment.cgi?id=8810361

Rest seems self explanatory, if an input has received focus before the password manager had marked that as being controlled then the password autocomplete never got attached, now it checks if it already has focus when its marked and attaches and shows the autocomplete

Comment 11

[Dale Harvey (:daleharvey)]

Pushed to try in https://treeherder.mozilla.org/#/jobs?repo=try&revision=df6d488f7fabd14bc70072a8e53a515e65f7ed54

Comment 12

[Matthew N. [:MattN]]

There seems to be test failures on try and it would be easier to review with the two commits in one MozReview series.

Comment 13

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (partial PTO Nov. 3–11) from comment #12)
> There seems to be test failures on try and it would be easier to review with
> the two commits in one MozReview series.

Oh, sorry I thought that patch was from the bug that got duped. Now that I saw bug 376668 comment 103 I understand and you can leave the patches in the two bugs but please mark old patches as obsolete.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8810442 [details] [diff] [review]
Ensure autocomplete is shown on autofocus

Review of attachment 8810442 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: please clarify in the commit message that this is about login manager autocomplete, not other autocomplete.

r=me with nits

::: toolkit/components/passwordmgr/test/browser/browser_autofocus_autocomplete.js
@@ +20,5 @@
> +    username: "username2",
> +    password: "password2",
> +  });
> +  Services.logins.addLogin(login);
> +  yield SpecialPowers.pushPrefEnv({ "set": [["signon.autofillForms.http", false]] });

Hey Dale, could you clarify how this pushPrefEnv is relevant to the test/bug? I may be missing some context but I don't think it's related so perhaps its leftover code?

::: toolkit/components/passwordmgr/test/browser/form_autofocus.html
@@ +1,1 @@
> +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body onload="document.getElementById('form-basic-username').focus();">

Nit: Start the <body…> on a new line

@@ +1,3 @@
> +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body onload="document.getElementById('form-basic-username').focus();">
> +<!-- Any copyright is dedicated to the Public Domain.
> +   - http://creativecommons.org/publicdomain/zero/1.0/ -->

Just nuke the license since it's not needed in tests anymore since they're PD by default. It also makes it easier to notice the @onload.

@@ +1,5 @@
> +<!DOCTYPE html><html><head><meta charset="utf-8"></head><body onload="document.getElementById('form-basic-username').focus();">
> +<!-- Any copyright is dedicated to the Public Domain.
> +   - http://creativecommons.org/publicdomain/zero/1.0/ -->
> +
> +<!-- Simplest form with username and password fields. -->

Please update this comment to mention that the username field is autofocused via JS (in contrast to @autofocus).

On that same note can you rename the file to make it more clear which method of autofocusing is used. e.g. form_autofocus_js.html?

Comment 15

[Dale Harvey (:daleharvey)]

Attachment: Ensure login managed inputs focus on load

Now that the dep has landed, carrying review, amended the comments and pushing to try will land if its all looking green https://treeherder.mozilla.org/#/jobs?repo=try&revision=31662330c674347e01ecb810ffad5a171ebee2b5

Comment 16

[Dale Harvey (:daleharvey)]

Attachment: Ensure login managed inputs focus on load

So this has changed enough that it probably needs a rereview, The main changes are: 

I added a try catch in checkRowCount, as mentioned in the comments this would intermittently throw an exception for me, its used in other code however I believe since I am calling it directly on the child loading I think if its called too early it hasnt finished initialising and can throw, since its a waitFor it seemed safe to do so.

I added autocomplete=off to the input and added |if (!mFocusedInput && fm) {| this avoided a failure in http://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html#752

This means the fix is only valid for autocomplete=off inputs which is not ideal, the caching test is specifically testing against an already focused input receiving updated autocomplete information (which is what this bug is fixing)

I think we should remove the mFocusedInput check and the autocomplete=off and the failing cache test, we may want to replace the cache test in a way that doesnt break with this fix?

Comment 17

[Dale Harvey (:daleharvey)]

Attachment: Ensure login managed inputs focus on load

So there is no point in fixing this only for autocomplete=off inputs, the removed test is explicitly testing for the bug we are fixing. We could test for the cache a new way? but I would need some guidance for that.

Cheers

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8814770 [details] [diff] [review]
Ensure login managed inputs focus on load

Review of attachment 8814770 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. I looked at this quickly the other day but I don't understand why autocomplete=off is relevant at all since it should have no effect for login fields (username fields that are marked or any type=password, for example). If it is changing outcomes then that's a bug.

The caching test is also correct AFAICT so it seems like you're actually introducing a regression which I don't think is a good idea for perf though it's probably less of an issue nowadays with the in-memory JSON logins on desktop. It may still have a bigger impact on Android which uses SQLite but I'm not sure if that goes through this code.

TL;DR: I don't understand why you think it's fine to break caching and why autocomplete=off is relevant.

Comment 19

[Dale Harvey (:daleharvey)]

If an input is autocomplete=off when first focused but not yet registered by the password manager then nsFormFillController does not control it (http://searchfox.org/mozilla-central/source/toolkit/components/satchel/nsFormFillController.cpp#926) meaning when it is later marked as a password manager input we can start controlling it and provide the appropriate logins

However if it is not autocomplete=off then onload nsFormFillController will start controlling it and when we mark it as a login field then unless we break the cache then the input will continue to have no logins associated which is what the bug actually is, (I maybe should have kept this in the original bug @ https://bugzilla.mozilla.org/show_bug.cgi?id=1120037, I duped them since the solution was almost identical)

This will only break caching for inputs who get marked as handled by the login manager while they are currently focused, which I believe should be a rare event.

Comment 20

[Matthew N. [:MattN]]

(In reply to Dale Harvey (:daleharvey) from comment #19)
> If an input is autocomplete=off when first focused but not yet registered by
> the password manager then nsFormFillController does not control it
> (http://searchfox.org/mozilla-central/source/toolkit/components/satchel/
> nsFormFillController.cpp#926) meaning when it is later marked as a password
> manager input we can start controlling it and provide the appropriate logins
> 
> However if it is not autocomplete=off then onload nsFormFillController will
> start controlling it and when we mark it as a login field then unless we
> break the cache then the input will continue to have no logins associated
> which is what the bug actually is, (I maybe should have kept this in the
> original bug @ https://bugzilla.mozilla.org/show_bug.cgi?id=1120037, I duped
> them since the solution was almost identical)

Why would we have a cached result at that point? Is it for a different field (that would be a bug)?

This sounds related to the XXX at http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/toolkit/components/satchel/nsFormFillController.cpp#658-659

> This will only break caching for inputs who get marked as handled by the
> login manager while they are currently focused, which I believe should be a
> rare event.

What is actually breaking the cache? I worry that doing `MaybeStartControllingInput(input);` and `ShowPopup();` again will cause other side-effects and it seems a bit wrong to me.

Perhaps the test can be `hg copy`'d (moved) to a file with "signon.autofillForms.http" set to False so we can retain the test coverage?

Comment 21

[Dale Harvey (:daleharvey)]

Attachment: Ensure login managed inputs focus on load

You were right it wasnt needed to entirely teardown and resetup controlling the input again, we just need to do it in the cases that it wasnt picked up on the first focus so dont need to delete any tests, green run @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab4bf4abbb63fcbfc1d07870fecdace9dcd4d074

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8815458 [details] [diff] [review]
Ensure login managed inputs focus on load

Review of attachment 8815458 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks and sorry for the delay!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72cef09ee478
Ensure login managed inputs focus on load. r=mattn

Comment 24

[Tanvi Vyas[:tanvi]]

Dale, can you request this for aurora uplift after it makes it to central?

Comment 25

[Dale Harvey (:daleharvey)]

Will do, I was planning on giving it a day or 2 to make sure there was no fallout then request uplift

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8f99127624
Skip test on android, on a CLOSED TREE. r=philor

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/72cef09ee478
https://hg.mozilla.org/mozilla-central/rev/fd8f99127624

Comment 28

[Tanvi Vyas[:tanvi]]

Dale, please request aurora uplift.

Comment 29

[Dale Harvey (:daleharvey)]

Comment on attachment 8815458 [details] [diff] [review]
Ensure login managed inputs focus on load

Approval Request Comment
[Feature/Bug causing the regression]: Improvement in password manager UX
[User impact if declined]: 
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Not particularly
[Why is the change risky/not risky?]: Settled on beta, simple isolated patch
[String changes made/needed]:

Comment 30

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8815458 [details] [diff] [review]
Ensure login managed inputs focus on load

login form autocomplete fix for aurora52

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/732bd3ae0bd3
https://hg.mozilla.org/releases/mozilla-aurora/rev/9dbc06789272

Comment 32

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Comment 29.