Fill passwords on combined sign-up + sign-in forms |
|||
Issue descriptionMany pages (e.g., https://www.vistaprint.com/, https://tools.usps.com/, https://www.showroomprive.com/, https://olui2.fs.ml.com/, https://www.adecco.fr/) combine multiple logical forms (sign-in form + sign-up form) in a single <form> tag. While autofill server often hints what the new-password (and confirmation password) fields on those are, there is no hint about the current-password. As a result, Chrome does not fill the sign-in part automatically (although fill on demand works). One potential way to fix this would be to add the current-password annotations to the server (ideally through voting from the client, but also with manual overrides in the worst case) and teach Chrome that it is OK to fill current-passwords hinted at by the server. We'd need to also make sure that the server marks the username and that the username is the one for sign-in (no point marking the sign-up username, which should not be filled anyway).
,
Oct 16
I had a peek at a couple of examples, and the login field classifier is able to predict the username and current-password fields. However, we are lacking logic for merging signup and login predictions. At the very least, I image we need new types: new-username and current-username, because we make username predictions on signup forms too.
,
Oct 16
I'm afraid that we indeed do need separate username hints: while there is no need to fill the sign-up username, there might be need to pick one of multiple text fields on saving the sign-up form. We have the choice of doing the merging either on server (there we would need to add new types such as new-username and current-username), or on client (where we could just consider predictions separately and only use sign-in ones for filling, and decide on which to use based on non-empty passwords on saving.
,
Oct 18
I wrote up a proposal which I plan to implement. https://docs.google.com/document/d/1i4Hwx8yxfHPACAy4ds85ZhET4Q58IEgPZsN3EXH4TfQ/edit?usp=sharing Comments welcome.
,
Oct 29
Shouldn't this bug be type=regression? From what I can see from a related email thread, this used to work in the old parser.
,
Oct 29
We've never worked on sign-in + sign-up forms reliable, everything depends on forms layout.
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cada912e3f6cf9c9425560218a175b7b2944ec40 commit cada912e3f6cf9c9425560218a175b7b2944ec40 Author: Vaclav Brozek <vabr@chromium.org> Date: Wed Nov 07 11:12:51 2018 Make password form parser handle two usernames This CL implements the part of the design https://goo.gl/Mc2KRe, which handles the situations when the autofill server marks two fields as usernames, and the parser needs to decide which is relevant. Bug: 895781 Change-Id: I37ef3bc55697c108b1761bf45e28deb9a20a2c1d Reviewed-on: https://chromium-review.googlesource.com/c/1318894 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#606015} [modify] https://crrev.com/cada912e3f6cf9c9425560218a175b7b2944ec40/components/password_manager/core/browser/form_parsing/form_parser.cc [modify] https://crrev.com/cada912e3f6cf9c9425560218a175b7b2944ec40/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
,
Nov 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f2fba73d326bfddce3f24f097902068a27c8ea5 commit 3f2fba73d326bfddce3f24f097902068a27c8ea5 Author: Vaclav Brozek <vabr@chromium.org> Date: Fri Nov 09 19:32:41 2018 Add UKM: PasswordForm.FillOnLoad Chrome currently won't autofill credentials into forms which have a "new-password" field. A better criterion seems to be: only fill into forms with a "current-password" field. This CL adds a UKM metric to understand the impact of such change. Privacy review of this metric: The whole UKM event "PasswordForm" has been reviewed in crbug.com/728707 and go/gkmnc, where the privacy TL agreed that adding new metrics for this event is OK under certain conditions (see the linked doc). The new metric, which measures changes to filling on load of forms for which the user previously stored some credentials, satisfies such conditions, in the opinion of the CL author, and hence is covered by that review. The new metric has been added to the requested spreadsheet listing all passwords-related metrics. Bug: 895781 Change-Id: I8bb6537f97c96d0ed345d885a7ddd7c065370b22 Reviewed-on: https://chromium-review.googlesource.com/c/1326497 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#606931} [modify] https://crrev.com/3f2fba73d326bfddce3f24f097902068a27c8ea5/components/password_manager/core/browser/DEPS [modify] https://crrev.com/3f2fba73d326bfddce3f24f097902068a27c8ea5/components/password_manager/core/browser/password_form_filling.cc [modify] https://crrev.com/3f2fba73d326bfddce3f24f097902068a27c8ea5/components/password_manager/core/browser/password_form_filling_unittest.cc [modify] https://crrev.com/3f2fba73d326bfddce3f24f097902068a27c8ea5/components/password_manager/core/browser/password_form_metrics_recorder.cc [modify] https://crrev.com/3f2fba73d326bfddce3f24f097902068a27c8ea5/components/password_manager/core/browser/password_form_metrics_recorder.h [modify] https://crrev.com/3f2fba73d326bfddce3f24f097902068a27c8ea5/tools/metrics/ukm/ukm.xml
,
Nov 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/722eff78702fd3339f27c2107d75f731f0320781 commit 722eff78702fd3339f27c2107d75f731f0320781 Author: Vaclav Brozek <vabr@chromium.org> Date: Fri Nov 09 21:00:24 2018 Start filling change password forms Chrome currently won't autofill credentials into forms which have a "new-password" field. This includes change-password forms, which also have the "current-password" field. Filling that is actually useful for users. Therefore this CL changes the condition aginst autofilling to only pick forms which do not have the "current-password" field. This is limited to the kNewPasswordFormParsing feature, to make it easier to understand the impact of that change to the overall difference between the old parser and the new parser. Bug: 895781 Change-Id: I39cee16c725d108213e045f0cef94f52a6e80837 Reviewed-on: https://chromium-review.googlesource.com/c/1309742 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Cr-Commit-Position: refs/heads/master@{#606964} [modify] https://crrev.com/722eff78702fd3339f27c2107d75f731f0320781/components/password_manager/core/browser/password_form_filling.cc [modify] https://crrev.com/722eff78702fd3339f27c2107d75f731f0320781/components/password_manager/core/browser/password_form_filling_unittest.cc
,
Nov 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cd0a836988755d7ddaf498c06c4b9cfa0f18c5de commit cd0a836988755d7ddaf498c06c4b9cfa0f18c5de Author: Vaclav Brozek <vabr@chromium.org> Date: Mon Nov 19 10:02:25 2018 Fix PasswordForm.FillOnLoad UKM The PasswordForm.FillOnLoad UKM aims at measuring the change in behaviour of Chrome with the new password form parser, if an explicit condition for filling on load is changed from A to B. It turned out that even if A is the explicit condition, B still can break fill on load. So the change is not so much A -> B as A && B -> B. This means that the metric's bucket recording the state when A is true and B false needs to be merged into the bucket when A == B, because in both cases the behaviour does not change. Bug: 895781 Change-Id: Ic3ab14d9a1e43c0312580d2dfe5bff34f2cdd20a Reviewed-on: https://chromium-review.googlesource.com/c/1340274 Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Commit-Queue: Vaclav Brozek <vabr@chromium.org> Cr-Commit-Position: refs/heads/master@{#609221} [modify] https://crrev.com/cd0a836988755d7ddaf498c06c4b9cfa0f18c5de/components/password_manager/core/browser/password_form_filling.cc [modify] https://crrev.com/cd0a836988755d7ddaf498c06c4b9cfa0f18c5de/components/password_manager/core/browser/password_form_filling_unittest.cc [modify] https://crrev.com/cd0a836988755d7ddaf498c06c4b9cfa0f18c5de/components/password_manager/core/browser/password_form_metrics_recorder.h
,
Nov 19
Once https://crrev.com/c/1341540 lands, this will be finished except for generating server hints containing current passwords for the combined forms. That point has not been part of version 1, and while it would be good to do it, it won't be tracked here.
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e66f706936cd2ae9334bb5c80760c7b34ba6e2c commit 6e66f706936cd2ae9334bb5c80760c7b34ba6e2c Author: Vaclav Brozek <vabr@chromium.org> Date: Wed Nov 21 09:30:09 2018 Clean up PasswordForm.FillOnLoad UKM There was a discussion about whether Chrome should avoid filling on load if a form has some new-password fields, or if the condition should be "has no current-password fields". The PasswordForm.FillOnLoad metric supported a team decision on that. The decision was to go with the new condition (for the new parser). Therefore the metric is now marked as obsolete and no longer collected. Bug: 895781 Change-Id: I3a153bf706abe12a5621b0a209cb53b6604ce875 Reviewed-on: https://chromium-review.googlesource.com/c/1341540 Commit-Queue: Vaclav Brozek <vabr@chromium.org> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Cr-Commit-Position: refs/heads/master@{#609965} [modify] https://crrev.com/6e66f706936cd2ae9334bb5c80760c7b34ba6e2c/components/password_manager/core/browser/DEPS [modify] https://crrev.com/6e66f706936cd2ae9334bb5c80760c7b34ba6e2c/components/password_manager/core/browser/password_form_filling.cc [modify] https://crrev.com/6e66f706936cd2ae9334bb5c80760c7b34ba6e2c/components/password_manager/core/browser/password_form_filling_unittest.cc [modify] https://crrev.com/6e66f706936cd2ae9334bb5c80760c7b34ba6e2c/components/password_manager/core/browser/password_form_metrics_recorder.cc [modify] https://crrev.com/6e66f706936cd2ae9334bb5c80760c7b34ba6e2c/components/password_manager/core/browser/password_form_metrics_recorder.h [modify] https://crrev.com/6e66f706936cd2ae9334bb5c80760c7b34ba6e2c/tools/metrics/ukm/ukm.xml |
|||
►
Sign in to add a comment |
|||
Comment 1 by cfroussios@chromium.org
, Oct 16