Improve discoverability of login autocompletion autofocused inputs

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jkt, Assigned: daleharvey)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

3 years ago
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.

Updated

3 years ago
Blocks: 1304224
Priority: -- → P1
Reporter

Updated

3 years ago
Assignee: nobody → jkt
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8803331 - Flags: review?(MattN+bmo)
Assignee

Comment 4

3 years ago
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

Updated

3 years ago
Assignee: jkt → dale
Dale, any update here?
Flags: needinfo?(dale)
Assignee

Comment 6

3 years ago
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
Flags: needinfo?(dale)
(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.
Assignee

Comment 8

3 years ago
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
Assignee

Updated

3 years ago
Duplicate of this bug: 1120037
Assignee

Comment 10

3 years ago
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
Attachment #8810442 - Flags: review?(bugzilla)
Assignee

Updated

3 years ago
Attachment #8810442 - Flags: review?(bugzilla) → review?(MattN+bmo)
There seems to be test failures on try and it would be easier to review with the two commits in one MozReview series.
(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 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?
Attachment #8810442 - Flags: review?(MattN+bmo) → review+
Assignee

Comment 15

3 years ago
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
Attachment #8810442 - Attachment is obsolete: true
Attachment #8812793 - Flags: review+
Assignee

Comment 16

3 years ago
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?
Attachment #8812793 - Attachment is obsolete: true
Attachment #8813429 - Flags: feedback?(MattN+bmo)
Assignee

Updated

3 years ago
Attachment #8813429 - Flags: feedback?(MattN+bmo)
Assignee

Comment 17

3 years ago
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
Attachment #8813429 - Attachment is obsolete: true
Attachment #8814770 - Flags: review?(MattN+bmo)
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.
Attachment #8814770 - Flags: review?(MattN+bmo) → review-
Assignee

Comment 19

3 years ago
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.
Flags: needinfo?(MattN+bmo)
(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?
Flags: needinfo?(MattN+bmo) → needinfo?(dale)
Assignee

Comment 21

3 years ago
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
Attachment #8814770 - Attachment is obsolete: true
Flags: needinfo?(dale)
Attachment #8815458 - Flags: review?(MattN+bmo)
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!
Attachment #8815458 - Flags: review?(MattN+bmo) → review+

Comment 23

3 years ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72cef09ee478
Ensure login managed inputs focus on load. r=mattn
Dale, can you request this for aurora uplift after it makes it to central?

Updated

3 years ago
Status: NEW → ASSIGNED
Assignee

Comment 25

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

Comment 26

3 years ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8f99127624
Skip test on android, on a CLOSED TREE. r=philor

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72cef09ee478
https://hg.mozilla.org/mozilla-central/rev/fd8f99127624
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Dale, please request aurora uplift.
Flags: needinfo?(dale)
Assignee

Comment 29

3 years ago
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]:
Flags: needinfo?(dale)
Attachment #8815458 - Flags: approval-mozilla-aurora?
Comment on attachment 8815458 [details] [diff] [review]
Ensure login managed inputs focus on load

login form autocomplete fix for aurora52
Attachment #8815458 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
Depends on: 1335332
Setting qe-verify- based on Comment 29.
Flags: qe-verify-
See Also: → 1341582
You need to log in before you can comment on or make changes to this bug.