1149500 - Delay autofill on background tabs until selected

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

The timing when the (modal!) master password prompt appears is seemingly arbitrary. The user is prompted once per session, but sometimes that prompt happens directly on startup, sometimes a few minutes later and sometimes only when actually filling a password (even then the timing is bad since it appears before the page is even loaded).

We should find a way to make that behavior more predictable

Comment 1

[David Lawrence [:dkl]]

Mistakenly filed against Firefox 38 and should be instead 38 Branch. Sorry for the spam. dkl

Comment 2

[Albert Scheiner [:alberts]]

This is currently set for Platform Mac OS, but I am pretty sure it's the same under Windows. Change to all?

Comment 3

[Matthew N. [:MattN]]

I'm going to morph this bug to cover the case where Sync is not setup.

We should either:
A) not autofill on page load and require the user to use the autocomplete dropdown or context menu
or
B) not autofill for background tabs, wait until the tab becomes focused. We can use the visibilitychange event for this.

Comment 6

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

We should either:
A) not autofill on page load and require the user to use the autocomplete
dropdown or context menu

This doesn't really work since the username is also encrypted so we wouldn't be able to show them without first getting the MP. If we wait to show the MP when autocomplete opens then the MP dialog will cause autocomplete to close which would be bad.

B) not autofill for background tabs, wait until the tab becomes focused. We
can use the visibilitychange event for this.

We should implement this for now as it helps delay the MP dialog at least until the login page is visible and prevents tracking the user via autofill in hidden frames or background tabs/popups.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1149500 - Delay autofill until the document is visible. r?MattN

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #7)

Created attachment 9044372 [details]
Bug 1149500 - Delay autofill until the document is visible. r?MattN

MattN: This is a first pass at this, with a couple of tests to verify the defer form autofill until the document is visible thing. I'm not sure if there are other cases that should be covered by tests - I see various secure/not, cross-origin, iframed cases in browser_http_autofill.js, but it wasnt clear to me if covering all those again here would be valuable.

Other cases which come to mind - closing a tab without ever making it visible, navigating away from the form in a hidden tab. Destroying a form while the document is hidden?

The case where logins are added while the document is hidden that might change how it is autofilled - should just work with this approach.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Oh and master password. I could add an explicit test with LoginTestUtils.masterPassword.enable().

Comment 10

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #8)

(In reply to Sam Foster [:sfoster] (he/him) from comment #7)

Created attachment 9044372 [details]
Bug 1149500 - Delay autofill until the document is visible. r?MattN

MattN: This is a first pass at this, with a couple of tests to verify the defer form autofill until the document is visible thing. I'm not sure if there are other cases that should be covered by tests - I see various secure/not, cross-origin, iframed cases in browser_http_autofill.js, but it wasnt clear to me if covering all those again here would be valuable.

I don't think so. For some reason I thought visibilityState was on a per-frame basis but now I see that the top-level browsing context is all that matters so I guess you don't need to test with sub-frames.

Other cases which come to mind - closing a tab without ever making it visible, navigating away from the form in a hidden tab. Destroying a form while the document is hidden?

I'm not sure you need those since those should mostly be testing visibilitychange/VisibilityState and WeakMaps since I believe we want to do nothing in those cases.

The case where logins are added while the document is hidden that might change how it is autofilled - should just work with this approach.

Agreed.

Comment 11

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #9)

Oh and master password. I could add an explicit test with LoginTestUtils.masterPassword.enable().

I think that would be good. Just a simple one where there is a MP and it's logged out and a background login form tab opens with a matching saved login.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d16bf53b3585
Delay autofill until the document is visible. r=MattN

Comment 13

[Natalia Csoregi [:nataliaCs]]

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

Comment 14

[Robin Steuber (they/them) [:bytesized]]

https://hg.mozilla.org/projects/oak/rev/d16bf53b358583baa538e24697f85a597e4cf41c
Bug 1149500 - Delay autofill until the document is visible. r=MattN

Comment 15

[Matthew N. [:MattN]]

Due to regressions we stopped delaying the autofill if a master password is set (bug 1538460) so we didn't address the issue of MP prompts from background tabs in this bug. I filed bug 1567574 for that.