Skip to content

GitLab

T
tangled
Commit e93874be authored by Moe Ahmadi's avatar Moe Ahmadi Committed by Commit Bot
Browse files
Options

[AF] Do not discard form data after dynamic form submission 

Prior to this change form data were being discarded after a dynamic form
submission. This was because the dynamically changed form was being added
to the cache with its user entered/autofilled values. At the time of
submission, form data were being discarded as they were thought to be the
initial values of the fields at page load. This CL changes
AutofillHandler::OnFormsSeen (which is called on dynamic form changes) to
attempt to find a previously cached version of the form first. Prior to
addition to the cache, form values are overridden by the cached values.
This prevents those values from getting confused with the initial form
values at the time of submission.

Bug: 863954

Change-Id: Iba4771ee87b9777e66a6b10b6b369379dc123a46
Reviewed-on: https://chromium-review.googlesource.com/1137021
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: default avatarSebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577033}
parent 4b39a10c
Hide whitespace changes
Inline Side-by-side
Showing with 119 additions and 7 deletions
chrome/browser/autofill/autofill_interactive_uitest.cc
View file @ e93874be
...@@ -2913,6 +2913,35 @@ IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest, ...@@ -2913,6 +2913,35 @@ IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest,
ExpectFieldValue("phone", "15125551234"); ExpectFieldValue("phone", "15125551234");
} }
// Test that form data gets saved after submitting dynamically changing form.
IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest,
Submit_DynamicChangingFormFill) {
CreateTestProfile();
GURL url = embedded_test_server()->GetURL(
"a.com", "/autofill/dynamic_form_new_field.html");
ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
TriggerFormFill("firstname");
// Wait for the re-fill to happen.
bool has_refilled = false;
ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
GetWebContents(), "hasRefilled()", &has_refilled));
ASSERT_TRUE(has_refilled);
// Edit the company field.
ASSERT_TRUE(content::ExecuteScript(
GetWebContents(), "document.getElementById('company').value = 'NASA';"));
// Submit the form.
ASSERT_TRUE(content::ExecuteScript(
GetWebContents(), "document.getElementById('form1').submit();"));
// A new autofill profile gets added.
ASSERT_EQ(2u, GetProfiles(browser()).size());
}
INSTANTIATE_TEST_CASE_P(All, AutofillInteractiveTest, testing::Bool()); INSTANTIATE_TEST_CASE_P(All, AutofillInteractiveTest, testing::Bool());
INSTANTIATE_TEST_CASE_P(All, INSTANTIATE_TEST_CASE_P(All,
AutofillCreditCardInteractiveTest, AutofillCreditCardInteractiveTest,
......
chrome/browser/autofill/autofill_uitest_util.cc
View file @ e93874be
...@@ -99,4 +99,12 @@ void AddTestAutofillData(Browser* browser, ...@@ -99,4 +99,12 @@ void AddTestAutofillData(Browser* browser,
observer.Wait(); observer.Wait();
} }
std::vector<AutofillProfile*> GetProfiles(Browser* browser) {
// Wait for asynchronous updates to PersonalDataManager to complete.
PdmChangeWaiter observer(browser);
observer.Wait();
return GetPersonalDataManager(browser->profile())->GetProfiles();
}
} // namespace autofill } // namespace autofill
chrome/browser/autofill/autofill_uitest_util.h
View file @ e93874be
...@@ -21,6 +21,7 @@ void AddTestCreditCard(Browser* browser, const CreditCard& card); ...@@ -21,6 +21,7 @@ void AddTestCreditCard(Browser* browser, const CreditCard& card);
void AddTestAutofillData(Browser* browser, void AddTestAutofillData(Browser* browser,
const AutofillProfile& profile, const AutofillProfile& profile,
const CreditCard& card); const CreditCard& card);
std::vector<AutofillProfile*> GetProfiles(Browser* browser);
} // namespace autofill } // namespace autofill
#endif // CHROME_BROWSER_AUTOFILL_AUTOFILL_UITEST_UTIL_H_ #endif // CHROME_BROWSER_AUTOFILL_AUTOFILL_UITEST_UTIL_H_
chrome/test/data/autofill/dynamic_form_new_field.html 0 → 100644
View file @ e93874be
<!-- A page that is used to test that a dynamic form fill feature works properly. -->
<body>
<form name="addr1.1" id="form1" action="https://example.com/" method="post">
Name: <input type="text" name="firstname" id="firstname"><br>
Address: <input type="text" name="address1" id="address1"><br>
City: <input type="text" name="city" id="city"><br>
State: <select name="state" id="state">
<option value="CA">CA</option>
<option value="MA">MA</option>
<option value="NY">NY</option>
<option value="MD">MD</option>
<option value="OR">OR</option>
<option value="OH">OH</option>
<option value="IL">IL</option>
<option value="DC">DC</option>
</select> <br>
Country: <select name="country" id="country" onchange="CountryChanged()">
<option value="CA">Canada</option>
<option value="US">United States</option>
</select> <br>
Company: <input name="company" id="company"> <br>
Email: <input name="email" id="email"> <br>
Phone: <input name="phone" id="phone"> <br>
<input type="reset" value="Reset">
<input type="submit" value="Submit" id="profile_submit">
</form>
</body>
<script src="dynamic_form_utils.js"></script>
<script>
var notify_on_first_name_input_change = false;
var first_name_input_changed = false;
function CountryChanged() {
form = document.getElementById('form1');
var first_name_input = form.elements[0];
first_name_input.value = '';
first_name_input.onchange = function() {
if (notify_on_first_name_input_change)
window.domAutomationController.send(first_name_input.value != '');
else
first_name_input_changed = true;
}
var zip_input = document.createElement('input');
zip_input.setAttribute('type', 'text');
zip_input.setAttribute('name', 'zip');
zip_input.setAttribute('id', 'zip');
form.insertBefore(zip_input, form.elements[form.elements.length - 2]);
}
function hasRefilled() {
var first_name_input = document.getElementById('firstname');
if (first_name_input && first_name_input_changed) {
window.domAutomationController.send(first_name_input.value != '');
} else {
notify_on_first_name_input_change = true;
}
}
</script>
components/autofill/core/browser/autofill_handler.cc
View file @ e93874be
...@@ -51,7 +51,10 @@ void AutofillHandler::OnFormsSeen(const std::vector<FormData>& forms, ...@@ -51,7 +51,10 @@ void AutofillHandler::OnFormsSeen(const std::vector<FormData>& forms,
for (const FormData& form : forms) { for (const FormData& form : forms) {
const auto parse_form_start_time = TimeTicks::Now(); const auto parse_form_start_time = TimeTicks::Now();
FormStructure* form_structure = nullptr; FormStructure* form_structure = nullptr;
if (!ParseForm(form, /*cached_form=*/nullptr, &form_structure)) // Try to find the FormStructure that corresponds to |form|.
// |form_structure| may still be nullptr after this call.
ignore_result(FindCachedForm(form, &form_structure));
if (!ParseForm(form, form_structure, &form_structure))
continue; continue;
DCHECK(form_structure); DCHECK(form_structure);
if (form_structure == nullptr) if (form_structure == nullptr)
...@@ -182,6 +185,7 @@ bool AutofillHandler::ParseForm(const FormData& form, ...@@ -182,6 +185,7 @@ bool AutofillHandler::ParseForm(const FormData& form,
// We need to keep the server data if available. We need to use them while // We need to keep the server data if available. We need to use them while
// determining the heuristics. // determining the heuristics.
form_structure->RetrieveFromCache(*cached_form, form_structure->RetrieveFromCache(*cached_form,
/*apply_value=*/true,
/*apply_is_autofilled=*/true, /*apply_is_autofilled=*/true,
/*only_server_and_autofill_state=*/true); /*only_server_and_autofill_state=*/true);
} }
......
components/autofill/core/browser/autofill_manager.cc
View file @ e93874be
...@@ -1403,6 +1403,7 @@ std::unique_ptr<FormStructure> AutofillManager::ValidateSubmittedForm( ...@@ -1403,6 +1403,7 @@ std::unique_ptr<FormStructure> AutofillManager::ValidateSubmittedForm(
return std::unique_ptr<FormStructure>(); return std::unique_ptr<FormStructure>();
submitted_form->RetrieveFromCache(*cached_submitted_form, submitted_form->RetrieveFromCache(*cached_submitted_form,
/*apply_value=*/false,
/*apply_is_autofilled=*/false, /*apply_is_autofilled=*/false,
/*only_server_and_autofill_state=*/false); /*only_server_and_autofill_state=*/false);
return submitted_form; return submitted_form;
......
components/autofill/core/browser/form_structure.cc
View file @ e93874be
...@@ -730,6 +730,7 @@ bool FormStructure::ShouldBeUploaded() const { ...@@ -730,6 +730,7 @@ bool FormStructure::ShouldBeUploaded() const {
void FormStructure::RetrieveFromCache( void FormStructure::RetrieveFromCache(
const FormStructure& cached_form, const FormStructure& cached_form,
const bool apply_value,
const bool apply_is_autofilled, const bool apply_is_autofilled,
const bool only_server_and_autofill_state) { const bool only_server_and_autofill_state) {
// Map from field signatures to cached fields. // Map from field signatures to cached fields.
...@@ -754,11 +755,14 @@ void FormStructure::RetrieveFromCache( ...@@ -754,11 +755,14 @@ void FormStructure::RetrieveFromCache(
if (apply_is_autofilled) { if (apply_is_autofilled) {
field->is_autofilled = cached_field->second->is_autofilled; field->is_autofilled = cached_field->second->is_autofilled;
} }
if (field->form_control_type != "select-one" && if (field->form_control_type != "select-one") {
field->value == cached_field->second->value) { if (apply_value) {
// From the perspective of learning user data, text fields containing field->value = cached_field->second->value;
// default values are equivalent to empty fields. } else if (field->value == cached_field->second->value) {
field->value = base::string16(); // From the perspective of learning user data, text fields containing
// default values are equivalent to empty fields.
field->value = base::string16();
}
} }
field->set_server_type(cached_field->second->server_type()); field->set_server_type(cached_field->second->server_type());
field->set_previously_autofilled( field->set_previously_autofilled(
......
components/autofill/core/browser/form_structure.h
View file @ e93874be
...@@ -132,8 +132,10 @@ class FormStructure { ...@@ -132,8 +132,10 @@ class FormStructure {
// server. // server.
bool ShouldBeUploaded() const; bool ShouldBeUploaded() const;
// Sets the field types to be those set for |cached_form|. // Sets the field types to be those set for |cached_form|. if |apply_value| is
// true, value is copied over from |cached_form| for non-select elements only.
void RetrieveFromCache(const FormStructure& cached_form, void RetrieveFromCache(const FormStructure& cached_form,
const bool apply_value,
const bool apply_is_autofilled, const bool apply_is_autofilled,
const bool only_server_and_autofill_state); const bool only_server_and_autofill_state);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment