Closed Bug 1708455 Opened 3 years ago Closed 3 years ago

Add support for multi-page login forms

Categories

(Toolkit :: Password Manager, enhancement, P2)

enhancement

Tracking

()

VERIFIED FIXED
91 Branch
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:

  1. Implement heuristics to detect username-only forms
  2. Notify the password manager when a form has username-compatible <input>
  3. Support autofill, autocomplete, context menu in the username-only form
  4. Support login capture when users submit the form with <password>

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

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:

  1. 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.

  2. 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

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

This patch does two things:

  1. 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.
  2. Makes LoginManagerChild._fillForm compatible with an empty password field when there is an username field.

Depends on D113798

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

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

Severity: -- → N/A
Blocks: 1710811
Blocks: 1710824

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

Attachment #9223994 - Attachment description: Bug 1708455 - P9. Add telemetry probe for detecting username-only forms. r=sfoster → Bug 1708455 - P9. Add telemetry probe to measure the performance of detecting username-only forms. r=sfoster
Attached file request.md
Attachment #9224307 - Flags: data-review?(chutten)

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+

Attachment #9224307 - Flags: data-review?(chutten) → data-review+
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
Regressions: 1716033
Blocks: 1692167
Blocks: 1691745
Blocks: 1691671
Blocks: 1608203
Blocks: 1662192

Hi Timea,
Could you help verify whether the dependent bugs are fixed? thanks!

Flags: qe-verify+
Flags: needinfo?(timea.babos)

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(timea.babos)
Regressions: 1718797

Hi Dimi, this improvement sounds like something that could be mentioned in our beta and final release notes for 91, is that planned? Thanks

Flags: needinfo?(dlee)

(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.

Flags: needinfo?(dlee) → needinfo?(pascalc)

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.

Flags: needinfo?(pascalc)
Blocks: 1721971
Regressions: 1724136
Blocks: 1732901
Blocks: 1733944
See Also: → 1736632
Regressions: 1740894
No longer regressions: 1740894
Regressions: 1740894
Blocks: 1745918
No longer blocks: 1733944
See Also: → 1791929
Regressions: 1794636
Regressions: 1796528
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: