[Autofill] Skip non-focusable fields in phone number rationalization.

Current phone number rationalization does not distinguish between
focusable and non-focusable fields. If a form has a hidden number field
then a visible number field, the first one will be considered as the
first complete number field, and the second field is not filled
automatically as a result.

We should skip the non-focusable fields in the rationalization process.

Bug: 782240
Change-Id: I71eeb613d3127770b2bf0a0bb120c7cca3282728
Reviewed-on: https://chromium-review.googlesource.com/757217
Commit-Queue: Hui Wu <wuandy@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515977}

components/autofill/core/browser/form_structure.cc

@@ -320,6 +320,8 @@ void RationalizePhoneNumberFieldPredictionsInSection(
   // valid set of fields that can compose a whole number. The |found_*| pointers
   // will be set to that set of fields when iteration finishes.
   for (AutofillField* field : *fields_in_section) {
+    if (!field->is_focusable)
+      continue;
     ServerFieldType current_field_type = field->Type().GetStorableType();
     switch (current_field_type) {
       case PHONE_HOME_NUMBER:

components/autofill/core/browser/form_structure_unittest.cc

@@ -29,28 +29,6 @@ using base::ASCIIToUTF16;
 
 namespace autofill {
 
-namespace content {
-
-std::ostream& operator<<(std::ostream& os, const FormData& form) {
-  os << base::UTF16ToUTF8(form.name)
-     << " "
-     << form.origin.spec()
-     << " "
-     << form.action.spec()
-     << " ";
-
-  for (std::vector<FormFieldData>::const_iterator iter =
-           form.fields.begin();
-       iter != form.fields.end(); ++iter) {
-    os << *iter
-       << " ";
-  }
-
-  return os;
-}
-
-}  // namespace content
-
 class FormStructureTest : public testing::Test {
  public:
   static std::string Hash64Bit(const std::string& str) {
@@ -63,9 +41,7 @@ class FormStructureTest : public testing::Test {
   }
 
  protected:
-  void DisableAutofillMetadataFieldTrial() {
-    field_trial_list_.reset();
-  }
+  void DisableAutofillMetadataFieldTrial() { field_trial_list_.reset(); }
 
  private:
   void EnableAutofillMetadataFieldTrial() {
@@ -434,7 +410,7 @@ TEST_F(FormStructureTest, HeuristicsContactInfo) {
   EXPECT_EQ(EMAIL_ADDRESS, form_structure->field(2)->heuristic_type());
   // Phone.
   EXPECT_EQ(PHONE_HOME_WHOLE_NUMBER,
-      form_structure->field(3)->heuristic_type());
+            form_structure->field(3)->heuristic_type());
   // Phone extension.
   EXPECT_EQ(PHONE_HOME_EXTENSION, form_structure->field(4)->heuristic_type());
   // Address.
@@ -1259,7 +1235,7 @@ TEST_F(FormStructureTest, HeuristicsSample8) {
   EXPECT_EQ(ADDRESS_HOME_COUNTRY, form_structure->field(7)->heuristic_type());
   // Phone.
   EXPECT_EQ(PHONE_HOME_WHOLE_NUMBER,
-      form_structure->field(8)->heuristic_type());
+            form_structure->field(8)->heuristic_type());
   // Submit.
   EXPECT_EQ(UNKNOWN_TYPE, form_structure->field(9)->heuristic_type());
 }
@@ -1380,7 +1356,7 @@ TEST_F(FormStructureTest, HeuristicsLabelsOnly) {
   EXPECT_EQ(EMAIL_ADDRESS, form_structure->field(2)->heuristic_type());
   // Phone.
   EXPECT_EQ(PHONE_HOME_WHOLE_NUMBER,
-      form_structure->field(3)->heuristic_type());
+            form_structure->field(3)->heuristic_type());
   // Address.
   EXPECT_EQ(ADDRESS_HOME_LINE1, form_structure->field(4)->heuristic_type());
   // Address Line 2.
@@ -1808,11 +1784,9 @@ TEST_F(FormStructureTest, ThreePartPhoneNumber) {
   // Area code.
   EXPECT_EQ(PHONE_HOME_CITY_CODE, form_structure->field(0)->heuristic_type());
   // Phone number suffix.
-  EXPECT_EQ(PHONE_HOME_NUMBER,
-            form_structure->field(1)->heuristic_type());
+  EXPECT_EQ(PHONE_HOME_NUMBER, form_structure->field(1)->heuristic_type());
   // Phone number suffix.
-  EXPECT_EQ(PHONE_HOME_NUMBER,
-            form_structure->field(2)->heuristic_type());
+  EXPECT_EQ(PHONE_HOME_NUMBER, form_structure->field(2)->heuristic_type());
   // Phone extension.
   EXPECT_EQ(PHONE_HOME_EXTENSION, form_structure->field(3)->heuristic_type());
 }
@@ -3300,8 +3274,9 @@ TEST_F(FormStructureTest, CheckMultipleTypes) {
 
   // Match last field as both address home line 1 and 2.
   possible_field_types[3].insert(ADDRESS_HOME_LINE2);
-  form_structure->field(form_structure->field_count() - 1)->set_possible_types(
-      possible_field_types[form_structure->field_count() - 1]);
+  form_structure->field(form_structure->field_count() - 1)
+      ->set_possible_types(
+          possible_field_types[form_structure->field_count() - 1]);
 
   // Adjust the expected upload proto.
   test::FillUploadField(upload.add_field(), 509334676U, "address", "text",
@@ -3319,8 +3294,9 @@ TEST_F(FormStructureTest, CheckMultipleTypes) {
   possible_field_types[3].clear();
   possible_field_types[3].insert(ADDRESS_HOME_LINE1);
   possible_field_types[3].insert(COMPANY_NAME);
-  form_structure->field(form_structure->field_count() - 1)->set_possible_types(
-      possible_field_types[form_structure->field_count() - 1]);
+  form_structure->field(form_structure->field_count() - 1)
+      ->set_possible_types(
+          possible_field_types[form_structure->field_count() - 1]);
 
   // Adjust the expected upload proto.
   upload.mutable_field(5)->set_autofill_type(60);
@@ -4758,7 +4734,6 @@ TEST_F(FormStructureTest,
   field.name = ASCIIToUTF16("fullName");
   // Needed to force multiple section in the form as the section identifying
   // logic only process fields with |is_focusable|=true.
-  field.is_focusable = true;
   form.fields.push_back(field);
 
   field.label = ASCIIToUTF16("Address");
@@ -4775,9 +4750,6 @@ TEST_F(FormStructureTest,
 
   field.label = ASCIIToUTF16("Full Name");
   field.name = ASCIIToUTF16("fullName1");
-  // Needed to force multiple section in the form as the section identifying
-  // logic only process fields with |is_focusable|=true.
-  field.is_focusable = true;
   form.fields.push_back(field);
 
   field.label = ASCIIToUTF16("Billing Address");
@@ -4890,6 +4862,93 @@ TEST_F(FormStructureTest,
   }
 }
 
+TEST_F(FormStructureTest,
+       ParseQueryResponse_SkipHiddenFieldsInPhoneNumberRationalization) {
+  FormData form;
+  form.origin = GURL("http://foo.com");
+  FormFieldData field;
+  field.form_control_type = "text";
+  field.max_length = 10000;
+
+  field.label = ASCIIToUTF16("Full Name");
+  field.name = ASCIIToUTF16("fullName");
+  form.fields.push_back(field);
+
+  field.label = ASCIIToUTF16("Address");
+  field.name = ASCIIToUTF16("address");
+  form.fields.push_back(field);
+
+  field.label = ASCIIToUTF16("");
+  field.name = ASCIIToUTF16("phoneNumberMask");
+  field.is_focusable = false;
+  form.fields.push_back(field);
+
+  // This should be considered as the first number in the form since
+  // previous one is not focusable.
+  field.label = ASCIIToUTF16("PhoneNumber");
+  field.name = ASCIIToUTF16("bPhoneNumber");
+  field.is_focusable = true;  // Setting it back to true.
+  form.fields.push_back(field);
+
+  AutofillQueryResponseContents response;
+  response.add_field()->set_overall_type_prediction(NAME_FULL);
+  response.add_field()->set_overall_type_prediction(
+      ADDRESS_HOME_STREET_ADDRESS);
+  response.add_field()->set_overall_type_prediction(PHONE_HOME_CITY_AND_NUMBER);
+  response.add_field()->set_overall_type_prediction(PHONE_HOME_CITY_AND_NUMBER);
+
+  std::string response_string;
+  ASSERT_TRUE(response.SerializeToString(&response_string));
+
+  // Verify phone number in different section still gets autofilled.
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeature(
+        autofill::kAutofillRationalizeFieldTypePredictions);
+
+    FormStructure form_structure(form);
+    std::vector<FormStructure*> forms;
+    forms.push_back(&form_structure);
+    FormStructure::ParseQueryResponse(response_string, forms);
+    ASSERT_EQ(1U, forms.size());
+    ASSERT_EQ(4U, forms[0]->field_count());
+    EXPECT_EQ(NAME_FULL, forms[0]->field(0)->overall_server_type());
+    EXPECT_EQ(ADDRESS_HOME_STREET_ADDRESS,
+              forms[0]->field(1)->overall_server_type());
+
+    EXPECT_EQ(PHONE_HOME_CITY_AND_NUMBER,
+              forms[0]->field(2)->overall_server_type());
+    EXPECT_TRUE(forms[0]->field(2)->only_fill_when_focused());
+    EXPECT_EQ(PHONE_HOME_CITY_AND_NUMBER,
+              forms[0]->field(3)->overall_server_type());
+    EXPECT_FALSE(forms[0]->field(3)->only_fill_when_focused());
+  }
+
+  // Sanity check that the enable/disabled works. Same output as when the
+  // feature is on.
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndDisableFeature(
+        autofill::kAutofillRationalizeFieldTypePredictions);
+
+    FormStructure form_structure(form);
+    std::vector<FormStructure*> forms;
+    forms.push_back(&form_structure);
+    FormStructure::ParseQueryResponse(response_string, forms);
+
+    EXPECT_EQ(NAME_FULL, forms[0]->field(0)->overall_server_type());
+    EXPECT_EQ(ADDRESS_HOME_STREET_ADDRESS,
+              forms[0]->field(1)->overall_server_type());
+
+    EXPECT_EQ(PHONE_HOME_CITY_AND_NUMBER,
+              forms[0]->field(2)->overall_server_type());
+    EXPECT_FALSE(forms[0]->field(2)->only_fill_when_focused());
+    EXPECT_EQ(PHONE_HOME_CITY_AND_NUMBER,
+              forms[0]->field(3)->overall_server_type());
+    EXPECT_FALSE(forms[0]->field(3)->only_fill_when_focused());
+  }
+}
+
 TEST_F(FormStructureTest, FindLongestCommonPrefix) {
   // Normal case: All strings are longer than threshold; some are common.
   std::vector<base::string16> strings;

components/autofill/core/common/form_field_data.cc

@@ -125,7 +125,7 @@ FormFieldData::FormFieldData()
     : max_length(0),
       is_autofilled(false),
       check_status(NOT_CHECKABLE),
-      is_focusable(false),
+      is_focusable(true),
       should_autocomplete(true),
       role(ROLE_ATTRIBUTE_OTHER),
       text_direction(base::i18n::UNKNOWN_DIRECTION),
@@ -133,8 +133,7 @@ FormFieldData::FormFieldData()
 
 FormFieldData::FormFieldData(const FormFieldData& other) = default;
 
-FormFieldData::~FormFieldData() {
-}
+FormFieldData::~FormFieldData() {}
 
 bool FormFieldData::SameFieldAs(const FormFieldData& field) const {
   // A FormFieldData stores a value, but the value is not part of the identity
@@ -184,33 +183,59 @@ bool FormFieldData::operator<(const FormFieldData& field) const {
   // context.)
 
   // Like SameFieldAs this ignores the value.
-  if (label < field.label) return true;
-  if (label > field.label) return false;
-  if (name < field.name) return true;
-  if (name > field.name) return false;
-  if (id < field.id) return true;
-  if (id > field.id) return false;
-  if (form_control_type < field.form_control_type) return true;
-  if (form_control_type > field.form_control_type) return false;
-  if (autocomplete_attribute < field.autocomplete_attribute) return true;
-  if (autocomplete_attribute > field.autocomplete_attribute) return false;
-  if (placeholder < field.placeholder) return true;
-  if (placeholder > field.placeholder) return false;
-  if (max_length < field.max_length) return true;
-  if (max_length > field.max_length) return false;
-  if (css_classes < field.css_classes) return true;
-  if (css_classes > field.css_classes) return false;
+  if (label < field.label)
+    return true;
+  if (label > field.label)
+    return false;
+  if (name < field.name)
+    return true;
+  if (name > field.name)
+    return false;
+  if (id < field.id)
+    return true;
+  if (id > field.id)
+    return false;
+  if (form_control_type < field.form_control_type)
+    return true;
+  if (form_control_type > field.form_control_type)
+    return false;
+  if (autocomplete_attribute < field.autocomplete_attribute)
+    return true;
+  if (autocomplete_attribute > field.autocomplete_attribute)
+    return false;
+  if (placeholder < field.placeholder)
+    return true;
+  if (placeholder > field.placeholder)
+    return false;
+  if (max_length < field.max_length)
+    return true;
+  if (max_length > field.max_length)
+    return false;
+  if (css_classes < field.css_classes)
+    return true;
+  if (css_classes > field.css_classes)
+    return false;
   // Skip |is_checked| and |is_autofilled| as in SameFieldAs.
-  if (IsCheckable(check_status) < IsCheckable(field.check_status)) return true;
-  if (IsCheckable(check_status) > IsCheckable(field.check_status)) return false;
-  if (is_focusable < field.is_focusable) return true;
-  if (is_focusable > field.is_focusable) return false;
-  if (should_autocomplete < field.should_autocomplete) return true;
-  if (should_autocomplete > field.should_autocomplete) return false;
-  if (role < field.role) return true;
-  if (role > field.role) return false;
-  if (text_direction < field.text_direction) return true;
-  if (text_direction > field.text_direction) return false;
+  if (IsCheckable(check_status) < IsCheckable(field.check_status))
+    return true;
+  if (IsCheckable(check_status) > IsCheckable(field.check_status))
+    return false;
+  if (is_focusable < field.is_focusable)
+    return true;
+  if (is_focusable > field.is_focusable)
+    return false;
+  if (should_autocomplete < field.should_autocomplete)
+    return true;
+  if (should_autocomplete > field.should_autocomplete)
+    return false;
+  if (role < field.role)
+    return true;
+  if (role > field.role)
+    return false;
+  if (text_direction < field.text_direction)
+    return true;
+  if (text_direction > field.text_direction)
+    return false;
   // See SameFieldAs above for why we don't check option_values/contents.
   return false;
 }