[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the
original item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*

Review-Url: https://codereview.chromium.org/2561373002
Cr-Commit-Position: refs/heads/master@{#437723}

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

@@ -2,9 +2,10 @@
 <!-- Copyright 2016 The Chromium Authors. All rights reserved.
      Use of this source code is governed by a BSD-style license that can be
      found in the LICENSE file. -->
-     
+
 <LinearLayout
         xmlns:android="http://schemas.android.com/apk/res/android"
+        android:id="@+id/payments_open_editor_pencil_button"
         android:layout_width="wrap_content"
         android:layout_height="wrap_content"
         android:contentDescription="@string/payments_edit_button">
@@ -25,4 +26,4 @@
             android:src="@drawable/bookmark_edit_normal"
             android:contentDescription="@null"
             android:importantForAccessibility="no" />
-</LinearLayout>
\ No newline at end of file
+</LinearLayout>

chrome/android/java/res/values/ids.xml

@@ -67,7 +67,7 @@
     <item type="id" name="payments_edit_cancel_button" />
     <item type="id" name="payments_edit_done_button" />
     <item type="id" name="payments_edit_checkbox" />
-    
+
     <!-- Password update prompt -->
     <item type="id" name="password_infobar_accounts_spinner" />
 </resources>

chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java

@@ -62,7 +62,8 @@ public class AddressEditor extends EditorBase<AutofillAddress> {
      * [ phone number field ] <----- phone is always present and required.
      */
     @Override
-    public void edit(@Nullable AutofillAddress toEdit, final Callback<AutofillAddress> callback) {
+    public void edit(
+            @Nullable final AutofillAddress toEdit, final Callback<AutofillAddress> callback) {
         super.edit(toEdit, callback);
 
         if (mAutofillProfileBridge == null) mAutofillProfileBridge = new AutofillProfileBridge();
@@ -167,11 +168,12 @@ public class AddressEditor extends EditorBase<AutofillAddress> {
         mPhoneField.setValue(profile.getPhoneNumber());
         editor.addField(mPhoneField);
 
-        // If the user clicks [Cancel], send a null address back to the caller.
+        // If the user clicks [Cancel], send |toEdit| address back to the caller, which was the
+        // original state (could be null, a complete address, a partial address).
         editor.setCancelCallback(new Runnable() {
             @Override
             public void run() {
-                callback.onResult(null);
+                callback.onResult(toEdit);
             }
         });
 

chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java

@@ -293,11 +293,12 @@ public class CardEditor extends EditorBase<AutofillPaymentInstrument>
         // Allow saving new cards on disk.
         if (isNewCard) addSaveCardCheckbox(editor);
 
-        // If the user clicks [Cancel], send a null card back to the caller.
+        // If the user clicks [Cancel], send |toEdit| card back to the caller (will return original
+        // state, which could be null, a full card, or a partial card).
         editor.setCancelCallback(new Runnable() {
             @Override
             public void run() {
-                callback.onResult(null);
+                callback.onResult(toEdit);
             }
         });
 

chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java

@@ -96,7 +96,8 @@ public class ContactEditor extends EditorBase<AutofillContact> {
     }
 
     @Override
-    public void edit(@Nullable AutofillContact toEdit, final Callback<AutofillContact> callback) {
+    public void edit(
+            @Nullable final AutofillContact toEdit, final Callback<AutofillContact> callback) {
         super.edit(toEdit, callback);
 
         final AutofillContact contact = toEdit == null
@@ -135,10 +136,12 @@ public class ContactEditor extends EditorBase<AutofillContact> {
         if (phoneField != null) editor.addField(phoneField);
         if (emailField != null) editor.addField(emailField);
 
+        // If the user clicks [Cancel], send |toEdit| contact back to the caller, which was the
+        // original state (could be null, a complete contact, a partial contact).
         editor.setCancelCallback(new Runnable() {
             @Override
             public void run() {
-                callback.onResult(null);
+                callback.onResult(toEdit);
             }
         });
 

chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java

@@ -902,19 +902,22 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
         }
         mAddressEditor.edit(toEdit, new Callback<AutofillAddress>() {
             @Override
-            public void onResult(AutofillAddress completeAddress) {
+            public void onResult(AutofillAddress editedAddress) {
                 if (mUI == null) return;
 
-                if (completeAddress == null) {
-                    mShippingAddressesSection.setSelectedItemIndex(SectionInformation.NO_SELECTION);
+                // |editedAddress| could be null if something went wrong or user was adding a new
+                // address and cancelled out.
+                if (editedAddress == null) {
                     providePaymentInformation();
                 } else {
                     // Set the shipping address label.
-                    completeAddress.setShippingAddressLabelWithCountry();
+                    editedAddress.setShippingAddressLabelWithCountry();
 
-                    if (toEdit == null) mShippingAddressesSection.addAndSelectItem(completeAddress);
-                    mCardEditor.updateBillingAddressIfComplete(completeAddress);
-                    mClient.onShippingAddressChange(completeAddress.toPaymentAddress());
+                    // We add a new item if the user was in the "add" flow and the result in
+                    // |editedAddress| is non-null.
+                    if (toEdit == null) mShippingAddressesSection.addAndSelectItem(editedAddress);
+                    mCardEditor.updateBillingAddressIfComplete(editedAddress);
+                    mClient.onShippingAddressChange(editedAddress.toPaymentAddress());
                 }
             }
         });
@@ -928,13 +931,15 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
         }
         mContactEditor.edit(toEdit, new Callback<AutofillContact>() {
             @Override
-            public void onResult(AutofillContact completeContact) {
+            public void onResult(AutofillContact editedContact) {
                 if (mUI == null) return;
 
-                if (completeContact == null) {
-                    mContactSection.setSelectedItemIndex(SectionInformation.NO_SELECTION);
-                } else if (toEdit == null) {
-                    mContactSection.addAndSelectItem(completeContact);
+                // |editedContact| could be null if something went wrong or user was adding a
+                // contact and cancelled out of that flow. In the following block we add an item to
+                // the list if we were in the "add" flow, and the result in |editedContact| is
+                // non-null.
+                if (toEdit == null && editedContact != null) {
+                    mContactSection.addAndSelectItem(editedContact);
                 }
 
                 mUI.updateSection(PaymentRequestUI.TYPE_CONTACT_DETAILS, mContactSection);
@@ -950,13 +955,14 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
         }
         mCardEditor.edit(toEdit, new Callback<AutofillPaymentInstrument>() {
             @Override
-            public void onResult(AutofillPaymentInstrument completeCard) {
+            public void onResult(AutofillPaymentInstrument editedCard) {
                 if (mUI == null) return;
 
-                if (completeCard == null) {
-                    mPaymentMethodsSection.setSelectedItemIndex(SectionInformation.NO_SELECTION);
-                } else if (toEdit == null) {
-                    mPaymentMethodsSection.addAndSelectItem(completeCard);
+                // |editedCard| could be null if something went wrong or user was adding a card
+                // and cancelled out of that flow. In the following block we add an item to the list
+                // if we were in the "add" flow and the result in |editedCard| is non-null.
+                if (toEdit == null && editedCard != null) {
+                    mPaymentMethodsSection.addAndSelectItem(editedCard);
                 }
 
                 updateInstrumentModifiedTotals();

chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java

@@ -427,6 +427,9 @@ public class EditorView extends AlwaysDismissedDialog
                     if (mObserverForTest != null) mObserverForTest.onPaymentRequestReadyToEdit();
                 }
             });
+        } else {
+            // The first field will be focused, we are ready to edit.
+            if (mObserverForTest != null) mObserverForTest.onPaymentRequestReadyToEdit();
         }
     }
 

chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java

@@ -893,6 +893,11 @@ public abstract class PaymentRequestSection extends LinearLayout implements View
                 }
             }
 
+            /** Returns whether this OptionRow's RadioButton is checked. */
+            public boolean isChecked() {
+                return ((RadioButton) mButton).isChecked();
+            }
+
             /** Change the label for the row. */
             public void setLabel(int stringId) {
                 setLabel(getContext().getString(stringId));

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java

@@ -131,6 +131,40 @@ public class PaymentRequestContactDetailsTest extends PaymentRequestTestBase {
         expectResultContains(new String[] {"Request cancelled"});
     }
 
+    /** Test that going into the editor and cancelling will leave the row checked. */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testEditContactAndCancelEditorShouldKeepContactSelected()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        triggerUIAndWait(mReadyToPay);
+        clickInContactInfoAndWait(R.id.payments_section, mReadyForInput);
+        expectContactDetailsRowIsSelected(0);
+        clickInContactInfoAndWait(R.id.payments_open_editor_pencil_button, mReadyToEdit);
+
+        // Cancel the editor.
+        clickInEditorAndWait(R.id.payments_edit_cancel_button, mReadyForInput);
+
+        // Expect the row to still be selected in the Contact Details section.
+        expectContactDetailsRowIsSelected(0);
+    }
+
+    /** Test that going into the "add" flow and cancelling will leave existing row checked. */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testAddContactAndCancelEditorShouldKeepContactSelected()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        triggerUIAndWait(mReadyToPay);
+        clickInContactInfoAndWait(R.id.payments_section, mReadyForInput);
+        expectContactDetailsRowIsSelected(0);
+        clickInContactInfoAndWait(R.id.payments_add_option_button, mReadyToEdit);
+
+        // Cancel the editor.
+        clickInEditorAndWait(R.id.payments_edit_cancel_button, mReadyForInput);
+
+        // Expect the existing row to still be selected in the Contact Details section.
+        expectContactDetailsRowIsSelected(0);
+    }
+
     /** Quickly pressing on "add contact info" and then "cancel" should not crash. */
     @MediumTest
     @Feature({"Payments"})

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java

@@ -85,6 +85,40 @@ public class PaymentRequestPaymentAppAndCardsTest extends PaymentRequestTestBase
         runTest(HAVE_INSTRUMENTS, DELAYED_RESPONSE);
     }
 
+    /** Test that going into the editor and cancelling will leave the row checked. */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testEditPaymentMethodAndCancelEditorShouldKeepCardSelected()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        triggerUIAndWait(mReadyToPay);
+        clickInPaymentMethodAndWait(R.id.payments_section, mReadyForInput);
+        expectPaymentMethodRowIsSelected(0);
+        clickInPaymentMethodAndWait(R.id.payments_add_option_button, mReadyToEdit);
+
+        // Cancel the editor.
+        clickInCardEditorAndWait(R.id.payments_edit_cancel_button, mReadyForInput);
+
+        // Expect the existing row to still be selected in the Shipping Address section.
+        expectPaymentMethodRowIsSelected(0);
+    }
+
+    /** Test that going into "add" flow editor and cancelling will leave existing row checked. */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testAddPaymentMethodAndCancelEditorShouldKeepExistingCardSelected()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        triggerUIAndWait(mReadyToPay);
+        clickInPaymentMethodAndWait(R.id.payments_section, mReadyForInput);
+        expectPaymentMethodRowIsSelected(0);
+        clickInPaymentMethodAndWait(R.id.payments_open_editor_pencil_button, mReadyToEdit);
+
+        // Cancel the editor.
+        clickInCardEditorAndWait(R.id.payments_edit_cancel_button, mReadyForInput);
+
+        // Expect the row to still be selected in the Shipping Address section.
+        expectPaymentMethodRowIsSelected(0);
+    }
+
     private void runTest(int instrumentPresence, int responseSpeed) throws InterruptedException,
             ExecutionException, TimeoutException  {
         installPaymentApp(instrumentPresence, responseSpeed);

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressTest.java

@@ -113,4 +113,38 @@ public class PaymentRequestShippingAddressTest extends PaymentRequestTestBase {
         assertTrue(getShippingAddressOptionRowAtIndex(0).getLabelText().toString().equals(
                 "Seb Doe\nGoogle, 340 Main St, Los Angeles, CA 90291\n555-555-5555"));
     }
+
+    /** Test that going into the editor and cancelling will leave the row checked. */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testEditShippingAddressAndCancelEditorShouldKeepAddressSelected()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        triggerUIAndWait(mReadyToPay);
+        clickInShippingSummaryAndWait(R.id.payments_section, mReadyForInput);
+        expectShippingAddressRowIsSelected(0);
+        clickInShippingAddressAndWait(R.id.payments_open_editor_pencil_button, mReadyToEdit);
+
+        // Cancel the editor.
+        clickInEditorAndWait(R.id.payments_edit_cancel_button, mReadyForInput);
+
+        // Expect the row to still be selected in the Shipping Address section.
+        expectShippingAddressRowIsSelected(0);
+    }
+
+    /** Test that going into the "add" flow  and cancelling will leave the existing row checked. */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testAddShippingAddressAndCancelEditorShouldKeepAddressSelected()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        triggerUIAndWait(mReadyToPay);
+        clickInShippingSummaryAndWait(R.id.payments_section, mReadyForInput);
+        expectShippingAddressRowIsSelected(0);
+        clickInShippingAddressAndWait(R.id.payments_add_option_button, mReadyToEdit);
+
+        // Cancel the editor.
+        clickInEditorAndWait(R.id.payments_edit_cancel_button, mReadyForInput);
+
+        // Expect the existing row to still be selected in the Shipping Address section.
+        expectShippingAddressRowIsSelected(0);
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java

@@ -321,7 +321,7 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
         });
     }
 
-    /**  Returns the the number of payment instruments. */
+    /**  Returns the number of payment instruments. */
     protected int getNumberOfPaymentInstruments() throws ExecutionException {
         return ThreadUtils.runOnUiThreadBlocking(new Callable<Integer>() {
             @Override
@@ -332,7 +332,7 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
         });
     }
 
-    /**  Returns the the number of contact detail suggestions. */
+    /**  Returns the number of contact detail suggestions. */
     protected int getNumberOfContactDetailSuggestions() throws ExecutionException {
         return ThreadUtils.runOnUiThreadBlocking(new Callable<Integer>() {
             @Override
@@ -595,6 +595,57 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
         });
     }
 
+    /**  Will fail if the OptionRow at |index| is not selected in Contact Details.*/
+    protected void expectContactDetailsRowIsSelected(final int index)
+            throws ExecutionException, InterruptedException {
+        CriteriaHelper.pollInstrumentationThread(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                boolean isSelected = ((OptionSection) mUI.getContactDetailsSectionForTest())
+                                             .getOptionRowAtIndex(index)
+                                             .isChecked();
+                if (!isSelected) {
+                    updateFailureReason("Contact Details row at " + index + " was not selected.");
+                }
+                return isSelected;
+            }
+        });
+    }
+
+    /**  Will fail if the OptionRow at |index| is not selected in Shipping Address section.*/
+    protected void expectShippingAddressRowIsSelected(final int index)
+            throws ExecutionException, InterruptedException {
+        CriteriaHelper.pollInstrumentationThread(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                boolean isSelected = ((OptionSection) mUI.getShippingAddressSectionForTest())
+                                             .getOptionRowAtIndex(index)
+                                             .isChecked();
+                if (!isSelected) {
+                    updateFailureReason("Shipping Address row at " + index + " was not selected.");
+                }
+                return isSelected;
+            }
+        });
+    }
+
+    /**  Will fail if the OptionRow at |index| is not selected in PaymentMethod section.*/
+    protected void expectPaymentMethodRowIsSelected(final int index)
+            throws ExecutionException, InterruptedException {
+        CriteriaHelper.pollInstrumentationThread(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                boolean isSelected = ((OptionSection) mUI.getPaymentMethodSectionForTest())
+                                             .getOptionRowAtIndex(index)
+                                             .isChecked();
+                if (!isSelected) {
+                    updateFailureReason("Payment Method row at " + index + " was not selected.");
+                }
+                return isSelected;
+            }
+        });
+    }
+
     @Override
     public void onPaymentRequestReadyForInput(PaymentRequestUI ui) {
         ThreadUtils.assertOnUiThread();