Add support for multi-page login forms
Categories
(Toolkit :: Password Manager, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox91 | --- | verified |
People
(Reporter: dimi, Assigned: dimi)
References
(Blocks 3 open bugs, Regressed 4 open bugs)
Details
Attachments
(11 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
3.57 KB,
text/plain
|
chutten
:
data-review+
|
Details |
To support password manager features for multi-page login forms, we will need to:
- Implement heuristics to detect username-only forms
- Notify the password manager when a form has username-compatible <input>
- Support autofill, autocomplete, context menu in the username-only form
- Support login capture when users submit the form with <password>
Assignee | ||
Comment 1•3 years ago
|
||
Assignee | ||
Comment 2•3 years ago
|
||
Before this patch, '_getFormFields' doesn't support forms without password fields.
In this patch, when a form doesn't have a password field, we use the
heuristic added in the previous patch to determine whether the form is a
username-only form. If it is, return the username field.
Depends on D113795
Assignee | ||
Comment 3•3 years ago
|
||
Before the patch, we don't show password manager items when there is no password fields found in a form.
In this patch, we do two things to support a username-only form in the context menu:
-
Add "other" category to fieldname hints. "other" is used for fields that are
in a form but are neither "current-password", "new-password", nor "username".
With the change, the "username" hint is now only used for fields that are considered a username field by the password manager. -
When there is no password field in a form, ContextMenu also treat a
form as a login form when the active field is a username field, which means it is a username-only form.
Depends on D113796
Assignee | ||
Comment 4•3 years ago
|
||
Right now, we limit the type of a username field in username-only forms to be either text or email.
This is different from what the password manager currently support in LoginHelper.isUsernameFieldType.
This is because text and email type are the most common cases for a username field, and we want to focus
on the cases that are more likely a username field.
This patch adds "DOMFormHasPossibleUsername" event to notify the password manager when a form has a possible
username field (text or email). The event works similar to the existing "DOMFormHasPassword" event.
Depends on D113797
Assignee | ||
Comment 5•3 years ago
|
||
This patch does two things:
- Checks whether a form is a username-only form after receiving 'DOMFormHasPossibleUsername' event.
If it is, fetch logins from the parent to trigger form autofill. - Makes LoginManagerChild._fillForm compatible with an empty password field when there is an username field.
Depends on D113798
Assignee | ||
Comment 6•3 years ago
|
||
In the previous patch to support autofill, a username field in a
username-only form is marked as a login manager field in '_fillForm'.
So this patch only makes autocomplete highlight work when autocomplete a username field.
This patch calls '_highlightFilledField' in 'onUsernameAutoCompleted'
when the autocompleted field is the username field in a username-only
form.
This is because when we autocomplete a username field in a form with password fields,
the highlight is done in '_fillForm'. However, in the case of a username-only
form, we don't have to call '_fillForm' anymore due to no password to fill.
Depends on D113799
Assignee | ||
Comment 7•3 years ago
|
||
This patch saves the username field in a username-only form when the form is submitted.
When another form in the same document is submitted after that, if the form doesn't
have a username field, we then use the username field found in the username-only form to capture login.
This fits the case that in a multipage sign-in form, the first form is
usually a username-only form and the second form is usually a password-only form.
However, in the current approach, if in the second form, there is an input field before the
password field, we will use the input field in the second form as the username field, not the username field
in the first form. For example, in a multipage registration form, the first form is a username-only form to
enter your email, and the second form has an input field to enter the last name and a password field. With the
current approach, we will save "last name" + "password" instead of "username" + "password".
An alternate is always using the username field in the first form or coming up with a heuristic
to "compare" the two username fields. But since I haven't found a real-world example of the above scenario,
using the current approach seems a safer way to introduce the feature.
Depends on D113800
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D113801
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 9•3 years ago
|
||
This patch adds two telemetry to measure the performance impact after adding
multi-page login support.
Telemtry PWMGR_IS_USERNAME_ONLY_FORM gives us an idea among
all forms that contain a possible username input (type is email or text), the propotion
of those forms that are considered as a username-only form by our heuristic. We can
use this data as a hint of whether the username-only form heuristic works properly.
Telemetry PWMGR_NUM_FORM_HAS_POSSIBLE_USERNAME_EVENT_PER_DOC gives us an
idea how many forms contain a possible username input per page. If the data shows that there are a
lot of pages that contain multiple forms with a possible username input, which
triggers the new code path added in this bug, we might need to pay more attention to see whether the
change introduces performance overhead for page load.
Ex. A doc has 4 forms
<form><input type=email autocomplete=username/></form> <!-- This is a form with a possible username input, and it is a username-only form-->
<form><input type=text autocomplete=username/></form> <!-- This is a form with a possible username input, and it is a username-only form-->
<form><input type=email/></form> <!-- This is a form with a possible username input, but it is NOT a username-onlyc form -->
<form><input type=urk/></form> <!-- This is a form WITHOUT a possible username input -->
PWMGR_IS_USERNAME_ONLY_FORM records
bucket[0] = 1 // 1 form with a possible username input but not a username-only form
bucket[1] = 2 // 2 forms are username-only form.
PWMGR_NUM_FORM_HAS_POSSIBLE_USERNAME_EVENT_PER_DOC records
bucket[0] = 0
bucket[1] = 1 // 1 doc has 1 or more than 1 form with a possible-username input
bucket[2] = 1 // 1 doc has 2 or more than 2 form with a possible-username input
bucket[3] = 1 // 1 doc has 3 or more than 3 form with a possible-username input
bucket[4] = 0 // 0 doc has 4 or more than 4 form with a possible-username input
Updated•3 years ago
|
Assignee | ||
Comment 10•3 years ago
|
||
Assignee | ||
Comment 11•3 years ago
|
||
Comment 12•3 years ago
|
||
Comment on attachment 9224307 [details]
request.md
DATA COLLECTION REVIEW RESPONSE:
Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?
Yes.
Is there a control mechanism that allows the user to turn the data collection on and off?
Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.
If the request is for permanent data collection, is there someone who will monitor the data over time?
Yes, Dimi Lee is responsible.
Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Category 1, Technical.
Is the data collection request for default-on or default-off?
Default on for all channels.
Does the instrumentation include the addition of any new identifiers?
No.
Is the data collection covered by the existing Firefox privacy notice?
Yes.
Does the data collection use a third-party collection tool?
No.
Result: datareview+
Comment 13•3 years ago
|
||
Pushed by dlee@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/11466700b3aa P1. Add a heuristic to detect username-only login forms r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/6fd40aa5bb6a P2. Support username-only forms in _getFormFields r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/9edf52be81f4 P3. Support showing password manager items in context menu for username fields that are in a username-only form. r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/0a0c13fc50dc P4. Add DOMFormHasPossibleUsernameField event to notify the password manager when a form has a text input or an email input. r=sfoster,tgiles,smaug https://hg.mozilla.org/integration/autoland/rev/e272694ff107 P5. Support autofilling a username-only form. r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/f229df28fb65 P6. Support autocomplete in a username-only form. r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/c35ab1fbad38 P7. Support login capture for multipage login forms. r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/58c928c7be48 P8. Add AutoFillResult.FILLED_USERNAME_ONLY_FORM to record autofill in username-only forms. r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/2049353f2034 P9. Add telemetry probe to measure the performance of detecting username-only forms. r=sfoster,tgiles https://hg.mozilla.org/integration/autoland/rev/fb9022ada542 P10. Add signon.usernameOnlyForm.enabled preference r=sfoster,tgiles
Comment 14•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11466700b3aa
https://hg.mozilla.org/mozilla-central/rev/6fd40aa5bb6a
https://hg.mozilla.org/mozilla-central/rev/9edf52be81f4
https://hg.mozilla.org/mozilla-central/rev/0a0c13fc50dc
https://hg.mozilla.org/mozilla-central/rev/e272694ff107
https://hg.mozilla.org/mozilla-central/rev/f229df28fb65
https://hg.mozilla.org/mozilla-central/rev/c35ab1fbad38
https://hg.mozilla.org/mozilla-central/rev/58c928c7be48
https://hg.mozilla.org/mozilla-central/rev/2049353f2034
https://hg.mozilla.org/mozilla-central/rev/fb9022ada542
Assignee | ||
Comment 15•3 years ago
|
||
Hi Timea,
Could you help verify whether the dependent bugs are fixed? thanks!
Comment 16•3 years ago
|
||
Hi Dimi,
All the dependent fixed bugs were verified and confirmed as fixed. There was 1 new Bug 1717200 raised in the process that affects Disney+ and NewEgg.com, otherwise looks good. Thus, marking this issue as well verified-fixed.
Comment 17•3 years ago
|
||
Hi Dimi, this improvement sounds like something that could be mentioned in our beta and final release notes for 91, is that planned? Thanks
Assignee | ||
Comment 18•3 years ago
|
||
(In reply to Pascal Chevrel:pascalc from comment #17)
Hi Dimi, this improvement sounds like something that could be mentioned in our beta and final release notes for 91, is that planned? Thanks
Hi Pascal, Yes, I think this is something worth adding to the release note.
However, I have one question. The supporting of the multi-page login form has not yet covered all the cases (A very rough estimation is, right now, 50% cases are covered), which means there is still room to improve. In this case, do we want to put this item in the release note now? or do we prefer waiting until the other improvements are done.
Assignee | ||
Comment 19•3 years ago
|
||
After discussing with the team, we decide to disable the support of multi-page login form on release channel for 2-3 cycles ito make sure the feature is in a stable state. We have seen some false-positive cases so far (See Bug 1718797).
We will continue to monitor whether there is other false-positive cases, if everything goes well, the plan is to re-enable is on release 94.
Description
•