[Autofill Assistant] Changing progress bar icons must keep state

Before this change the state of the progress bar got lost when changing
icons. This means that icons and lines marked as "done" would become
unmarked again and the currently active (i.e. animated) icon would stop
being active.

This generally is only a problem for the first icon when starting with
the proper experiment. The flow would set the active step to 0,
then change the icons and thus lose the state.

Additionally, it was not possible to stop using the new progress
bar without specifying icons, as the action would fail.

Bug: b/161962005
Change-Id: I80592cb6215a94a95dd67fe66a7d5d494e0f3c28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2315883
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Reviewed-by: Luca Hunkeler <hluca@google.com>
Cr-Commit-Position: refs/heads/master@{#791270}

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/header/AssistantHeaderModel.java

@@ -108,6 +108,9 @@ public class AssistantHeaderModel extends PropertyModel {
     @CalledByNative
     private void setStepProgressBarIcons(List<AssistantDrawable> icons) {
         set(STEP_PROGRESS_BAR_ICONS, icons);
+        // Reset progress bar entries.
+        set(PROGRESS_ACTIVE_STEP, -1);
+        set(PROGRESS_BAR_ERROR, false);
     }
 
     @CalledByNative

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/header/AssistantHeaderViewBinder.java

@@ -92,8 +92,10 @@ class AssistantHeaderViewBinder
         } else if (AssistantHeaderModel.PROGRESS == propertyKey) {
             view.mProgressBar.setProgress(model.get(AssistantHeaderModel.PROGRESS));
         } else if (AssistantHeaderModel.PROGRESS_ACTIVE_STEP == propertyKey) {
-            view.mStepProgressBar.setActiveStep(
-                    model.get(AssistantHeaderModel.PROGRESS_ACTIVE_STEP));
+            int activeStep = model.get(AssistantHeaderModel.PROGRESS_ACTIVE_STEP);
+            if (activeStep >= 0) {
+                view.mStepProgressBar.setActiveStep(activeStep);
+            }
         } else if (AssistantHeaderModel.PROGRESS_BAR_ERROR == propertyKey) {
             view.mStepProgressBar.setError(model.get(AssistantHeaderModel.PROGRESS_BAR_ERROR));
         } else if (AssistantHeaderModel.PROGRESS_VISIBLE == propertyKey
@@ -102,7 +104,6 @@ class AssistantHeaderViewBinder
                     model.get(AssistantHeaderModel.USE_STEP_PROGRESS_BAR));
         } else if (AssistantHeaderModel.STEP_PROGRESS_BAR_ICONS == propertyKey) {
             view.mStepProgressBar.setSteps(model.get(AssistantHeaderModel.STEP_PROGRESS_BAR_ICONS));
-            view.mStepProgressBar.setError(model.get(AssistantHeaderModel.PROGRESS_BAR_ERROR));
             view.mStepProgressBar.disableAnimations(
                     model.get(AssistantHeaderModel.DISABLE_ANIMATIONS_FOR_TESTING));
         } else if (AssistantHeaderModel.SPIN_POODLE == propertyKey) {

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/header/AssistantStepProgressBar.java

@@ -352,6 +352,7 @@ public class AssistantStepProgressBar {
 
         assert icons.size() >= 2;
         mNumberOfSteps = icons.size();
+        mCurrentStep = -1;
 
         mIcons = new IconViewHolder[mNumberOfSteps];
         mLines = new LineViewHolder[mNumberOfSteps - 1];
@@ -395,11 +396,11 @@ public class AssistantStepProgressBar {
                 mIcons[i].setEnabled(i < step);
             }
 
-            if (i == step && step == mCurrentStep + 1) {
+            if (i == step && step == mCurrentStep + 1 && mCurrentStep != -1) {
                 // In case we advance to a new step, start the enable animation on the current
                 // icon. If not for the first step, start the pulsating animation with a delay such
                 // that it only starts after the other animations have run.
-                mIcons[i].startPulsingAnimation(/* delayed= */ mCurrentStep != -1);
+                mIcons[i].startPulsingAnimation(/* delayed= */ true);
             } else {
                 mIcons[i].setPulsingAnimationEnabled(i == step);
             }

chrome/android/features/autofill_assistant/javatests/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantProgressBarIntegrationTest.java

@@ -249,4 +249,74 @@ public class AutofillAssistantProgressBarIntegrationTest {
                     .check(matches(allOf(isEnabled(), hasTintColor(R.color.modern_blue_600))));
         }
     }
+
+    @Test
+    @MediumTest
+    public void updatingIconsRestoresActiveState() {
+        ArrayList<ActionProto> list = new ArrayList<>();
+        list.add((ActionProto) ActionProto.newBuilder()
+                         .setShowProgressBar(
+                                 ShowProgressBarProto.newBuilder().setStepProgressBarConfiguration(
+                                         StepProgressBarConfiguration.newBuilder()
+                                                 .setUseStepProgressBar(true)))
+                         .build());
+        list.add((ActionProto) ActionProto.newBuilder()
+                         .setShowProgressBar(ShowProgressBarProto.newBuilder().setActiveStep(1))
+                         .build());
+        list.add((ActionProto) ActionProto.newBuilder()
+                         .setPrompt(PromptProto.newBuilder().setMessage("Prompt").addChoices(
+                                 Choice.newBuilder().setChip(
+                                         ChipProto.newBuilder().setText("Update"))))
+                         .build());
+        list.add((ActionProto) ActionProto.newBuilder()
+                         .setShowProgressBar(
+                                 ShowProgressBarProto.newBuilder().setStepProgressBarConfiguration(
+                                         StepProgressBarConfiguration.newBuilder()
+                                                 .setUseStepProgressBar(true)
+                                                 .addStepIcons(DrawableProto.newBuilder().setIcon(
+                                                         Icon.PROGRESSBAR_DEFAULT_INITIAL_STEP))
+                                                 .addStepIcons(DrawableProto.newBuilder().setIcon(
+                                                         Icon.PROGRESSBAR_DEFAULT_DATA_COLLECTION))
+                                                 .addStepIcons(DrawableProto.newBuilder().setIcon(
+                                                         Icon.PROGRESSBAR_DEFAULT_PAYMENT))
+                                                 .addStepIcons(DrawableProto.newBuilder().setIcon(
+                                                         Icon.PROGRESSBAR_DEFAULT_FINAL_STEP))))
+                         .build());
+        list.add((ActionProto) ActionProto.newBuilder()
+                         .setPrompt(PromptProto.newBuilder().setMessage("Updated").addChoices(
+                                 Choice.newBuilder().setChip(ChipProto.newBuilder())))
+                         .build());
+
+        AutofillAssistantTestScript script = new AutofillAssistantTestScript(
+                (SupportedScriptProto) SupportedScriptProto.newBuilder()
+                        .setPath("form_target_website.html")
+                        .setPresentation(PresentationProto.newBuilder().setAutostart(true).setChip(
+                                ChipProto.newBuilder().setText("Autostart")))
+                        .build(),
+                list);
+        runScript(script);
+
+        waitUntilViewMatchesCondition(withText("Prompt"), isCompletelyDisplayed());
+        onView(allOf(isDescendantOfA(withTagValue(is(String.format(Locale.getDefault(),
+                             AssistantTagsForTesting.PROGRESSBAR_ICON_TAG, 0)))),
+                       withClassName(is(ChromeImageView.class.getName()))))
+                .check(matches(allOf(isEnabled(), hasTintColor(R.color.modern_blue_600))));
+        onView(allOf(isDescendantOfA(withTagValue(is(String.format(Locale.getDefault(),
+                             AssistantTagsForTesting.PROGRESSBAR_ICON_TAG, 1)))),
+                       withClassName(is(ChromeImageView.class.getName()))))
+                .check(matches(
+                        allOf(not(isEnabled()), hasTintColor(R.color.modern_grey_800_alpha_38))));
+
+        onView(withText("Update")).perform(click());
+        waitUntilViewMatchesCondition(withText("Updated"), isCompletelyDisplayed());
+        onView(allOf(isDescendantOfA(withTagValue(is(String.format(Locale.getDefault(),
+                             AssistantTagsForTesting.PROGRESSBAR_ICON_TAG, 0)))),
+                       withClassName(is(ChromeImageView.class.getName()))))
+                .check(matches(allOf(isEnabled(), hasTintColor(R.color.modern_blue_600))));
+        onView(allOf(isDescendantOfA(withTagValue(is(String.format(Locale.getDefault(),
+                             AssistantTagsForTesting.PROGRESSBAR_ICON_TAG, 1)))),
+                       withClassName(is(ChromeImageView.class.getName()))))
+                .check(matches(
+                        allOf(not(isEnabled()), hasTintColor(R.color.modern_grey_800_alpha_38))));
+    }
 }

components/autofill_assistant/browser/actions/show_progress_bar_action.cc

@@ -34,15 +34,14 @@ void ShowProgressBarAction::InternalProcessAction(
     delegate_->SetProgressVisible(!proto_.show_progress_bar().hide());
   }
   if (proto_.show_progress_bar().has_step_progress_bar_configuration()) {
-    if (proto_.show_progress_bar()
-            .step_progress_bar_configuration()
-            .step_icons()
-            .size() < 2) {
+    const auto& configuration =
+        proto_.show_progress_bar().step_progress_bar_configuration();
+    if (!configuration.step_icons().empty() &&
+        configuration.step_icons().size() < 2) {
       EndAction(std::move(callback), INVALID_ACTION);
       return;
     }
-    delegate_->SetStepProgressBarConfiguration(
-        proto_.show_progress_bar().step_progress_bar_configuration());
+    delegate_->SetStepProgressBarConfiguration(configuration);
   }
 
   switch (proto_.show_progress_bar().progress_indicator_case()) {

components/autofill_assistant/browser/actions/show_progress_bar_action_unittest.cc

@@ -83,7 +83,9 @@ TEST_F(ShowProgressBarActionTest, ShowsProgressBar) {
 }
 
 TEST_F(ShowProgressBarActionTest, FewerThanTwoStepsProgressBarFailsAction) {
-  proto_.mutable_step_progress_bar_configuration()->add_step_icons()->set_icon(
+  auto* config = proto_.mutable_step_progress_bar_configuration();
+  config->set_use_step_progress_bar(true);
+  config->add_step_icons()->set_icon(
       DrawableProto::PROGRESSBAR_DEFAULT_INITIAL_STEP);
 
   EXPECT_CALL(mock_action_delegate_, SetStepProgressBarConfiguration(_))
@@ -96,6 +98,7 @@ TEST_F(ShowProgressBarActionTest, FewerThanTwoStepsProgressBarFailsAction) {
 
 TEST_F(ShowProgressBarActionTest, UpdateStepProgressBarConfiguration) {
   auto* config = proto_.mutable_step_progress_bar_configuration();
+  config->set_use_step_progress_bar(true);
   config->add_step_icons()->set_icon(
       DrawableProto::PROGRESSBAR_DEFAULT_INITIAL_STEP);
   config->add_step_icons()->set_icon(
@@ -108,6 +111,17 @@ TEST_F(ShowProgressBarActionTest, UpdateStepProgressBarConfiguration) {
   Run();
 }
 
+TEST_F(ShowProgressBarActionTest, DeactivateStepProgressBar) {
+  auto* config = proto_.mutable_step_progress_bar_configuration();
+  config->set_use_step_progress_bar(false);
+
+  EXPECT_CALL(mock_action_delegate_, SetStepProgressBarConfiguration(_));
+  EXPECT_CALL(
+      callback_,
+      Run(Pointee(Property(&ProcessedActionProto::status, ACTION_APPLIED))));
+  Run();
+}
+
 TEST_F(ShowProgressBarActionTest, SetProgress) {
   proto_.set_progress(50);
 

components/autofill_assistant/browser/controller.cc

@@ -292,8 +292,17 @@ bool Controller::GetProgressVisible() const {
 void Controller::SetStepProgressBarConfiguration(
     const ShowProgressBarProto::StepProgressBarConfiguration& configuration) {
   step_progress_bar_configuration_ = configuration;
+  if (!configuration.step_icons().empty() &&
+      progress_active_step_.has_value() &&
+      configuration.step_icons().size() < *progress_active_step_) {
+    progress_active_step_ = configuration.step_icons().size();
+  }
   for (ControllerObserver& observer : observers_) {
     observer.OnStepProgressBarConfigurationChanged(configuration);
+    if (progress_active_step_.has_value()) {
+      observer.OnProgressActiveStepChanged(*progress_active_step_);
+    }
+    observer.OnProgressBarErrorStateChanged(progress_bar_error_state_);
   }
 }