Fix DCHECK when closing Embedded Assistant UI

This DCHECK was caused because the Embedded Assistant UI tried to grab
the focus while it was being closed.

This |RequestFocus| call was added explicitly to fix a bug where the
input text field in the Assistant UI would lose focus when submitting
a query while in tablet mode.

And this was happening because we tried to dismiss the virtual on-screen
keyboard.

I added unittests to test for these 3 scenarios, and I fixed it by dismissing
the virtual keyboard using a |HideKeyboard| method instead of dismissing it
by dropping the focus of the text field.

Bug: b/141945964
Change-Id: I3aca9e08ab0e2b7419cfe53c1f4dc831130cd0df
Tests: Added unittests and verified manually
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1848796
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707983}

ash/app_list/views/assistant/assistant_dialog_plate.cc

@@ -12,6 +12,7 @@
 #include "ash/assistant/ui/dialog_plate/mic_view.h"
 #include "ash/assistant/ui/logo_view/logo_view.h"
 #include "ash/assistant/util/animation_util.h"
+#include "ash/keyboard/ui/keyboard_ui_controller.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/bind.h"
@@ -25,7 +26,6 @@
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/textfield/textfield.h"
-#include "ui/views/focus/focus_manager.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 
@@ -101,7 +101,7 @@ bool AssistantDialogPlate::HandleKeyEvent(views::Textfield* textfield,
       // In tablet mode the virtual keyboard should not be sticky, so we hide it
       // when committing a query.
       if (delegate_->IsTabletMode())
-        textfield_->GetFocusManager()->ClearFocus();
+        keyboard::KeyboardUIController::Get()->HideKeyboardImplicitlyBySystem();
 
       const base::StringPiece16& trimmed_text = base::TrimWhitespace(
           textfield_->GetText(), base::TrimPositions::TRIM_ALL);

ash/app_list/views/assistant/assistant_page_view.cc

@@ -154,10 +154,6 @@ void AssistantPageView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
 void AssistantPageView::ChildPreferredSizeChanged(views::View* child) {
   MaybeUpdateAppListState(child->GetHeightForWidth(width()));
   PreferredSizeChanged();
-
-  // After layout events, focus can be lost so we need to explicitly request
-  // on behalf of the child views.
-  RequestFocus();
 }
 
 void AssistantPageView::ChildVisibilityChanged(views::View* child) {
@@ -294,7 +290,7 @@ void AssistantPageView::OnUiVisibilityChanged(
   }
 }
 
-const AssistantMainView* AssistantPageView::GetMainViewForTest() const {
+AssistantMainView* AssistantPageView::GetMainViewForTest() {
   return assistant_main_view_;
 }
 

ash/app_list/views/assistant/assistant_page_view.h

@@ -67,7 +67,7 @@ class APP_LIST_EXPORT AssistantPageView : public AppListPage,
       base::Optional<AssistantEntryPoint> entry_point,
       base::Optional<AssistantExitPoint> exit_point) override;
 
-  const AssistantMainView* GetMainViewForTest() const;
+  AssistantMainView* GetMainViewForTest();
 
  private:
   int GetChildViewHeightForWidth(int width) const;

ash/app_list/views/assistant/assistant_page_view_unittest.cc

@@ -2,13 +2,62 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "ash/app_list/views/assistant/assistant_page_view.h"
 #include "ash/app_list/views/assistant/assistant_main_view.h"
 #include "ash/assistant/test/assistant_ash_test_base.h"
 #include "ash/assistant/ui/assistant_ui_constants.h"
 #include "base/run_loop.h"
+#include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/focus/focus_manager.h"
 
 namespace ash {
 
+namespace {
+
+// Stubbed |FocusChangeListener| that simply remembers all the views that
+// received focus.
+class FocusChangeListenerStub : public views::FocusChangeListener {
+ public:
+  explicit FocusChangeListenerStub(views::View* view)
+      : focus_manager_(view->GetFocusManager()) {
+    focus_manager_->AddFocusChangeListener(this);
+  }
+  ~FocusChangeListenerStub() override {
+    focus_manager_->RemoveFocusChangeListener(this);
+  }
+
+  void OnWillChangeFocus(views::View* focused_before,
+                         views::View* focused_now) override {}
+
+  void OnDidChangeFocus(views::View* focused_before,
+                        views::View* focused_now) override {
+    focused_views_.push_back(focused_now);
+  }
+
+  // Returns all views that received the focus at some point.
+  const std::vector<views::View*>& focused_views() const {
+    return focused_views_;
+  }
+
+ private:
+  std::vector<views::View*> focused_views_;
+  views::FocusManager* focus_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(FocusChangeListenerStub);
+};
+
+// Ensures that the given view has the focus. If it doesn't, this will print a
+// nice error message indicating which view has the focus instead.
+#define EXPECT_HAS_FOCUS(expected_)                                           \
+  ({                                                                          \
+    const views::View* actual =                                               \
+        main_view()->GetFocusManager()->GetFocusedView();                     \
+    EXPECT_TRUE(expected_->HasFocus())                                        \
+        << "Expected focus on '" << expected_->GetClassName()                 \
+        << "' but it is on '" << (actual ? actual->GetClassName() : "<null>") \
+        << "'.";                                                              \
+  })
+
 class AssistantPageViewTest : public AssistantAshTestBase {
  public:
   AssistantPageViewTest() = default;
@@ -17,8 +66,10 @@ class AssistantPageViewTest : public AssistantAshTestBase {
   DISALLOW_COPY_AND_ASSIGN(AssistantPageViewTest);
 };
 
+}  // namespace
+
 TEST_F(AssistantPageViewTest, ShouldStartAtMinimumHeight) {
-  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchBox);
+  ShowAssistantUi();
 
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(ash::kMinHeightEmbeddedDip, main_view()->size().height());
@@ -26,7 +77,7 @@ TEST_F(AssistantPageViewTest, ShouldStartAtMinimumHeight) {
 
 TEST_F(AssistantPageViewTest,
        ShouldRemainAtMinimumHeightWhenDisplayingOneLiner) {
-  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchBox);
+  ShowAssistantUi();
 
   MockAssistantInteractionWithResponse("Short one-liner");
 
@@ -35,7 +86,7 @@ TEST_F(AssistantPageViewTest,
 }
 
 TEST_F(AssistantPageViewTest, ShouldGetBiggerWithMultilineText) {
-  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchBox);
+  ShowAssistantUi();
 
   MockAssistantInteractionWithResponse(
       "This\ntext\nhas\na\nlot\nof\nlinebreaks.");
@@ -45,7 +96,7 @@ TEST_F(AssistantPageViewTest, ShouldGetBiggerWithMultilineText) {
 }
 
 TEST_F(AssistantPageViewTest, ShouldGetBiggerWhenWrappingTextLine) {
-  ShowAssistantUi(AssistantEntryPoint::kLauncherSearchBox);
+  ShowAssistantUi();
 
   MockAssistantInteractionWithResponse(
       "This is a very long text without any linebreaks. "
@@ -56,4 +107,101 @@ TEST_F(AssistantPageViewTest, ShouldGetBiggerWhenWrappingTextLine) {
   EXPECT_EQ(ash::kMaxHeightEmbeddedDip, main_view()->size().height());
 }
 
+TEST_F(AssistantPageViewTest, ShouldNotRequestFocusWhenOtherAppWindowOpens) {
+  // This tests the root cause of b/141945964.
+  // Namely, the Assistant code should not request the focus while being closed.
+  ShowAssistantUi();
+
+  // Start observing all focus changes.
+  FocusChangeListenerStub focus_listener(main_view());
+
+  // Steal the focus by creating a new App window.
+  SwitchToNewAppWindow();
+
+  // This causes the Assistant UI to close.
+  ASSERT_FALSE(window()->IsVisible());
+
+  // Now check that no child view of our UI received the focus.
+  for (const views::View* view : focus_listener.focused_views()) {
+    EXPECT_FALSE(page_view()->Contains(view))
+        << "Focus was received by Assistant view '" << view->GetClassName()
+        << "' while Assistant was closing";
+  }
+}
+
+TEST_F(AssistantPageViewTest, ShouldFocusTextDialogWhenOpeningWithHotkey) {
+  ShowAssistantUi(AssistantEntryPoint::kHotkey);
+
+  EXPECT_HAS_FOCUS(input_text_field());
+}
+
+TEST_F(AssistantPageViewTest, ShouldFocusTextDialogAfterSendingQuery) {
+  ShowAssistantUi();
+
+  SendQueryThroughTextField("The query");
+
+  EXPECT_HAS_FOCUS(input_text_field());
+}
+
+TEST_F(AssistantPageViewTest, ShouldFocusMicWhenOpeningWithHotword) {
+  ShowAssistantUi(AssistantEntryPoint::kHotword);
+
+  EXPECT_HAS_FOCUS(mic_view());
+}
+
+// Tests the |AssistantPageView| with tablet mode enabled.
+class AssistantPageViewTabletModeTest : public AssistantAshTestBase {
+ public:
+  AssistantPageViewTabletModeTest() = default;
+
+  void SetUp() override {
+    AssistantAshTestBase::SetUp();
+    SetTabletMode(true);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AssistantPageViewTabletModeTest);
+};
+
+TEST_F(AssistantPageViewTabletModeTest,
+       ShouldFocusMicWhenOpeningWithLongPressLauncher) {
+  ShowAssistantUi(AssistantEntryPoint::kLongPressLauncher);
+
+  EXPECT_HAS_FOCUS(mic_view());
+}
+
+TEST_F(AssistantPageViewTabletModeTest, ShouldFocusMicWhenOpeningWithHotword) {
+  ShowAssistantUi(AssistantEntryPoint::kHotword);
+
+  EXPECT_HAS_FOCUS(mic_view());
+}
+
+TEST_F(AssistantPageViewTabletModeTest,
+       ShouldFocusTextDialogAfterSendingQuery) {
+  ShowAssistantUi();
+
+  SendQueryThroughTextField("The query");
+
+  EXPECT_HAS_FOCUS(input_text_field());
+}
+
+TEST_F(AssistantPageViewTabletModeTest, ShouldHideKeyboardAfterSendingQuery) {
+  ShowAssistantUi();
+  ShowKeyboard();
+
+  SendQueryThroughTextField("The query");
+
+  EXPECT_FALSE(IsKeyboardShowing());
+}
+
+TEST_F(AssistantPageViewTabletModeTest,
+       ShouldShowKeyboardAfterTouchingInputTextbox) {
+  ShowAssistantUi();
+  EXPECT_FALSE(IsKeyboardShowing());
+
+  TapOnTextField();
+
+  EXPECT_TRUE(IsKeyboardShowing());
+}
+
 }  // namespace ash

ash/assistant/test/assistant_ash_test_base.cc

@@ -11,13 +11,65 @@
 #include "ash/app_list/views/assistant/assistant_page_view.h"
 #include "ash/app_list/views/contents_view.h"
 #include "ash/assistant/assistant_controller.h"
+#include "ash/keyboard/ui/keyboard_ui_controller.h"
+#include "ash/keyboard/ui/test/keyboard_test_util.h"
 #include "ash/public/cpp/app_list/app_list_features.h"
+#include "ash/public/cpp/keyboard/keyboard_switches.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller.h"
+#include "base/run_loop.h"
 #include "ui/compositor/scoped_animation_duration_scale_mode.h"
+#include "ui/views/controls/textfield/textfield.h"
 
 namespace ash {
 
+namespace {
+
+using chromeos::assistant::mojom::AssistantInteractionMetadata;
+using chromeos::assistant::mojom::AssistantInteractionType;
+
+gfx::Point GetPointInside(const views::View* view) {
+  return view->GetBoundsInScreen().CenterPoint();
+}
+
+bool CanProcessEvents(const views::View* view) {
+  const views::View* ancestor = view;
+  while (ancestor != nullptr) {
+    if (!ancestor->CanProcessEventsWithinSubtree())
+      return false;
+    ancestor = ancestor->parent();
+  }
+  return true;
+}
+
+views::View* GetDescendantViewWithNameOrNull(views::View* parent,
+                                             const std::string& name) {
+  for (views::View* child : parent->children()) {
+    if (child->GetClassName() == name)
+      return child;
+
+    views::View* descendant = GetDescendantViewWithNameOrNull(child, name);
+    if (descendant)
+      return descendant;
+  }
+  return nullptr;
+}
+
+// Recursively search for a descendant view with the given name.
+// Asserts if no such view exists.
+views::View* GetDescendantViewWithName(views::View* parent,
+                                       const std::string& name) {
+  views::View* descendant_maybe = GetDescendantViewWithNameOrNull(parent, name);
+  if (descendant_maybe == nullptr) {
+    ADD_FAILURE() << "View " << parent->GetClassName()
+                  << " has no descendant with name '" << name << "'.";
+  }
+  return descendant_maybe;
+}
+
+}  // namespace
+
 AssistantAshTestBase::AssistantAshTestBase() = default;
 
 AssistantAshTestBase::~AssistantAshTestBase() = default;
@@ -26,8 +78,15 @@ void AssistantAshTestBase::SetUp() {
   scoped_feature_list_.InitAndEnableFeature(
       app_list_features::kEnableEmbeddedAssistantUI);
 
+  // Enable virtual keyboard.
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      keyboard::switches::kEnableVirtualKeyboard);
+
   AshTestBase::SetUp();
 
+  // Make the display big enough to hold the app list.
+  UpdateDisplay("1024x768");
+
   // Enable Assistant in settings.
   Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetBoolean(
       chromeos::assistant::prefs::kAssistantEnabled, true);
@@ -41,19 +100,54 @@ void AssistantAshTestBase::SetUp() {
   AssistantState::Get()->NotifyStatusChanged(mojom::AssistantState::READY);
 
   DisableAnimations();
+
+  // Wait for virtual keyboard to load.
+  SetTouchKeyboardEnabled(true);
 }
 
 void AssistantAshTestBase::TearDown() {
+  windows_.clear();
+  SetTouchKeyboardEnabled(false);
   AshTestBase::TearDown();
   scoped_feature_list_.Reset();
   ReenableAnimations();
 }
 
-const AssistantMainView* AssistantAshTestBase::main_view() const {
+void AssistantAshTestBase::ShowAssistantUi(AssistantEntryPoint entry_point) {
+  if (entry_point == AssistantEntryPoint::kHotword) {
+    // If the Assistant is triggered via Hotword, the interaction is triggered
+    // by the Assistant service.
+    assistant_service()->StartVoiceInteraction();
+  } else {
+    // Otherwise, the interaction is triggered by a call to |ShowUi|.
+    controller_->ui_controller()->ShowUi(entry_point);
+  }
+  // Send all mojom messages to/from the assistant service.
+  base::RunLoop().RunUntilIdle();
+}
+
+void AssistantAshTestBase::CloseAssistantUi(AssistantExitPoint exit_point) {
+  controller_->ui_controller()->CloseUi(exit_point);
+}
+
+void AssistantAshTestBase::CloseLauncher() {
+  ash::Shell::Get()->app_list_controller()->ViewClosing();
+}
+
+void AssistantAshTestBase::SetTabletMode(bool enable) {
+  ash::Shell::Get()->tablet_mode_controller()->SetEnabledForTest(enable);
+}
+
+void AssistantAshTestBase::SetPreferVoice(bool prefer_voice) {
+  Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetBoolean(
+      chromeos::assistant::prefs::kAssistantLaunchWithMicOpen, prefer_voice);
+}
+
+AssistantMainView* AssistantAshTestBase::main_view() {
   return page_view()->GetMainViewForTest();
 }
 
-const AssistantPageView* AssistantAshTestBase::page_view() const {
+AssistantPageView* AssistantAshTestBase::page_view() {
   const int index = contents_view()->GetPageIndexForState(
       AppListState::kStateEmbeddedAssistant);
   return static_cast<AssistantPageView*>(contents_view()->GetPageView(index));
@@ -63,26 +157,66 @@ void AssistantAshTestBase::MockAssistantInteractionWithResponse(
     const std::string& response_text) {
   const std::string query = std::string("input text");
 
-  // |controller_| will blackhole any server response if it hasn't sent
-  // a request first, so we need to start by sending a request.
-  controller_->interaction_controller()->OnDialogPlateContentsCommitted(query);
-  // Then the server can start an interaction and return the response.
-  controller_->interaction_controller()->OnInteractionStarted(
-      chromeos::assistant::mojom::AssistantInteractionMetadata::New(
-          /*type=*/chromeos::assistant::mojom::AssistantInteractionType::kText,
-          /*query=*/query));
-  controller_->interaction_controller()->OnTextResponse(response_text);
-  controller_->interaction_controller()->OnInteractionFinished(
-      AssistantInteractionController::AssistantInteractionResolution::kNormal);
+  SendQueryThroughTextField(query);
+  assistant_service()->SetInteractionResponse(
+      InteractionResponse()
+          .AddTextResponse(response_text)
+          .AddResolution(InteractionResponse::Resolution::kNormal)
+          .Clone());
+
+  base::RunLoop().RunUntilIdle();
 }
 
-void AssistantAshTestBase::ShowAssistantUi(AssistantEntryPoint entry_point) {
-  LOG(ERROR) << "Assistant ash test base, showing UI...";
-  controller_->ui_controller()->ShowUi(entry_point);
-  LOG(ERROR) << "Assistant ash test base, done.";
+void AssistantAshTestBase::SendQueryThroughTextField(const std::string& query) {
+  if (!input_text_field()->HasFocus()) {
+    ADD_FAILURE()
+        << "The TextField should be focussed before we can send a query";
+  }
+
+  input_text_field()->SetText(base::ASCIIToUTF16(query));
+  // Send <return> to commit the query.
+  GetEventGenerator()->PressKey(ui::KeyboardCode::VKEY_RETURN,
+                                /*flags=*/ui::EF_NONE);
+}
+
+void AssistantAshTestBase::TapOnTextField() {
+  if (!CanProcessEvents(input_text_field()))
+    ADD_FAILURE() << "TextField can not process tap events";
+
+  GetEventGenerator()->GestureTapAt(GetPointInside(input_text_field()));
+}
+
+aura::Window* AssistantAshTestBase::SwitchToNewAppWindow() {
+  windows_.push_back(CreateAppWindow());
+
+  aura::Window* window = windows_.back().get();
+  window->SetName("<app-window>");
+  return window;
+}
+
+aura::Window* AssistantAshTestBase::window() {
+  return main_view()->GetWidget()->GetNativeWindow();
 }
 
-const ContentsView* AssistantAshTestBase::contents_view() const {
+views::Textfield* AssistantAshTestBase::input_text_field() {
+  views::View* result = GetDescendantViewWithName(main_view(), "Textfield");
+  return static_cast<views::Textfield*>(result);
+}
+
+views::View* AssistantAshTestBase::mic_view() {
+  return GetDescendantViewWithName(main_view(), "MicView");
+}
+
+void AssistantAshTestBase::ShowKeyboard() {
+  auto* keyboard_controller = keyboard::KeyboardUIController::Get();
+  keyboard_controller->ShowKeyboard(/*lock=*/false);
+}
+
+bool AssistantAshTestBase::IsKeyboardShowing() const {
+  return keyboard::IsKeyboardShowing();
+}
+
+ContentsView* AssistantAshTestBase::contents_view() {
   auto* app_list_view =
       Shell::Get()->app_list_controller()->presenter()->GetView();
 
@@ -92,15 +226,20 @@ const ContentsView* AssistantAshTestBase::contents_view() const {
   return app_list_view->app_list_main_view()->contents_view();
 }
 
+AssistantInteractionController* AssistantAshTestBase::interaction_controller() {
+  return controller_->interaction_controller();
+}
+
+TestAssistantService* AssistantAshTestBase::assistant_service() {
+  return ash_test_helper()->test_assistant_service();
+}
+
 void AssistantAshTestBase::DisableAnimations() {
   AppListView::SetShortAnimationForTesting(true);
 
-  // The |AssistantProcessIndicator| uses a cyclic animation,
-  // which will go in an infinite loop if the animation has ZERO_DURATION.
-  // So we use a NON_ZERO_DURATION instead.
   scoped_animation_duration_ =
       std::make_unique<ui::ScopedAnimationDurationScaleMode>(
-          ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+          ui::ScopedAnimationDurationScaleMode::ZERO_DURATION);
 }
 
 void AssistantAshTestBase::ReenableAnimations() {

ash/assistant/test/assistant_ash_test_base.h

@@ -6,18 +6,26 @@
 #define ASH_ASSISTANT_TEST_ASSISTANT_ASH_TEST_BASE_H_
 
 #include <memory>
+#include <vector>
 
 #include "ash/assistant/model/assistant_ui_model.h"
 #include "ash/test/ash_test_base.h"
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
 
+namespace views {
+class Textfield;
+class View;
+}  // namespace views
+
 namespace ash {
 
 class AssistantController;
+class AssistantInteractionController;
 class AssistantMainView;
 class AssistantPageView;
 class ContentsView;
+class TestAssistantService;
 
 // Helper class to make testing the Assistant Ash UI easier.
 class AssistantAshTestBase : public AshTestBase {
@@ -28,26 +36,69 @@ class AssistantAshTestBase : public AshTestBase {
   void SetUp() override;
   void TearDown() override;
 
+  // Show the Assistant UI. The optional |entry_point| can be used to emulate
+  // the different ways of launching the Assistant.
+  void ShowAssistantUi(
+      AssistantEntryPoint entry_point = AssistantEntryPoint::kUnspecified);
+  // Close the Assistant UI without closing the launcher. The optional
+  // |exit_point| can be used to emulate the different ways of closing the
+  // Assistant.
+  void CloseAssistantUi(
+      AssistantExitPoint exit_point = AssistantExitPoint::kUnspecified);
+  // Close the Assistant UI by closing the launcher.
+  void CloseLauncher();
+
+  void SetTabletMode(bool enable);
+
+  // Change the user setting controlling whether the user prefers voice or
+  // keyboard.
+  void SetPreferVoice(bool value);
+
   // Return the actual displayed Assistant main view.
   // Can only be used after |ShowAssistantUi| has been called.
-  const AssistantMainView* main_view() const;
+  AssistantMainView* main_view();
 
   // This is the top-level Assistant specific view.
   // Can only be used after |ShowAssistantUi| has been called.
-  const AssistantPageView* page_view() const;
+  AssistantPageView* page_view();
 
   // Spoof sending a request to the Assistant service,
   // and receiving |response_text| as a response to display.
   void MockAssistantInteractionWithResponse(const std::string& response_text);
 
-  void ShowAssistantUi(
-      AssistantEntryPoint entry_point = AssistantEntryPoint::kUnspecified);
+  // Simulate the user entering a query followed by <return>.
+  void SendQueryThroughTextField(const std::string& query);
+
+  // Simulate the user tapping on the text field.
+  void TapOnTextField();
+
+  // Create a new App window, and activate it. This will take the focus away
+  // from the Assistant UI (and force it to close).
+  // Returns a pointer to the newly created window.
+  // The window will be destroyed when the test if finished.
+  aura::Window* SwitchToNewAppWindow();
+
+  // Return the window containing the Assistant UI.
+  // Note that this window is shared for all components of the |AppList|.
+  aura::Window* window();
+
+  // Return the text field used for inputting new queries.
+  views::Textfield* input_text_field();
+  // Return the mic field used for dictating new queries.
+  views::View* mic_view();
+
+  // Show the on-screen keyboard.
+  void ShowKeyboard();
+  // Returns if the on-screen keyboard is being displayed.
+  bool IsKeyboardShowing() const;
 
  private:
-  const ContentsView* contents_view() const;
+  ContentsView* contents_view();
 
-  void DisableAnimations();
+  AssistantInteractionController* interaction_controller();
+  TestAssistantService* assistant_service();
 
+  void DisableAnimations();
   void ReenableAnimations();
 
   base::test::ScopedFeatureList scoped_feature_list_;
@@ -55,6 +106,8 @@ class AssistantAshTestBase : public AshTestBase {
   std::unique_ptr<ui::ScopedAnimationDurationScaleMode>
       scoped_animation_duration_;
 
+  std::vector<std::unique_ptr<aura::Window>> windows_;
+
   DISALLOW_COPY_AND_ASSIGN(AssistantAshTestBase);
 };
 

ash/assistant/test/test_assistant_service.cc

@@ -6,9 +6,171 @@
 
 #include <utility>
 
+#include "ash/assistant/assistant_interaction_controller.h"
+#include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
 namespace ash {
 
-TestAssistantService::TestAssistantService() : binding_(this) {}
+using chromeos::assistant::mojom::AssistantInteractionMetadata;
+using chromeos::assistant::mojom::AssistantInteractionResolution;
+using chromeos::assistant::mojom::AssistantInteractionSubscriber;
+using chromeos::assistant::mojom::AssistantInteractionType;
+
+// Subscriber that will ensure the LibAssistant contract is enforced.
+// More specifically, it will ensure that:
+//    - A conversation is finished before starting a new one.
+//    - No responses (text, card, ...) are sent before starting or after
+//    finishing an interaction.
+class SanityCheckSubscriber : public AssistantInteractionSubscriber {
+ public:
+  SanityCheckSubscriber() : receiver_(this) {}
+  ~SanityCheckSubscriber() override = default;
+
+  mojo::PendingRemote<AssistantInteractionSubscriber>
+  BindNewPipeAndPassRemote() {
+    return receiver_.BindNewPipeAndPassRemote();
+  }
+
+  // AssistantInteractionSubscriber implementation:
+  void OnInteractionStarted(
+      chromeos::assistant::mojom::AssistantInteractionMetadataPtr metadata)
+      override {
+    if (current_state_ == ConversationState::kInProgress) {
+      ADD_FAILURE()
+          << "Cannot start a new Assistant interaction without finishing the "
+             "previous interaction first.";
+    }
+    current_state_ = ConversationState::kInProgress;
+  }
+
+  void OnInteractionFinished(
+      AssistantInteractionResolution resolution) override {
+    // Note: We do not check |current_state_| here as this method can be called
+    // even if no interaction is in progress.
+    current_state_ = ConversationState::kFinished;
+  }
+
+  void OnHtmlResponse(const std::string& response,
+                      const std::string& fallback) override {
+    CheckResponse();
+  }
+
+  void OnSuggestionsResponse(
+      std::vector<chromeos::assistant::mojom::AssistantSuggestionPtr> response)
+      override {
+    CheckResponse();
+  }
+
+  void OnTextResponse(const std::string& response) override { CheckResponse(); }
+
+  void OnOpenUrlResponse(const ::GURL& url, bool in_background) override {
+    CheckResponse();
+  }
+
+  void OnOpenAppResponse(chromeos::assistant::mojom::AndroidAppInfoPtr app_info,
+                         OnOpenAppResponseCallback callback) override {
+    CheckResponse();
+  }
+
+  void OnSpeechRecognitionStarted() override {}
+
+  void OnSpeechRecognitionIntermediateResult(
+      const std::string& high_confidence_text,
+      const std::string& low_confidence_text) override {}
+
+  void OnSpeechRecognitionEndOfUtterance() override {}
+
+  void OnSpeechRecognitionFinalResult(
+      const std::string& final_result) override {}
+
+  void OnSpeechLevelUpdated(float speech_level) override {}
+
+  void OnTtsStarted(bool due_to_error) override {}
+
+  void OnWaitStarted() override {}
+
+ private:
+  void CheckResponse() {
+    if (current_state_ == ConversationState::kNotStarted)
+      ADD_FAILURE() << "Cannot send a response before starting an interaction.";
+    if (current_state_ == ConversationState::kFinished) {
+      ADD_FAILURE()
+          << "Cannot send a response after finishing the interaction.";
+    }
+  }
+
+  enum class ConversationState {
+    kNotStarted,
+    kInProgress,
+    kFinished,
+  };
+
+  ConversationState current_state_ = ConversationState::kNotStarted;
+  mojo::Receiver<AssistantInteractionSubscriber> receiver_;
+
+  DISALLOW_COPY_AND_ASSIGN(SanityCheckSubscriber);
+};
+
+class InteractionResponse::Response {
+ public:
+  Response() = default;
+  virtual ~Response() = default;
+
+  virtual std::unique_ptr<Response> Clone() const = 0;
+
+  virtual void SendTo(
+      chromeos::assistant::mojom::AssistantInteractionSubscriber* receiver) = 0;
+};
+
+class TextResponse : public InteractionResponse::Response {
+ public:
+  TextResponse(const std::string& text) : text_(text) {}
+  ~TextResponse() override = default;
+
+  std::unique_ptr<Response> Clone() const override {
+    return std::make_unique<TextResponse>(text_);
+  }
+
+  void SendTo(chromeos::assistant::mojom::AssistantInteractionSubscriber*
+                  receiver) override {
+    receiver->OnTextResponse(text_);
+  }
+
+ private:
+  std::string text_;
+
+  DISALLOW_COPY_AND_ASSIGN(TextResponse);
+};
+
+class ResolutionResponse : public InteractionResponse::Response {
+ public:
+  using Resolution = InteractionResponse::Resolution;
+
+  ResolutionResponse(Resolution resolution) : resolution_(resolution) {}
+  ~ResolutionResponse() override = default;
+
+  std::unique_ptr<Response> Clone() const override {
+    return std::make_unique<ResolutionResponse>(resolution_);
+  }
+
+  void SendTo(chromeos::assistant::mojom::AssistantInteractionSubscriber*
+                  receiver) override {
+    receiver->OnInteractionFinished(resolution_);
+  }
+
+ private:
+  Resolution resolution_;
+
+  DISALLOW_COPY_AND_ASSIGN(ResolutionResponse);
+};
+
+TestAssistantService::TestAssistantService()
+    : binding_(this),
+      sanity_check_subscriber_(std::make_unique<SanityCheckSubscriber>()) {
+  AddAssistantInteractionSubscriber(
+      sanity_check_subscriber_->BindNewPipeAndPassRemote());
+}
 
 TestAssistantService::~TestAssistantService() = default;
 
@@ -19,8 +181,139 @@ TestAssistantService::CreateInterfacePtrAndBind() {
   return ptr;
 }
 
-void TestAssistantService::CacheScreenContext(
+void TestAssistantService::SetInteractionResponse(
+    InteractionResponse&& response) {
+  interaction_response_ = std::move(response);
+}
+
+void TestAssistantService::StartCachedScreenContextInteraction() {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService ::StartEditReminderInteraction(
+    const std::string& client_id) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService ::StartMetalayerInteraction(const gfx::Rect& region) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService ::StartTextInteraction(const std::string& query,
+                                                 bool allow_tts) {
+  StartInteraction(AssistantInteractionType::kText, query);
+  SendInteractionResponse();
+}
+
+void TestAssistantService ::StartVoiceInteraction() {
+  StartInteraction(AssistantInteractionType::kVoice);
+  SendInteractionResponse();
+}
+
+void TestAssistantService ::StartWarmerWelcomeInteraction(
+    int num_warmer_welcome_triggered,
+    bool allow_tts) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService ::StopActiveInteraction(bool cancel_conversation) {
+  for (auto& subscriber : interaction_subscribers_) {
+    subscriber->OnInteractionFinished(
+        AssistantInteractionResolution::kInterruption);
+  }
+}
+
+void TestAssistantService ::AddAssistantInteractionSubscriber(
+    mojo::PendingRemote<AssistantInteractionSubscriber> subscriber) {
+  interaction_subscribers_.Add(
+      mojo::Remote<AssistantInteractionSubscriber>(std::move(subscriber)));
+}
+
+void TestAssistantService ::RetrieveNotification(
+    chromeos::assistant::mojom::AssistantNotificationPtr notification,
+    int action_index) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService ::DismissNotification(
+    chromeos::assistant::mojom::AssistantNotificationPtr notification) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService ::CacheScreenContext(
     CacheScreenContextCallback callback) {
   std::move(callback).Run();
 }
+
+void TestAssistantService ::OnAccessibilityStatusChanged(
+    bool spoken_feedback_enabled) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService ::SendAssistantFeedback(
+    chromeos::assistant::mojom::AssistantFeedbackPtr feedback) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService::StopAlarmTimerRinging() {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService::CreateTimer(base::TimeDelta duration) {
+  NOTIMPLEMENTED_LOG_ONCE();
+}
+
+void TestAssistantService::StartInteraction(
+    chromeos::assistant::mojom::AssistantInteractionType type,
+    const std::string& query) {
+  for (auto& subscriber : interaction_subscribers_) {
+    subscriber->OnInteractionStarted(
+        AssistantInteractionMetadata::New(type, query));
+  }
+}
+
+void TestAssistantService::SendInteractionResponse() {
+  InteractionResponse response = PopInteractionResponse();
+  for (auto& subscriber : interaction_subscribers_)
+    response.SendTo(subscriber.get());
+}
+
+InteractionResponse TestAssistantService::PopInteractionResponse() {
+  return std::move(interaction_response_);
+}
+
+InteractionResponse::InteractionResponse() = default;
+InteractionResponse::InteractionResponse(InteractionResponse&& other) = default;
+InteractionResponse& InteractionResponse::operator=(
+    InteractionResponse&& other) = default;
+InteractionResponse::~InteractionResponse() = default;
+
+InteractionResponse InteractionResponse::Clone() const {
+  InteractionResponse result{};
+  for (const auto& response : responses_)
+    result.AddResponse(response->Clone());
+  return result;
+}
+
+InteractionResponse& InteractionResponse::AddTextResponse(
+    const std::string& text) {
+  AddResponse(std::make_unique<TextResponse>(text));
+  return *this;
+}
+
+InteractionResponse& InteractionResponse::AddResolution(Resolution resolution) {
+  AddResponse(std::make_unique<ResolutionResponse>(resolution));
+  return *this;
+}
+
+void InteractionResponse::AddResponse(std::unique_ptr<Response> response) {
+  responses_.push_back(std::move(response));
+}
+
+void InteractionResponse::SendTo(
+    chromeos::assistant::mojom::AssistantInteractionSubscriber* receiver) {
+  for (auto& response : responses_)
+    response->SendTo(receiver);
+}
+
 }  // namespace ash

ash/assistant/test/test_assistant_service.h

@@ -11,9 +11,62 @@
 #include "base/timer/timer.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/remote_set.h"
 
 namespace ash {
 
+class SanityCheckSubscriber;
+
+// A response issued when an Assistant interaction is started.
+// Used both for text and voice interactions.  To build a response, simply
+// chain calls to the provided |AddXYZ| methods.
+//
+// Example usage:
+//     assistant_service()->SetInteractionResponse(
+//         InteractionResponse()
+//             .AddTextResponse("The response text")
+//             .AddResolution(InteractionResponse::Resolution::kNormal)
+//             .Clone());
+class InteractionResponse {
+ public:
+  using Resolution = chromeos::assistant::mojom::AssistantInteractionResolution;
+  class Response;
+
+  InteractionResponse();
+  InteractionResponse(InteractionResponse&& other);
+  InteractionResponse& operator=(InteractionResponse&& other);
+  ~InteractionResponse();
+
+  InteractionResponse Clone() const;
+
+  // A simple textual response.
+  InteractionResponse& AddTextResponse(const std::string& text);
+  // If used this will cause us to finish the interaction by passing the given
+  // |resolution| to |AssistantInteractionSubscriber::OnInteractionFinished|.
+  InteractionResponse& AddResolution(Resolution resolution);
+
+  void SendTo(
+      chromeos::assistant::mojom::AssistantInteractionSubscriber* receiver);
+
+ private:
+  void AddResponse(std::unique_ptr<Response> responses);
+
+  std::vector<std::unique_ptr<Response>> responses_;
+
+  DISALLOW_COPY_AND_ASSIGN(InteractionResponse);
+};
+
+// Dummy implementation of the Assistant service.
+// It behaves as if the Assistant service is up-and-running,
+// and will inform the |AssistantInteractionSubscriber| instances when
+// interactions start/stop.
+// Note it is up to the test developer to assure the interactions are valid.
+// The contract with LibAssistant specifies there is only one
+// "conversation turn" (what we call "interaction") at any given time, so you
+// must:
+//    - Finish a conversation before starting a new one.
+//    - Not send any responses (text, card, ...) before starting or after
+//      finishing an interaction.
 class TestAssistantService : public chromeos::assistant::mojom::Assistant {
  public:
   TestAssistantService();
@@ -21,35 +74,47 @@ class TestAssistantService : public chromeos::assistant::mojom::Assistant {
 
   chromeos::assistant::mojom::AssistantPtr CreateInterfacePtrAndBind();
 
+  // Set the response that will be invoked when the next interaction starts.
+  void SetInteractionResponse(InteractionResponse&& response);
+
   // mojom::Assistant overrides:
-  void StartCachedScreenContextInteraction() override {}
-  void StartEditReminderInteraction(const std::string& client_id) override {}
-  void StartMetalayerInteraction(const gfx::Rect& region) override {}
-  void StartTextInteraction(const std::string& query, bool allow_tts) override {
-  }
-  void StartVoiceInteraction() override {}
+  void StartCachedScreenContextInteraction() override;
+  void StartEditReminderInteraction(const std::string& client_id) override;
+  void StartMetalayerInteraction(const gfx::Rect& region) override;
+  void StartTextInteraction(const std::string& query, bool allow_tts) override;
+  void StartVoiceInteraction() override;
   void StartWarmerWelcomeInteraction(int num_warmer_welcome_triggered,
-                                     bool allow_tts) override {}
-  void StopActiveInteraction(bool cancel_conversation) override {}
+                                     bool allow_tts) override;
+  void StopActiveInteraction(bool cancel_conversation) override;
   void AddAssistantInteractionSubscriber(
       mojo::PendingRemote<
           chromeos::assistant::mojom::AssistantInteractionSubscriber>
-          subscriber) override {}
+          subscriber) override;
   void RetrieveNotification(
       chromeos::assistant::mojom::AssistantNotificationPtr notification,
-      int action_index) override {}
+      int action_index) override;
   void DismissNotification(chromeos::assistant::mojom::AssistantNotificationPtr
-                               notification) override {}
+                               notification) override;
   void CacheScreenContext(CacheScreenContextCallback callback) override;
   void ClearScreenContextCache() override {}
-  void OnAccessibilityStatusChanged(bool spoken_feedback_enabled) override {}
+  void OnAccessibilityStatusChanged(bool spoken_feedback_enabled) override;
   void SendAssistantFeedback(
-      chromeos::assistant::mojom::AssistantFeedbackPtr feedback) override {}
-  void StopAlarmTimerRinging() override {}
-  void CreateTimer(base::TimeDelta duration) override {}
+      chromeos::assistant::mojom::AssistantFeedbackPtr feedback) override;
+  void StopAlarmTimerRinging() override;
+  void CreateTimer(base::TimeDelta duration) override;
 
  private:
+  void StartInteraction(
+      chromeos::assistant::mojom::AssistantInteractionType type,
+      const std::string& query = {});
+  void SendInteractionResponse();
+  InteractionResponse PopInteractionResponse();
+
   mojo::Binding<chromeos::assistant::mojom::Assistant> binding_;
+  mojo::RemoteSet<chromeos::assistant::mojom::AssistantInteractionSubscriber>
+      interaction_subscribers_;
+  std::unique_ptr<SanityCheckSubscriber> sanity_check_subscriber_;
+  InteractionResponse interaction_response_;
 
   DISALLOW_COPY_AND_ASSIGN(TestAssistantService);
 };

ash/assistant/ui/main_stage/assistant_progress_indicator.cc

@@ -12,6 +12,7 @@
 #include "base/time/time.h"
 #include "ui/compositor/layer_animation_element.h"
 #include "ui/compositor/layer_animator.h"
+#include "ui/compositor/scoped_animation_duration_scale_mode.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/views/background.h"
@@ -49,6 +50,11 @@ int GetDotSpacingDip() {
              : kSpacingDip;
 }
 
+bool AreAnimationsEnabled() {
+  return ui::ScopedAnimationDurationScaleMode::duration_scale_mode() !=
+         ui::ScopedAnimationDurationScaleMode::ZERO_DURATION;
+}
+
 // DotBackground ---------------------------------------------------------------
 
 class DotBackground : public views::Background {
@@ -142,6 +148,12 @@ void AssistantProgressIndicator::VisibilityChanged(views::View* starting_from,
   transform.Translate(translation_dip, translation_dip);
   transform.Scale(scale_factor, scale_factor);
 
+  // Don't animate if animations are disabled (during unittests).
+  // Otherwise we get in an infinite loop due to the cyclic animation used here
+  // repeating over and over without pause.
+  if (!AreAnimationsEnabled())
+    return;
+
   base::TimeDelta start_offset;
   for (auto* child : children()) {
     if (!start_offset.is_zero()) {

ash/test/ash_test_helper.h

@@ -130,6 +130,10 @@ class AshTestHelper {
     return test_keyboard_controller_observer_.get();
   }
 
+  TestAssistantService* test_assistant_service() {
+    return assistant_service_.get();
+  }
+
   void reset_commandline() { command_line_.reset(); }
 
  private: