[ios] Fix the is_user_edited bit in parsed fields for Autofill
Use a feature flag instead of an indirect strategy to enable the feature to set the is_user_edited bit in parsed fields. As is_user_edited has been no op for a while (probably since Jun 2023 where we did the JS -> TS migration, http://crrev.com/c/https://chromium-review.googlesource.com/c/chromium/src/+/4611286), we should do a feature gated rollout so we can measure the impact of fixing is_user_edited. With the fix, is_user_edited is set to false by default then flipped to true as soon as a user "input" is detected in the field, whereas right now is_user_edited is always set to true. With the fix we expect more perfect fills (metric-wise not in reality) and maybe a bit more card updates. R=tmartino@chromium.org Change-Id: I1b0227740213297e4fda5c8db02661cdcaa41466 Fixed: 397440877 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6281435 Reviewed-by: Tommy Martino <tmartino@chromium.org> Commit-Queue: Vincent Boisselle <vincb@google.com> Cr-Commit-Position: refs/heads/main@{#1423460}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
fa8bc2c948
commit
165972ede7
components/autofill/ios
browser
common
form_util
ios/chrome/browser/autofill
model
ui_bundled
form_input_accessory
@ -1350,10 +1350,6 @@ bool ContainsFocusableField(const FormData& form, FieldRendererId field_id) {
|
||||
formHandlerFeature->TrackFormMutations(frame,
|
||||
kMutationTrackingEnabledDelayInMs);
|
||||
|
||||
formHandlerFeature->ToggleTrackingUserEditedFields(
|
||||
frame,
|
||||
/*track_user_edited_fields=*/true);
|
||||
|
||||
driver->ScanForms(/*immediately=*/base::FeatureList::IsEnabled(
|
||||
kAutofillThrottleDocumentFormScanForceFirstScanIos));
|
||||
}
|
||||
|
@ -15,6 +15,10 @@ BASE_DECLARE_FEATURE(kAddAddressManually);
|
||||
// Returns true if the AddAddressManually feature is enabled
|
||||
bool IsAddAddressManuallyEnabled();
|
||||
|
||||
// Enables correctly setting the is_user_edited bit in the parsed form fields
|
||||
// instead of using true by default.
|
||||
BASE_DECLARE_FEATURE(kAutofillCorrectUserEditedBitInParsedField);
|
||||
|
||||
// Controls whether to dynamically load the address input fields in the save
|
||||
// flow and settings based on the country value.
|
||||
// TODO(crbug.com/40281788): Remove once launched.
|
||||
|
@ -16,6 +16,12 @@ bool IsAddAddressManuallyEnabled() {
|
||||
kAutofillDynamicallyLoadsFieldsForAddressInput);
|
||||
}
|
||||
|
||||
// LINT.IfChange(autofill_correct_user_edited_bit_in_parsed_field)
|
||||
BASE_FEATURE(kAutofillCorrectUserEditedBitInParsedField,
|
||||
"AutofillCorrectUserEditedBitInParsedField",
|
||||
base::FEATURE_DISABLED_BY_DEFAULT);
|
||||
// LINT.ThenChange(/components/autofill/ios/form_util/resources/autofill_form_features.ts:autofill_correct_user_edited_bit_in_parsed_field)
|
||||
|
||||
BASE_FEATURE(kAutofillDynamicallyLoadsFieldsForAddressInput,
|
||||
"AutofillDynamicallyLoadsFieldsForAddressInput",
|
||||
base::FEATURE_DISABLED_BY_DEFAULT);
|
||||
|
@ -53,6 +53,11 @@ void SetAutofillFormFeatureFlags(WebFrame* web_frame) {
|
||||
->SetAutofillFixPaymentSheetSpam(
|
||||
web_frame,
|
||||
base::FeatureList::IsEnabled(kAutofillFixPaymentSheetSpam));
|
||||
|
||||
AutofillFormFeaturesJavaScriptFeature::GetInstance()
|
||||
->SetAutofillCorrectUserEditedBitInParsedField(
|
||||
web_frame, base::FeatureList::IsEnabled(
|
||||
kAutofillCorrectUserEditedBitInParsedField));
|
||||
}
|
||||
|
||||
AutofillFormFeaturesInjector::~AutofillFormFeaturesInjector() = default;
|
||||
|
@ -62,7 +62,8 @@ TEST_F(AutofillFormInjectorTest, InjectFlagsWebFrames) {
|
||||
autofill::features::kAutofillAcrossIframesIos,
|
||||
autofill::features::
|
||||
kAutofillAcrossIframesIosThrottling,
|
||||
kAutofillFixPaymentSheetSpam},
|
||||
kAutofillFixPaymentSheetSpam,
|
||||
kAutofillCorrectUserEditedBitInParsedField},
|
||||
/* disabled_features= */ {});
|
||||
|
||||
AutofillFormFeaturesInjector injector(&fake_web_state_,
|
||||
@ -72,16 +73,18 @@ TEST_F(AutofillFormInjectorTest, InjectFlagsWebFrames) {
|
||||
for (auto* web_frame : fake_web_frames_manager_->GetAllWebFrames()) {
|
||||
auto* fake_frame = static_cast<FakeWebFrame*>(web_frame);
|
||||
|
||||
EXPECT_THAT(
|
||||
fake_frame->GetJavaScriptCallHistory(),
|
||||
UnorderedElementsAre(u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillIsolatedContentWorld(true);",
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillAcrossIframes(true);",
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillAcrossIframesThrottling(true);",
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillFixPaymentSheetSpam(true);"));
|
||||
EXPECT_THAT(fake_frame->GetJavaScriptCallHistory(),
|
||||
UnorderedElementsAre(
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillIsolatedContentWorld(true);",
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillAcrossIframes(true);",
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillAcrossIframesThrottling(true);",
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillFixPaymentSheetSpam(true);",
|
||||
u"__gCrWeb.autofill_form_features."
|
||||
u"setAutofillCorrectUserEditedBitInParsedField(true);"));
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -39,6 +39,11 @@ class AutofillFormFeaturesJavaScriptFeature : public web::JavaScriptFeature {
|
||||
// suggestion).
|
||||
void SetAutofillFixPaymentSheetSpam(web::WebFrame* frame, bool enabled);
|
||||
|
||||
// Enables/disables correctly setting the is_user_edited bit in the parsed
|
||||
// form fields instead of using true by default.
|
||||
void SetAutofillCorrectUserEditedBitInParsedField(web::WebFrame* frame,
|
||||
bool enabled);
|
||||
|
||||
private:
|
||||
friend class base::NoDestructor<AutofillFormFeaturesJavaScriptFeature>;
|
||||
|
||||
|
@ -70,4 +70,13 @@ void AutofillFormFeaturesJavaScriptFeature::SetAutofillFixPaymentSheetSpam(
|
||||
base::Value::List().Append(enabled));
|
||||
}
|
||||
|
||||
void AutofillFormFeaturesJavaScriptFeature::
|
||||
SetAutofillCorrectUserEditedBitInParsedField(web::WebFrame* frame,
|
||||
bool enabled) {
|
||||
CHECK(frame);
|
||||
frame->CallJavaScriptFunction(
|
||||
"autofill_form_features.setAutofillCorrectUserEditedBitInParsedField",
|
||||
base::Value::List().Append(enabled));
|
||||
}
|
||||
|
||||
} // namespace autofill
|
||||
|
@ -32,10 +32,6 @@ class FormHandlersJavaScriptFeature : public web::JavaScriptFeature {
|
||||
// true.
|
||||
void TrackFormMutations(web::WebFrame* frame, int mutation_tracking_delay);
|
||||
|
||||
// Toggles tracking the source of the input events in the frame.
|
||||
void ToggleTrackingUserEditedFields(web::WebFrame* frame,
|
||||
bool track_user_edited_fields);
|
||||
|
||||
private:
|
||||
friend class base::NoDestructor<FormHandlersJavaScriptFeature>;
|
||||
// TODO(crbug.com/359538514): Remove friend once isolated world for Autofill
|
||||
|
@ -76,13 +76,6 @@ void FormHandlersJavaScriptFeature::TrackFormMutations(
|
||||
base::Value::List().Append(mutation_tracking_delay));
|
||||
}
|
||||
|
||||
void FormHandlersJavaScriptFeature::ToggleTrackingUserEditedFields(
|
||||
web::WebFrame* frame,
|
||||
bool track_user_edited_fields) {
|
||||
CallJavaScriptFunction(frame, "formHandlers.toggleTrackingUserEditedFields",
|
||||
base::Value::List().Append(track_user_edited_fields));
|
||||
}
|
||||
|
||||
std::optional<std::string>
|
||||
FormHandlersJavaScriptFeature::GetScriptMessageHandlerName() const {
|
||||
return kScriptMessageName;
|
||||
|
@ -40,6 +40,15 @@ modal dialog that was triggered from the KA (e.g. filling a suggestion).
|
||||
let autofillFixPaymentSheetSpam: boolean = false;
|
||||
// LINT.ThenChange(//components/autofill/ios/common/features.mm:autofill_fix_post_filling_payment_sheet)
|
||||
|
||||
|
||||
// LINT.IfChange(autofill_correct_user_edited_bit_in_parsed_field)
|
||||
/**
|
||||
Enables correctly setting the is_user_edited bit in the parsed form fields
|
||||
instead of using true by default.
|
||||
*/
|
||||
let autofillCorrectUserEditedBitInParsedField: boolean = false;
|
||||
// LINT.ThenChange(//components/autofill/ios/common/features.mm:autofill_correct_user_edited_bit_in_parsed_field)
|
||||
|
||||
/**
|
||||
* @see autofillAcrossIframes
|
||||
*/
|
||||
@ -96,6 +105,20 @@ function isAutofillFixPaymentSheetSpamEnabled(): boolean {
|
||||
return autofillFixPaymentSheetSpam;
|
||||
}
|
||||
|
||||
/**
|
||||
* @see autofillCorrectUserEditedBitInParsedField
|
||||
*/
|
||||
function setAutofillCorrectUserEditedBitInParsedField(enabled: boolean): void {
|
||||
autofillCorrectUserEditedBitInParsedField = enabled;
|
||||
}
|
||||
|
||||
/**
|
||||
* @see autofillCorrectUserEditedBitInParsedField
|
||||
*/
|
||||
function isAutofillCorrectUserEditedBitInParsedField(): boolean {
|
||||
return autofillCorrectUserEditedBitInParsedField;
|
||||
}
|
||||
|
||||
// Expose globally via `gCrWeb` instead of `export` to ensure state (feature
|
||||
// on/off) is maintained across imports.
|
||||
gCrWeb.autofill_form_features = {
|
||||
@ -107,4 +130,6 @@ gCrWeb.autofill_form_features = {
|
||||
isAutofillIsolatedContentWorldEnabled,
|
||||
setAutofillFixPaymentSheetSpam,
|
||||
isAutofillFixPaymentSheetSpamEnabled,
|
||||
setAutofillCorrectUserEditedBitInParsedField,
|
||||
isAutofillCorrectUserEditedBitInParsedField,
|
||||
};
|
||||
|
@ -27,7 +27,7 @@ const kNamelessFieldIDPrefix = 'gChrome~field~';
|
||||
* programmatically.
|
||||
* If the map is null, the source of changed is not track.
|
||||
*/
|
||||
const wasEditedByUser: WeakMap<any, any>|null = null;
|
||||
const wasEditedByUser: WeakMap<any, any> = new WeakMap();
|
||||
|
||||
/**
|
||||
* Based on Element::isFormControlElement() (WebKit)
|
||||
@ -292,20 +292,15 @@ function getFormElementFromRendererId(identifier: number): HTMLFormElement|
|
||||
|
||||
/**
|
||||
* Returns whether the last `input` or `change` event on `element` was
|
||||
* triggered by a user action (was "trusted").
|
||||
* triggered by a user action (was "trusted"). Returns true by default if the
|
||||
* feature to fix the user edited bit isn't enabled which is the status quo.
|
||||
* TODO(crbug.com/40941928): Match Blink's behavior so that only a 'reset' event
|
||||
* makes an edited field unedited.
|
||||
*/
|
||||
function fieldWasEditedByUser(element: Element) {
|
||||
if (wasEditedByUser === null) {
|
||||
// Input event sources is not tracked.
|
||||
// Return true to preserve previous behavior.
|
||||
return true;
|
||||
}
|
||||
if (!wasEditedByUser.has(element)) {
|
||||
return false;
|
||||
}
|
||||
return wasEditedByUser.get(element);
|
||||
return !gCrWeb.autofill_form_features
|
||||
.isAutofillCorrectUserEditedBitInParsedField() ||
|
||||
(wasEditedByUser.get(element) ?? false);
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -445,16 +445,4 @@ function trackFormMutations(delay: number): void {
|
||||
formMutationObserver.observe(document, {childList: true, subtree: true});
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Enables or disables the tracking of input event sources.
|
||||
*/
|
||||
function toggleTrackingUserEditedFields(track: boolean): void {
|
||||
if (track) {
|
||||
gCrWeb.form.wasEditedByUser = gCrWeb.form.wasEditedByUser || new WeakMap();
|
||||
} else {
|
||||
gCrWeb.form.wasEditedByUser = null;
|
||||
}
|
||||
}
|
||||
|
||||
gCrWeb.formHandlers = {trackFormMutations, toggleTrackingUserEditedFields};
|
||||
gCrWeb.formHandlers = {trackFormMutations};
|
||||
|
@ -7,9 +7,11 @@
|
||||
#import "base/format_macros.h"
|
||||
#import "base/strings/sys_string_conversions.h"
|
||||
#import "base/test/ios/wait_util.h"
|
||||
#import "base/test/scoped_feature_list.h"
|
||||
#import "components/autofill/core/common/autofill_constants.h"
|
||||
#import "components/autofill/ios/browser/autofill_java_script_feature.h"
|
||||
#import "components/autofill/ios/browser/autofill_util.h"
|
||||
#import "components/autofill/ios/common/features.h"
|
||||
#import "components/autofill/ios/form_util/autofill_form_features_java_script_feature.h"
|
||||
#import "components/autofill/ios/form_util/form_util_java_script_feature.h"
|
||||
#import "ios/chrome/browser/shared/model/profile/test/test_profile_ios.h"
|
||||
@ -1923,6 +1925,49 @@ TEST_F(AutofillControllerJsTest, ExtractForms) {
|
||||
}];
|
||||
}
|
||||
|
||||
// Test that the is_user_edited bit is correctly set in the extracted fields
|
||||
// when the fix is enabled. This test is limited as it can't test if
|
||||
// is_user_edited can be set to true because there is no way to emulate an
|
||||
// input from the user in the unittest (i.e. Event.isTrusted set to true) - this
|
||||
// would required popping up a keyboard.
|
||||
TEST_F(AutofillControllerJsTest, ExtractForms_UserEdited_FixEnabled) {
|
||||
base::test::ScopedFeatureList feature_list;
|
||||
feature_list.InitAndEnableFeature(kAutofillCorrectUserEditedBitInParsedField);
|
||||
|
||||
// Load html form that consist of 2 plain text inputs that the user can type
|
||||
// in.
|
||||
NSString* html = @"<html><body>"
|
||||
"<form id='form1'>"
|
||||
"<input type='text' id='input1' />"
|
||||
"<input type='text' id='input2' />"
|
||||
"</form>"
|
||||
"</body></html>";
|
||||
web::test::LoadHtml(html, web_state());
|
||||
|
||||
// Enable the fix for the is_user_edited bit once the frame is loaded.
|
||||
autofill::AutofillFormFeaturesJavaScriptFeature::GetInstance()
|
||||
->SetAutofillCorrectUserEditedBitInParsedField(WaitForMainFrame(),
|
||||
/*enabled=*/true);
|
||||
|
||||
// Emulate a user input on the first input element.
|
||||
EXPECT_NSEQ(@YES, ExecuteJavaScript(
|
||||
@"document.getElementById('input1').dispatchEvent(new "
|
||||
@"Event('input', { bubbles: true }))"));
|
||||
|
||||
// Verify that the first <input> element that received the scripted input
|
||||
// event has is_user_edited still set to false because the user input wasn't
|
||||
// trusted, and that the second <input> has is_user_edited set to false
|
||||
// because it didn't receive any user input event.
|
||||
NSString* verifying_javascript = @"!forms[0].fields[0].is_user_edited && "
|
||||
@"!forms[0].fields[1].is_user_edited;";
|
||||
EXPECT_NSEQ(
|
||||
@YES,
|
||||
ExecuteJavaScript([NSString
|
||||
stringWithFormat:@"var forms = "
|
||||
"__gCrWeb.autofill.extractNewForms(false); %@",
|
||||
verifying_javascript]));
|
||||
}
|
||||
|
||||
// Test that, when xframes is enabled, forms that do not have input fields but
|
||||
// have child frames are still extracted because their child frames may contain
|
||||
// input fields.
|
||||
|
@ -118,6 +118,7 @@ source_set("eg2_tests") {
|
||||
deps = [
|
||||
":eg_test_support+eg2",
|
||||
"//components/autofill/core/browser:test_support",
|
||||
"//components/autofill/ios/common",
|
||||
"//components/password_manager/core/browser/features:password_features",
|
||||
"//components/password_manager/core/common:features",
|
||||
"//components/sync/service",
|
||||
|
@ -8,8 +8,11 @@
|
||||
#import <tuple>
|
||||
|
||||
#import "base/strings/sys_string_conversions.h"
|
||||
#import "base/test/ios/wait_util.h"
|
||||
#import "base/time/time.h"
|
||||
#import "components/autofill/core/browser/test_utils/autofill_test_utils.h"
|
||||
#import "components/autofill/core/common/autofill_features.h"
|
||||
#import "components/autofill/ios/common/features.h"
|
||||
#import "components/password_manager/core/browser/features/password_features.h"
|
||||
#import "components/password_manager/core/common/password_manager_features.h"
|
||||
#import "components/sync/service/sync_prefs.h"
|
||||
@ -126,6 +129,19 @@ void CheckPasswordAutofillSuggestionAcceptedIndexMetricsCount(
|
||||
@"suggestion index.");
|
||||
}
|
||||
|
||||
// Slowly type characters using the keyboard by waiting between each tap.
|
||||
void SlowlyTypeText(NSString* text) {
|
||||
for (NSUInteger i = 0; i < [text length]; ++i) {
|
||||
// Wait some time before typing the character.
|
||||
base::test::ios::SpinRunLoopWithMinDelay(base::Milliseconds(200));
|
||||
// Type a single character so the user input can be effective.
|
||||
[ChromeEarlGrey
|
||||
simulatePhysicalKeyboardEvent:[text
|
||||
substringWithRange:NSMakeRange(i, 1)]
|
||||
flags:0];
|
||||
}
|
||||
}
|
||||
|
||||
} // namespace
|
||||
|
||||
@interface FormInputAccessoryEGTest : WebHttpServerChromeTestCase
|
||||
@ -188,6 +204,13 @@ void CheckPasswordAutofillSuggestionAcceptedIndexMetricsCount(
|
||||
config.features_enabled.push_back(
|
||||
autofill::features::kAutofillAcrossIframesIosThrottling);
|
||||
}
|
||||
if ([self isRunningTest:@selector
|
||||
(testFillCreditCardFieldsOnForm_WithUserEditedFix_UserEdited)] ||
|
||||
[self isRunningTest:@selector
|
||||
(testFillCreditCardFieldsOnForm_WithUserEditedFix_NotUserEdited)]) {
|
||||
config.features_enabled.push_back(
|
||||
kAutofillCorrectUserEditedBitInParsedField);
|
||||
}
|
||||
return config;
|
||||
}
|
||||
|
||||
@ -501,6 +524,67 @@ id<GREYMatcher> PaymentsBottomSheetUseKeyboardButton() {
|
||||
[AutofillAppInterface clearMockReauthenticationModule];
|
||||
}
|
||||
|
||||
// Tests that the fix on the is_user_edited bit in the parsed form fields is
|
||||
// effective in the case the user has edited the input field for real.
|
||||
- (void)testFillCreditCardFieldsOnForm_WithUserEditedFix_UserEdited {
|
||||
// Fill using another test. The CVC number won't be filled because a local
|
||||
// card is used.
|
||||
[self testFillCreditCardFieldsOnForm];
|
||||
|
||||
// Focus on the cvc field to fill it.
|
||||
[ChromeEarlGrey evaluateJavaScriptForSideEffect:
|
||||
@"document.getElementById('cvc').focus();"];
|
||||
// Wait some time so the keyboard has time to show up then slowly type the CVC
|
||||
// number.
|
||||
base::test::ios::SpinRunLoopWithMinDelay(base::Milliseconds(200));
|
||||
SlowlyTypeText(@"123");
|
||||
|
||||
// Submit so the perfect fill metric is recorded.
|
||||
[ChromeEarlGrey tapWebStateElementWithID:@"Submit"];
|
||||
|
||||
// Verify the perfect fill metric in a wait loop to let the time to the submit
|
||||
// event to be propagated down to the browser. A not perfect fill should be
|
||||
// recorded because the user has manually edited the CVC field which wasn't
|
||||
// filled.
|
||||
GREYAssertTrue(
|
||||
base::test::ios::WaitUntilConditionOrTimeout(
|
||||
base::Seconds(2),
|
||||
^() {
|
||||
return [MetricsAppInterface
|
||||
expectUniqueSampleWithCount:1
|
||||
forBucket:0
|
||||
forHistogram:@"Autofill.PerfectFilling."
|
||||
@"CreditCards"] == nil;
|
||||
}),
|
||||
@"Autofill.PerfectFilling.CreditCards verification failed");
|
||||
}
|
||||
|
||||
// Tests that the fix on the is_user_edited bit in the parsed form fields is
|
||||
// effective in the case the user didn't edit the fields that weren't filled.
|
||||
- (void)testFillCreditCardFieldsOnForm_WithUserEditedFix_NotUserEdited {
|
||||
// Fill using another test. The CVC number won't be filled because a local
|
||||
// card is used.
|
||||
[self testFillCreditCardFieldsOnForm];
|
||||
|
||||
// Submit so the perfect fill metric is recorded.
|
||||
[ChromeEarlGrey tapWebStateElementWithID:@"Submit"];
|
||||
|
||||
// Verify the perfect fill metric in a wait loop to let the time to the submit
|
||||
// event to be propagated down to the browser. A perfect fill should be
|
||||
// recorded because the user didn't edit the unfilled CVC field.
|
||||
GREYAssertTrue(
|
||||
base::test::ios::WaitUntilConditionOrTimeout(
|
||||
base::Seconds(2),
|
||||
^() {
|
||||
return [MetricsAppInterface
|
||||
expectUniqueSampleWithCount:1
|
||||
forBucket:1
|
||||
forHistogram:@"Autofill.PerfectFilling."
|
||||
@"CreditCards"] == nil;
|
||||
}),
|
||||
@"Autofill.PerfectFilling.CreditCards verification failed");
|
||||
}
|
||||
|
||||
// Tests that a xframe credit card form can be filled from the keyboard
|
||||
// accessory.
|
||||
- (void)testFillXframeCreditCardForm {
|
||||
|
Reference in New Issue
Block a user