Improve label behaviour for the Contact Picker API

A "names" label has been added that enables the user to toggle
whether their contact names should be included in the data shared with
the website that opened the picker.

Secondly, chips for data types that have not been requested by the
developer will now be hidden. At least one type is required so we can be
certain that there is *a* chip. A LinearLayout had to be introduced to
make sure left-side spacing is consistent: previously, the first chip
(names) had a 20dp padding where others had a 10dp padding, but now any
of the chips can be first.

Urgent string updates (Issue 989208) are also included.

Bug: 985708, 988268, 989208
Change-Id: I054871b5c059a029450fd11fad961847b4e3e424
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726035Reviewed-by: Richard Knoll <knollr@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Auto-Submit: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682678}

chrome/android/java/res/layout/top_view.xml

@@ -18,28 +18,43 @@
         android:layout_marginStart="20dp"
         android:layout_marginEnd="16dp" />
 
-    <org.chromium.ui.widget.ChipView
-        android:id="@+id/email_filter"
-        android:gravity="center"
+    <LinearLayout
+        android:id="@+id/chip_container"
         android:layout_below="@id/explanation"
+        android:layout_width="match_parent"
         android:layout_height="wrap_content"
-        android:layout_marginStart="20dp"
-        android:layout_width="wrap_content"
-        style="@style/SuggestionChip" />
+        android:orientation="horizontal"
+        android:paddingStart="10dp">
 
-    <org.chromium.ui.widget.ChipView
-        android:id="@+id/tel_filter"
-        android:gravity="center"
-        android:layout_below="@id/explanation"
-        android:layout_toEndOf="@id/email_filter"
-        android:layout_height="wrap_content"
-        android:layout_marginStart="10dp"
-        android:layout_width="wrap_content"
-        style="@style/SuggestionChip" />
+        <org.chromium.ui.widget.ChipView
+            android:id="@+id/names_filter"
+            android:gravity="center"
+            android:layout_height="wrap_content"
+            android:layout_marginStart="10dp"
+            android:layout_width="wrap_content"
+            style="@style/SuggestionChip" />
+
+        <org.chromium.ui.widget.ChipView
+            android:id="@+id/email_filter"
+            android:gravity="center"
+            android:layout_height="wrap_content"
+            android:layout_marginStart="10dp"
+            android:layout_width="wrap_content"
+            style="@style/SuggestionChip" />
+
+        <org.chromium.ui.widget.ChipView
+            android:id="@+id/tel_filter"
+            android:gravity="center"
+            android:layout_height="wrap_content"
+            android:layout_marginStart="10dp"
+            android:layout_width="wrap_content"
+            style="@style/SuggestionChip" />
+
+    </LinearLayout>
 
     <LinearLayout
         android:id="@+id/content"
-        android:layout_below="@id/email_filter"
+        android:layout_below="@id/chip_container"
         android:visibility="gone"
         style="@style/ListItemContainer">
 

chrome/android/java/src/org/chromium/chrome/browser/contacts_picker/PickerAdapter.java

@@ -52,10 +52,11 @@ public class PickerAdapter extends Adapter<RecyclerView.ViewHolder>
      * The types of filters supported.
      */
     @Retention(RetentionPolicy.SOURCE)
-    @IntDef({FilterType.EMAILS, FilterType.TELEPHONES})
+    @IntDef({FilterType.NAMES, FilterType.EMAILS, FilterType.TELEPHONES})
     public @interface FilterType {
-        int EMAILS = 0;
-        int TELEPHONES = 1;
+        int NAMES = 0;
+        int EMAILS = 1;
+        int TELEPHONES = 2;
     }
 
     /**
@@ -92,6 +93,9 @@ public class PickerAdapter extends Adapter<RecyclerView.ViewHolder>
     // A list of search result indices into the larger data set.
     private ArrayList<Integer> mSearchResults;
 
+    // Whether to include names in the returned results.
+    private static boolean sIncludeNames;
+
     // Whether to include emails in the returned results.
     private static boolean sIncludeEmails;
 
@@ -112,6 +116,7 @@ public class PickerAdapter extends Adapter<RecyclerView.ViewHolder>
         mCategoryView = categoryView;
         mContentResolver = contentResolver;
         mFormattedOrigin = formattedOrigin;
+        sIncludeNames = true;
         sIncludeEmails = true;
         sIncludeTelephones = true;
 
@@ -198,6 +203,8 @@ public class PickerAdapter extends Adapter<RecyclerView.ViewHolder>
                 mTopView.registerSelectAllCallback(mCategoryView);
                 mTopView.registerChipToggledCallback(this);
                 mTopView.updateCheckboxVisibility(mCategoryView.multiSelectionAllowed());
+                mTopView.updateChipVisibility(mCategoryView.includeNames,
+                        mCategoryView.includeEmails, mCategoryView.includeTel);
                 mCategoryView.setTopView(mTopView);
                 if (mContactDetails != null) mTopView.updateContactCount(mContactDetails.size());
                 return new TopViewHolder(mTopView);
@@ -246,16 +253,30 @@ public class PickerAdapter extends Adapter<RecyclerView.ViewHolder>
 
     @Override
     public void onChipToggled(@FilterType int chip) {
-        if (chip == FilterType.EMAILS) {
-            sIncludeEmails = !sIncludeEmails;
-        } else if (chip == FilterType.TELEPHONES) {
-            sIncludeTelephones = !sIncludeTelephones;
-        } else {
-            assert false;
+        switch (chip) {
+            case FilterType.NAMES:
+                sIncludeNames = !sIncludeNames;
+                break;
+            case FilterType.EMAILS:
+                sIncludeEmails = !sIncludeEmails;
+                break;
+            case FilterType.TELEPHONES:
+                sIncludeTelephones = !sIncludeTelephones;
+                break;
+            default:
+                assert false;
         }
+
         notifyDataSetChanged();
     }
 
+    /**
+     * Returns true unless the adapter is filtering out names.
+     */
+    public static boolean includesNames() {
+        return sIncludeNames;
+    }
+
     /**
      * Returns true unless the adapter is filtering out emails.
      */

chrome/android/java/src/org/chromium/chrome/browser/contacts_picker/PickerCategoryView.java

@@ -341,7 +341,8 @@ public class PickerCategoryView extends RelativeLayout
 
         for (ContactDetails contactDetails : selectedContacts) {
             contacts.add(new ContactsPickerListener.Contact(
-                    getContactPropertyValues(includeNames, true, contactDetails.getDisplayNames()),
+                    getContactPropertyValues(includeNames, PickerAdapter.includesNames(),
+                            contactDetails.getDisplayNames()),
                     getContactPropertyValues(includeEmails, PickerAdapter.includesEmails(),
                             contactDetails.getEmails()),
                     getContactPropertyValues(includeTel, PickerAdapter.includesTelephones(),

chrome/android/java/src/org/chromium/chrome/browser/contacts_picker/TopView.java

@@ -61,6 +61,9 @@ public class TopView extends RelativeLayout
     // The callback to use when notifying that the Select All checkbox was toggled.
     private SelectAllToggleCallback mSelectAllCallback;
 
+    // A Chip for filtering out names.
+    private ChipView mNamesFilterChip;
+
     // A Chip for filtering out emails.
     private ChipView mEmailFilterChip;
 
@@ -92,8 +95,14 @@ public class TopView extends RelativeLayout
         TextView title = findViewById(R.id.checkbox_title);
         title.setText(R.string.contacts_picker_all_contacts);
 
+        mNamesFilterChip = findViewById(R.id.names_filter);
+        TextView textView = mNamesFilterChip.getPrimaryTextView();
+        textView.setText(R.string.top_view_names_filter_label);
+        mNamesFilterChip.setSelected(true);
+        mNamesFilterChip.setOnClickListener(this);
+
         mEmailFilterChip = findViewById(R.id.email_filter);
-        TextView textView = mEmailFilterChip.getPrimaryTextView();
+        textView = mEmailFilterChip.getPrimaryTextView();
         textView.setText(R.string.top_view_email_filter_label);
         mEmailFilterChip.setSelected(true);
         mEmailFilterChip.setOnClickListener(this);
@@ -108,7 +117,9 @@ public class TopView extends RelativeLayout
     @Override
     public void onClick(View view) {
         int id = view.getId();
-        if (id == R.id.email_filter) {
+        if (id == R.id.names_filter) {
+            notifyChipToggled(PickerAdapter.FilterType.NAMES);
+        } else if (id == R.id.email_filter) {
             notifyChipToggled(PickerAdapter.FilterType.EMAILS);
         } else if (id == R.id.tel_filter) {
             notifyChipToggled(PickerAdapter.FilterType.TELEPHONES);
@@ -120,8 +131,23 @@ public class TopView extends RelativeLayout
      * @param chip The id of the chip that was toggled.
      */
     public void notifyChipToggled(@PickerAdapter.FilterType int chip) {
-        ChipView chipView =
-                chip == PickerAdapter.FilterType.EMAILS ? mEmailFilterChip : mTelephonesFilterChip;
+        ChipView chipView;
+
+        switch (chip) {
+            case PickerAdapter.FilterType.NAMES:
+                chipView = mNamesFilterChip;
+                break;
+            case PickerAdapter.FilterType.EMAILS:
+                chipView = mEmailFilterChip;
+                break;
+            case PickerAdapter.FilterType.TELEPHONES:
+                chipView = mTelephonesFilterChip;
+                break;
+            default:
+                assert false;
+                return;
+        }
+
         chipView.setSelected(!chipView.isSelected());
         mChipToggledCallback.onChipToggled(chip);
     }
@@ -165,6 +191,19 @@ public class TopView extends RelativeLayout
         }
     }
 
+    /**
+     * Updates which chips should be displayed as part of the top view.
+     * @param shouldDisplayNames Whether the names chip should be displayed.
+     * @param shouldDisplayEmails Whether the emails chip should be displayed.
+     * @param shouldDisplayTel Whether the telephone chip should be displayed.
+     */
+    public void updateChipVisibility(
+            boolean shouldDisplayNames, boolean shouldDisplayEmails, boolean shouldDisplayTel) {
+        mNamesFilterChip.setVisibility(shouldDisplayNames ? View.VISIBLE : View.GONE);
+        mEmailFilterChip.setVisibility(shouldDisplayEmails ? View.VISIBLE : View.GONE);
+        mTelephonesFilterChip.setVisibility(shouldDisplayTel ? View.VISIBLE : View.GONE);
+    }
+
     /**
      * Updates the total number of contacts found in the dialog.
      * @param count The number of contacts found.

chrome/android/java/strings/android_chrome_strings.grd

@@ -3513,11 +3513,14 @@ To change this setting, <ph name="BEGIN_LINK">&lt;resetlink&gt;</ph>reset sync<p
       <message name="IDS_CONTACTS_PICKER_NO_CONTACTS_FOUND" desc="The label shown when no contacts are found (e.g. none exist on the device).">
         No contacts found
       </message>
+      <message name="IDS_TOP_VIEW_NAMES_FILTER_LABEL" desc="The label shown for the names filter toggle button (allowing the user to exclude names).">
+        Names
+      </message>
       <message name="IDS_TOP_VIEW_EMAIL_FILTER_LABEL" desc="The label shown for the email filter toggle button (allowing the user to exclude emails).">
-        Emails
+        Email addresses
       </message>
       <message name="IDS_TOP_VIEW_TELEPHONE_FILTER_LABEL" desc="The label shown for the telephone filter toggle button (allowing the user to exclude emails).">
-        Telephones
+        Phone numbers
       </message>
       <message name="IDS_CONTACTS_PICKER_MORE_DETAILS" desc="Label describing that the user has one or more telephone/emails (used for either). The leading space is a word separator, for those languages that use space as a separator.">
         {DETAIL_COUNT, plural,

chrome/android/javatests/src/org/chromium/chrome/browser/contacts_picker/ContactsPickerDialogTest.java

@@ -235,6 +235,16 @@ public class ContactsPickerDialogTest
         TestThreadUtils.runOnUiThreadBlocking(() -> mDialog.dismiss());
     }
 
+    private TopView getTopView() throws Exception {
+        RecyclerView recyclerView = getRecyclerView();
+        RecyclerViewTestUtils.waitForView(recyclerView, 0);
+        View view = recyclerView.getLayoutManager().findViewByPosition(0);
+        Assert.assertNotNull(view);
+        Assert.assertTrue(view instanceof TopView);
+
+        return (TopView) view;
+    }
+
     @Test
     @LargeTest
     public void testOriginString() throws Throwable {
@@ -243,13 +253,9 @@ public class ContactsPickerDialogTest
                 /* includeTel = */ true);
         Assert.assertTrue(mDialog.isShowing());
 
-        RecyclerView recyclerView = getRecyclerView();
-        RecyclerViewTestUtils.waitForView(recyclerView, 0);
-        View view = recyclerView.getLayoutManager().findViewByPosition(0);
-        Assert.assertNotNull(view);
-        Assert.assertTrue(view instanceof TopView);
-        TopView topView = (TopView) view;
+        TopView topView = getTopView();
         Assert.assertNotNull(topView);
+
         TextView explanation = (TextView) topView.findViewById(R.id.explanation);
         Assert.assertNotNull(explanation);
         Assert.assertEquals(explanation.getText().toString(),
@@ -258,6 +264,31 @@ public class ContactsPickerDialogTest
         dismissDialog();
     }
 
+    @Test
+    @LargeTest
+    public void testFilterVisibilityForDataInclusion() throws Throwable {
+        createDialog(/* multiselect = */ false, /* includeNames = */ true,
+                /* includeEmails = */ false,
+                /* includeTel = */ true);
+        Assert.assertTrue(mDialog.isShowing());
+
+        TopView topView = getTopView();
+        Assert.assertNotNull(topView);
+
+        View namesFilter = topView.findViewById(R.id.names_filter);
+        Assert.assertNotNull(namesFilter);
+        View emailFilter = topView.findViewById(R.id.email_filter);
+        Assert.assertNotNull(emailFilter);
+        View telFilter = topView.findViewById(R.id.tel_filter);
+        Assert.assertNotNull(telFilter);
+
+        // Per configuration given in the createDialog() call, the names and telephone filters
+        // should be visible, but the e-mail filter should be gone.
+        Assert.assertEquals(namesFilter.getVisibility(), View.VISIBLE);
+        Assert.assertEquals(emailFilter.getVisibility(), View.GONE);
+        Assert.assertEquals(telFilter.getVisibility(), View.VISIBLE);
+    }
+
     @Test
     @LargeTest
     public void testNoSelection() throws Throwable {
@@ -325,6 +356,29 @@ public class ContactsPickerDialogTest
         dismissDialog();
     }
 
+    @Test
+    @LargeTest
+    public void testNamesRemoved() throws Throwable {
+        createDialog(/* multiselect = */ false, /* includeNames = */ true,
+                /* includeEmails = */ true,
+                /* includeTel = */ true);
+        Assert.assertTrue(mDialog.isShowing());
+
+        toggleFilter(PickerAdapter.FilterType.NAMES);
+
+        int expectedSelectionCount = 1;
+        clickView(0, expectedSelectionCount, /* expectSelection = */ true);
+        clickDone();
+
+        Assert.assertEquals(ContactsPickerAction.CONTACTS_SELECTED, mLastActionRecorded);
+        Assert.assertEquals(1, mLastSelectedContacts.size());
+        Assert.assertEquals(new ArrayList<String>(), mLastSelectedContacts.get(0).names);
+        Assert.assertEquals(mTestContacts.get(0).getEmails().get(0),
+                mLastSelectedContacts.get(0).emails.get(0));
+
+        dismissDialog();
+    }
+
     @Test
     @LargeTest
     public void testEmailsRemoved() throws Throwable {