Commit 15faefd2 authored by Clemens Arbesser's avatar Clemens Arbesser Committed by Commit Bot

Reland "[Autofill Assistant] Hotfix for radio buttons in FormAction."

This is a reland of 8698d875
The test was failing on smaller screens due to a missing scrollToView().

Original change's description:
> [Autofill Assistant] Hotfix for radio buttons in FormAction.
>
> Without this CL, UI and model can go out-of-sync when tapping the radio
> button directly (see screenshots in linked bug). This is a hotfix for
> this issue. A proper fix requires a refactoring of the FormAction to
> make the selection state part of its model (same as was done for the
> T&C selection state in the CollectUserDataModel).
>
> Bug: b/150201921
> Change-Id: Ifa0a2839c532a1862358bc68e6d0537dee93b49c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2083489
> Commit-Queue: Clemens Arbesser <arbesser@google.com>
> Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#746539}

Bug: b/150201921
Change-Id: I1a7d9008eb625be46dac9d9bc5f26728c9fcee51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087612Reviewed-by: default avatarSandro Maggi <sandromaggi@google.com>
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/master@{#746750}
parent 240ee8df
...@@ -15,6 +15,7 @@ import org.chromium.chrome.autofill_assistant.R; ...@@ -15,6 +15,7 @@ import org.chromium.chrome.autofill_assistant.R;
import org.chromium.chrome.browser.autofill_assistant.AssistantTextUtils; import org.chromium.chrome.browser.autofill_assistant.AssistantTextUtils;
import org.chromium.chrome.browser.autofill_assistant.user_data.AssistantChoiceList; import org.chromium.chrome.browser.autofill_assistant.user_data.AssistantChoiceList;
import java.util.ArrayList;
import java.util.List; import java.util.List;
/** A form input that allows to choose between multiple choices. */ /** A form input that allows to choose between multiple choices. */
...@@ -28,6 +29,7 @@ class AssistantFormSelectionInput extends AssistantFormInput { ...@@ -28,6 +29,7 @@ class AssistantFormSelectionInput extends AssistantFormInput {
private final List<AssistantFormSelectionChoice> mChoices; private final List<AssistantFormSelectionChoice> mChoices;
private final boolean mAllowMultipleChoices; private final boolean mAllowMultipleChoices;
private final Delegate mDelegate; private final Delegate mDelegate;
private final List<View> mChoiceViews = new ArrayList<>();
public AssistantFormSelectionInput(String label, List<AssistantFormSelectionChoice> choices, public AssistantFormSelectionInput(String label, List<AssistantFormSelectionChoice> choices,
boolean allowMultipleChoices, Delegate delegate) { boolean allowMultipleChoices, Delegate delegate) {
...@@ -56,6 +58,7 @@ class AssistantFormSelectionInput extends AssistantFormInput { ...@@ -56,6 +58,7 @@ class AssistantFormSelectionInput extends AssistantFormInput {
ViewGroup checkboxList = root.findViewById(R.id.checkbox_list); ViewGroup checkboxList = root.findViewById(R.id.checkbox_list);
AssistantChoiceList radiobuttonList = root.findViewById(R.id.radiobutton_list); AssistantChoiceList radiobuttonList = root.findViewById(R.id.radiobutton_list);
radiobuttonList.setAllowMultipleChoices(false);
for (int i = 0; i < mChoices.size(); i++) { for (int i = 0; i < mChoices.size(); i++) {
AssistantFormSelectionChoice choice = mChoices.get(i); AssistantFormSelectionChoice choice = mChoices.get(i);
...@@ -79,10 +82,24 @@ class AssistantFormSelectionInput extends AssistantFormInput { ...@@ -79,10 +82,24 @@ class AssistantFormSelectionInput extends AssistantFormInput {
radiobuttonList.addItem(choiceView, /* hasEditButton= */ false, radiobuttonList.addItem(choiceView, /* hasEditButton= */ false,
(isChecked) (isChecked)
-> mDelegate.onChoiceSelectionChanged(index, isChecked), -> {
mDelegate.onChoiceSelectionChanged(index, isChecked);
// Workaround for radio buttons in FormAction: de-select all other
// items. This is needed because the current selection state is not part
// of AssistantFormModel (but it should be). TODO(b/150201921).
if (isChecked) {
for (View view : mChoiceViews) {
if (view == choiceView) {
continue;
}
radiobuttonList.setChecked(view, false);
}
}
},
/* itemEditedListener= */ null); /* itemEditedListener= */ null);
radiobuttonList.setChecked(choiceView, choice.isInitiallySelected()); radiobuttonList.setChecked(choiceView, choice.isInitiallySelected());
} }
mChoiceViews.add(choiceView);
TextView choiceLabel = choiceView.findViewById(R.id.label); TextView choiceLabel = choiceView.findViewById(R.id.label);
TextView descriptionLine1 = choiceView.findViewById(R.id.description_line_1); TextView descriptionLine1 = choiceView.findViewById(R.id.description_line_1);
......
...@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.autofill_assistant; ...@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.autofill_assistant;
import static android.support.test.espresso.Espresso.onView; import static android.support.test.espresso.Espresso.onView;
import static android.support.test.espresso.action.ViewActions.click; import static android.support.test.espresso.action.ViewActions.click;
import static android.support.test.espresso.action.ViewActions.scrollTo;
import static android.support.test.espresso.assertion.ViewAssertions.doesNotExist; import static android.support.test.espresso.assertion.ViewAssertions.doesNotExist;
import static android.support.test.espresso.assertion.ViewAssertions.matches; import static android.support.test.espresso.assertion.ViewAssertions.matches;
import static android.support.test.espresso.intent.Intents.intended; import static android.support.test.espresso.intent.Intents.intended;
...@@ -13,13 +14,17 @@ import static android.support.test.espresso.intent.Intents.intending; ...@@ -13,13 +14,17 @@ import static android.support.test.espresso.intent.Intents.intending;
import static android.support.test.espresso.intent.matcher.IntentMatchers.anyIntent; import static android.support.test.espresso.intent.matcher.IntentMatchers.anyIntent;
import static android.support.test.espresso.intent.matcher.IntentMatchers.hasAction; import static android.support.test.espresso.intent.matcher.IntentMatchers.hasAction;
import static android.support.test.espresso.intent.matcher.IntentMatchers.hasData; import static android.support.test.espresso.intent.matcher.IntentMatchers.hasData;
import static android.support.test.espresso.matcher.ViewMatchers.Visibility.VISIBLE;
import static android.support.test.espresso.matcher.ViewMatchers.assertThat; import static android.support.test.espresso.matcher.ViewMatchers.assertThat;
import static android.support.test.espresso.matcher.ViewMatchers.hasDescendant; import static android.support.test.espresso.matcher.ViewMatchers.hasDescendant;
import static android.support.test.espresso.matcher.ViewMatchers.hasSibling; import static android.support.test.espresso.matcher.ViewMatchers.hasSibling;
import static android.support.test.espresso.matcher.ViewMatchers.hasTextColor; import static android.support.test.espresso.matcher.ViewMatchers.hasTextColor;
import static android.support.test.espresso.matcher.ViewMatchers.isChecked;
import static android.support.test.espresso.matcher.ViewMatchers.isCompletelyDisplayed; import static android.support.test.espresso.matcher.ViewMatchers.isCompletelyDisplayed;
import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed; import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed;
import static android.support.test.espresso.matcher.ViewMatchers.isEnabled; import static android.support.test.espresso.matcher.ViewMatchers.isEnabled;
import static android.support.test.espresso.matcher.ViewMatchers.withClassName;
import static android.support.test.espresso.matcher.ViewMatchers.withEffectiveVisibility;
import static android.support.test.espresso.matcher.ViewMatchers.withId; import static android.support.test.espresso.matcher.ViewMatchers.withId;
import static android.support.test.espresso.matcher.ViewMatchers.withText; import static android.support.test.espresso.matcher.ViewMatchers.withText;
...@@ -30,6 +35,7 @@ import static org.hamcrest.Matchers.not; ...@@ -30,6 +35,7 @@ import static org.hamcrest.Matchers.not;
import static org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiTestUtil.hasTintColor; import static org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiTestUtil.hasTintColor;
import static org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiTestUtil.startAutofillAssistant; import static org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiTestUtil.startAutofillAssistant;
import static org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiTestUtil.waitUntilViewMatchesCondition; import static org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiTestUtil.waitUntilViewMatchesCondition;
import static org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiTestUtil.withParentIndex;
import android.app.Activity; import android.app.Activity;
import android.app.Instrumentation.ActivityResult; import android.app.Instrumentation.ActivityResult;
...@@ -38,6 +44,7 @@ import android.net.Uri; ...@@ -38,6 +44,7 @@ import android.net.Uri;
import android.support.test.InstrumentationRegistry; import android.support.test.InstrumentationRegistry;
import android.support.test.espresso.intent.Intents; import android.support.test.espresso.intent.Intents;
import android.support.test.filters.MediumTest; import android.support.test.filters.MediumTest;
import android.widget.RadioButton;
import org.junit.Before; import org.junit.Before;
import org.junit.Rule; import org.junit.Rule;
...@@ -128,13 +135,17 @@ public class AutofillAssistantFormActionTest { ...@@ -128,13 +135,17 @@ public class AutofillAssistantFormActionTest {
.setExpandText("Expand"))) .setExpandText("Expand")))
.addInputs(FormInputProto.newBuilder().setSelection( .addInputs(FormInputProto.newBuilder().setSelection(
SelectionInputProto.newBuilder() SelectionInputProto.newBuilder()
.addChoices( .addChoices(SelectionInputProto.Choice.newBuilder()
SelectionInputProto.Choice.newBuilder() .setLabel("Choice 1")
.setLabel("Choice 1") .setDescriptionLine1("$10.00 option")
.setDescriptionLine1("$10.00 per choice") .setDescriptionLine2(
.setDescriptionLine2( "<link1>Details</link1>"))
"<link1>Details</link1>")) .addChoices(SelectionInputProto.Choice.newBuilder()
.setAllowMultiple(true))) .setLabel("Choice 2")
.setDescriptionLine1("$20.00 option")
.setDescriptionLine2(
"<link1>Details</link1>"))
.setAllowMultiple(false)))
.addInputs(FormInputProto.newBuilder().setCounter( .addInputs(FormInputProto.newBuilder().setCounter(
CounterInputProto.newBuilder().addCounters( CounterInputProto.newBuilder().addCounters(
CounterInputProto.Counter.newBuilder() CounterInputProto.Counter.newBuilder()
...@@ -166,42 +177,58 @@ public class AutofillAssistantFormActionTest { ...@@ -166,42 +177,58 @@ public class AutofillAssistantFormActionTest {
waitUntilViewMatchesCondition(withText("Continue"), isCompletelyDisplayed()); waitUntilViewMatchesCondition(withText("Continue"), isCompletelyDisplayed());
// TODO(b/144690738) Remove the isDisplayed() condition. // TODO(b/144690738) Remove the isDisplayed() condition.
onView(allOf(isDisplayed(), withId(R.id.value), onView(allOf(withId(R.id.value), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 1"))))) hasSibling(hasDescendant(withText("Counter 1")))))
.check(matches(hasTextColor(R.color.modern_grey_800_alpha_38))); .check(matches(hasTextColor(R.color.modern_grey_800_alpha_38)));
onView(allOf(isDisplayed(), withId(R.id.increase_button), onView(allOf(withId(R.id.increase_button), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 1"))))) hasSibling(hasDescendant(withText("Counter 1")))))
.check(matches(hasTintColor(R.color.modern_blue_600))); .check(matches(hasTintColor(R.color.modern_blue_600)));
onView(allOf(isDisplayed(), withId(R.id.decrease_button), onView(allOf(withId(R.id.decrease_button), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 1"))))) hasSibling(hasDescendant(withText("Counter 1")))))
.check(matches(hasTintColor(R.color.modern_grey_800_alpha_38))); .check(matches(hasTintColor(R.color.modern_grey_800_alpha_38)));
// Click on Counter 1 +, increase from 0 to 1. // Click on Counter 1 +, increase from 0 to 1.
onView(allOf(isDisplayed(), withId(R.id.increase_button), onView(allOf(withId(R.id.increase_button), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 1"))))) hasSibling(hasDescendant(withText("Counter 1")))))
.perform(click()); .perform(scrollTo(), click());
onView(allOf(isDisplayed(), withId(R.id.value), onView(allOf(withId(R.id.value), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 1"))))) hasSibling(hasDescendant(withText("Counter 1")))))
.check(matches(hasTextColor(R.color.modern_blue_600))); .check(matches(hasTextColor(R.color.modern_blue_600)));
onView(allOf(isDisplayed(), withId(R.id.increase_button), onView(allOf(withId(R.id.increase_button), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 1"))))) hasSibling(hasDescendant(withText("Counter 1")))))
.check(matches(hasTintColor(R.color.modern_grey_800_alpha_38))); .check(matches(hasTintColor(R.color.modern_grey_800_alpha_38)));
// Decrease button is still disabled due to the minCountersSum requirement. // Decrease button is still disabled due to the minCountersSum requirement.
// Click expand label to make Counter 2 visible. // Click expand label to make Counter 2 visible.
onView(allOf(isDisplayed(), withId(R.id.expand_label))).perform(click()); onView(allOf(withId(R.id.expand_label), withEffectiveVisibility(VISIBLE)))
.perform(scrollTo(), click());
// Click on Counter 3 +, increase from 0 to 1. // Click on Counter 3 +, increase from 0 to 1.
onView(allOf(isDisplayed(), withId(R.id.increase_button), onView(allOf(withId(R.id.increase_button), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 3"))))) hasSibling(hasDescendant(withText("Counter 3")))))
.perform(click()); .perform(scrollTo(), click());
// Click on Choice 1, then Choice 2, then back to Choice 1.
onView(allOf(withClassName(is(RadioButton.class.getName())), withParentIndex(0),
withEffectiveVisibility(VISIBLE)))
.perform(scrollTo(), click());
onView(allOf(withClassName(is(RadioButton.class.getName())), withParentIndex(3),
withEffectiveVisibility(VISIBLE)))
.perform(scrollTo(), click());
onView(allOf(withClassName(is(RadioButton.class.getName())), withParentIndex(0),
withEffectiveVisibility(VISIBLE)))
.perform(scrollTo(), click());
// Check that choice 1 is visually selected and choice 2 is de-selected.
onView(allOf(withClassName(is(RadioButton.class.getName())), withParentIndex(0),
withEffectiveVisibility(VISIBLE)))
.check(matches(isChecked()));
onView(allOf(withClassName(is(RadioButton.class.getName())), withParentIndex(3),
withEffectiveVisibility(VISIBLE)))
.check(matches(not(isChecked())));
// Click on Choice 1, toggle to 'checked'.
onView(allOf(isDisplayed(), withId(R.id.checkbox),
hasSibling(hasDescendant(withText("Choice 1")))))
.perform(click());
// Click on Counter 2 +, increase from 0 to 1. // Click on Counter 2 +, increase from 0 to 1.
onView(allOf(isDisplayed(), withId(R.id.increase_button), onView(allOf(withId(R.id.increase_button), withEffectiveVisibility(VISIBLE),
hasSibling(hasDescendant(withText("Counter 2"))))) hasSibling(hasDescendant(withText("Counter 2")))))
.perform(click()); .perform(scrollTo(), click());
// Finish form action, wait for response and prepare next set of actions. // Finish form action, wait for response and prepare next set of actions.
List<ActionProto> nextActions = new ArrayList<>(); List<ActionProto> nextActions = new ArrayList<>();
...@@ -240,8 +267,9 @@ public class AutofillAssistantFormActionTest { ...@@ -240,8 +267,9 @@ public class AutofillAssistantFormActionTest {
// Choice 1 // Choice 1
assertThat(formResult.get(1).getInputTypeCase(), assertThat(formResult.get(1).getInputTypeCase(),
is(FormInputProto.Result.InputTypeCase.SELECTION)); is(FormInputProto.Result.InputTypeCase.SELECTION));
assertThat(formResult.get(1).getSelection().getSelectedCount(), is(1)); assertThat(formResult.get(1).getSelection().getSelectedCount(), is(2));
assertThat(formResult.get(1).getSelection().getSelected(0), is(true)); assertThat(formResult.get(1).getSelection().getSelected(0), is(true));
assertThat(formResult.get(1).getSelection().getSelected(1), is(false));
// Counter 3 // Counter 3
assertThat(formResult.get(2).getInputTypeCase(), assertThat(formResult.get(2).getInputTypeCase(),
......
...@@ -222,6 +222,26 @@ class AutofillAssistantUiTestUtil { ...@@ -222,6 +222,26 @@ class AutofillAssistantUiTestUtil {
}; };
} }
public static Matcher<View> withParentIndex(int parentIndex) {
return new TypeSafeMatcher<View>() {
@Override
public void describeTo(Description description) {
description.appendText("withParentIndex: " + parentIndex);
}
@Override
public boolean matchesSafely(View view) {
ViewParent parent = view.getParent();
if (!(parent instanceof ViewGroup)) {
return false;
} else {
ViewGroup parentGroup = (ViewGroup) parent;
return parentGroup.getChildAt(parentIndex) == view;
}
}
};
}
public static ViewAction openTextLink(String textLink) { public static ViewAction openTextLink(String textLink) {
return new ViewAction() { return new ViewAction() {
@Override @Override
......
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