[Embedded] Dismiss Assistant UI properly.

This CL fixed the bug that embedded UI will show on top of any
newly opened window instead of dismissing in tablet mode. Right
now in tablet mode the Assistant UI will dismiss when tapping on
empty space within AppList, or when other new window opens.

Tapping on the shelf will *not* hide Assistant UI by default, as
we aim to keep the behavior consistent among AppList pages (e.g.
search result page doesn't dismiss when tapping on shelf).

Misc:
Fix linter warning.

Bug: b/142549681
Test: run unittests added in this change.
Change-Id: I18a2424903f0523489a5fdea86cd29356623e0df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1925368
Commit-Queue: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719771}

ash/app_list/app_list_presenter_delegate_impl.cc

@@ -262,20 +262,6 @@ void AppListPresenterDelegateImpl::ProcessLocatedEvent(
     if (!hotseat_window || !hotseat_window->Contains(target))
       presenter_->Dismiss(event->time_stamp());
   }
-
-  if (IsTabletMode() && presenter_->IsShowingEmbeddedAssistantUI()) {
-    auto* contents_view =
-        presenter_->GetView()->app_list_main_view()->contents_view();
-    if (contents_view->bounds().Contains(event->location())) {
-      // Keep Assistant open if event happen inside.
-      return;
-    }
-
-    // Touching anywhere else closes Assistant.
-    view_->Back();
-    view_->search_box_view()->ClearSearch();
-    view_->search_box_view()->SetSearchBoxActive(false, ui::ET_UNKNOWN);
-  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////

ash/app_list/app_list_presenter_impl.cc

@@ -172,10 +172,6 @@ bool AppListPresenterImpl::HandleCloseOpenFolder() {
   return is_target_visibility_show_ && view_ && view_->HandleCloseOpenFolder();
 }
 
-bool AppListPresenterImpl::HandleCloseOpenSearchBox() {
-  return view_ && view_->HandleCloseOpenSearchBox();
-}
-
 ash::ShelfAction AppListPresenterImpl::ToggleAppList(
     int64_t display_id,
     AppListShowSource show_source,
@@ -423,50 +419,55 @@ void AppListPresenterImpl::OnVisibilityWillChange(bool visible,
 
 void AppListPresenterImpl::OnWindowFocused(aura::Window* gained_focus,
                                            aura::Window* lost_focus) {
-  if (view_ && is_target_visibility_show_) {
-    int gained_focus_container_id = kShellWindowId_Invalid;
-    if (gained_focus) {
-      gained_focus_container_id = gained_focus->id();
-      const aura::Window* container =
-          delegate_->GetContainerForWindow(gained_focus);
-      if (container)
-        gained_focus_container_id = container->id();
-    }
-    aura::Window* applist_window = view_->GetWidget()->GetNativeView();
-    const aura::Window* applist_container = applist_window->parent();
-
-    // An AppList dialog window, or a child window of the system tray, may
-    // take focus from the AppList window. Don't consider this a visibility
-    // change since the app list is still visible for the most part.
-    const bool gained_focus_hides_app_list =
-        gained_focus_container_id != kShellWindowId_Invalid &&
-        !base::Contains(kIdsOfContainersThatWontHideAppList,
-                        gained_focus_container_id);
-
-    const bool app_list_lost_focus =
-        gained_focus ? gained_focus_hides_app_list
-                     : (lost_focus && applist_container->Contains(lost_focus));
-
-    if (delegate_->IsTabletMode()) {
-      const bool is_shown = !app_list_lost_focus;
-      if (is_shown != delegate_->IsVisible()) {
-        if (is_shown)
-          view_->OnHomeLauncherGainingFocusWithoutAnimation();
-        else
-          HandleCloseOpenSearchBox();
-
-        OnVisibilityWillChange(is_shown, GetDisplayId());
-        OnVisibilityChanged(is_shown, GetDisplayId());
+  if (!view_ || !is_target_visibility_show_)
+    return;
+
+  int gained_focus_container_id = kShellWindowId_Invalid;
+  if (gained_focus) {
+    gained_focus_container_id = gained_focus->id();
+    const aura::Window* container =
+        delegate_->GetContainerForWindow(gained_focus);
+    if (container)
+      gained_focus_container_id = container->id();
+  }
+  aura::Window* applist_window = view_->GetWidget()->GetNativeView();
+  const aura::Window* applist_container = applist_window->parent();
+
+  // An AppList dialog window, or a child window of the system tray, may
+  // take focus from the AppList window. Don't consider this a visibility
+  // change since the app list is still visible for the most part.
+  const bool gained_focus_hides_app_list =
+      gained_focus_container_id != kShellWindowId_Invalid &&
+      !base::Contains(kIdsOfContainersThatWontHideAppList,
+                      gained_focus_container_id);
+
+  const bool app_list_lost_focus =
+      gained_focus ? gained_focus_hides_app_list
+                   : (lost_focus && applist_container->Contains(lost_focus));
+
+  if (delegate_->IsTabletMode()) {
+    const bool is_shown = !app_list_lost_focus;
+    if (is_shown != delegate_->IsVisible()) {
+      if (is_shown) {
+        view_->OnHomeLauncherGainingFocusWithoutAnimation();
+      } else {
+        // In tablet mode, when |AppList| lost focus after other new App window
+        // opened, we should perform "back" action on the active page, e.g.
+        // close the search box or the embedded Assistant UI if it's opened.
+        view_->Back();
       }
+
+      OnVisibilityWillChange(is_shown, GetDisplayId());
+      OnVisibilityChanged(is_shown, GetDisplayId());
     }
+  }
 
-    if (applist_window->Contains(gained_focus))
-      base::RecordAction(base::UserMetricsAction("AppList_WindowFocused"));
+  if (applist_window->Contains(gained_focus))
+    base::RecordAction(base::UserMetricsAction("AppList_WindowFocused"));
 
-    if (app_list_lost_focus && !switches::ShouldNotDismissOnBlur() &&
-        !delegate_->IsTabletMode()) {
-      Dismiss(base::TimeTicks());
-    }
+  if (app_list_lost_focus && !switches::ShouldNotDismissOnBlur() &&
+      !delegate_->IsTabletMode()) {
+    Dismiss(base::TimeTicks());
   }
 }
 

ash/app_list/app_list_presenter_impl.h

@@ -69,10 +69,6 @@ class APP_LIST_EXPORT AppListPresenterImpl
   // folder was closed.
   bool HandleCloseOpenFolder();
 
-  // If app list has an open search box, close it. Returns whether an open
-  // search box was closed.
-  bool HandleCloseOpenSearchBox();
-
   // Show the app list if it is visible, hide it if it is hidden. If
   // |event_time_stamp| is not 0, it means |ToggleAppList()| was triggered by
   // one of the AppListShowSources: kSearchKey or kShelfButton.

ash/app_list/views/app_list_view.cc

@@ -761,7 +761,10 @@ bool AppListView::HandleCloseOpenSearchBox() {
 }
 
 bool AppListView::Back() {
-  return app_list_main_view_->contents_view()->Back();
+  if (app_list_main_view_)
+    return app_list_main_view_->contents_view()->Back();
+
+  return false;
 }
 
 void AppListView::OnPaint(gfx::Canvas* canvas) {
@@ -919,8 +922,8 @@ void AppListView::HandleClickOrTap(ui::LocatedEvent* event) {
     if (!is_tablet_mode())
       return;
 
-    // Home launcher is shown on top of wallpaper with trasparent background. So
-    // trigger the wallpaper context menu for the same events.
+    // Home launcher is shown on top of wallpaper with transparent background.
+    // So trigger the wallpaper context menu for the same events.
     gfx::Point onscreen_location(event->location());
     ConvertPointToScreen(this, &onscreen_location);
     delegate_->ShowWallpaperContextMenu(

ash/app_list/views/assistant/assistant_page_view_unittest.cc

@@ -7,6 +7,7 @@
 #include "ash/assistant/ui/assistant_ui_constants.h"
 #include "base/run_loop.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom-shared.h"
+#include "ui/events/event.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/focus/focus_manager.h"
 
@@ -69,6 +70,21 @@ class FocusChangeListenerStub : public views::FocusChangeListener {
   DISALLOW_COPY_AND_ASSIGN(FocusChangeListenerStub);
 };
 
+// Simply constructs a GestureEvent for test.
+class GestureEventForTest : public ui::GestureEvent {
+ public:
+  GestureEventForTest(const gfx::Point& location,
+                      ui::GestureEventDetails details)
+      : GestureEvent(location.x(),
+                     location.y(),
+                     /*flags=*/ui::EF_NONE,
+                     base::TimeTicks(),
+                     details) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(GestureEventForTest);
+};
+
 class AssistantPageViewTest : public AssistantAshTestBase {
  public:
   AssistantPageViewTest() = default;
@@ -339,4 +355,51 @@ TEST_F(AssistantPageViewTabletModeTest,
   EXPECT_TRUE(IsKeyboardShowing());
 }
 
+TEST_F(AssistantPageViewTabletModeTest,
+       ShouldDismissWhenTappingOutsideWithinAppListView) {
+  ShowAssistantUi();
+  EXPECT_TRUE(IsVisible());
+
+  // Tapping position should be outside the Assistant UI but still inside the
+  // bounds of |AppList| window.
+  gfx::Point origin = page_view()->origin();
+  gfx::Point point_outside = gfx::Point(origin.x() - 10, origin.y());
+  EXPECT_TRUE(window()->bounds().Contains(point_outside));
+  EXPECT_FALSE(page_view()->bounds().Contains(point_outside));
+
+  // Tapping outside on the empty region of |AppListView| should dismiss the
+  // Assistant UI.
+  GestureEventForTest tap_outside(point_outside,
+                                  ui::GestureEventDetails(ui::ET_GESTURE_TAP));
+  app_list_view()->OnGestureEvent(&tap_outside);
+  EXPECT_FALSE(IsVisible());
+}
+
+TEST_F(AssistantPageViewTabletModeTest,
+       ShouldDismissIfLostFocusWhenOtherAppWindowOpens) {
+  ShowAssistantUi();
+  EXPECT_TRUE(IsVisible());
+
+  // Creates a new window to steal the focus should dismiss the Assistant UI.
+  SwitchToNewAppWindow();
+  EXPECT_FALSE(IsVisible());
+}
+
+TEST_F(AssistantPageViewTabletModeTest, ShouldNotDismissWhenTappingInside) {
+  ShowAssistantUi();
+  EXPECT_TRUE(IsVisible());
+
+  // Tapping position should be inside the Assistant UI.
+  gfx::Point origin = page_view()->origin();
+  gfx::Point point_inside = gfx::Point(origin.x() + 10, origin.y() + 10);
+  EXPECT_TRUE(page_view()->bounds().Contains(point_inside));
+
+  // Tapping inside should not dismiss the Assistant UI.
+  GestureEventForTest tap_inside(point_inside,
+                                 ui::GestureEventDetails(ui::ET_GESTURE_TAP));
+  page_view()->OnGestureEvent(&tap_inside);
+
+  EXPECT_TRUE(IsVisible());
+}
+
 }  // namespace ash

ash/app_list/views/assistant/assistant_test_api_impl.cc

@@ -89,6 +89,10 @@ aura::Window* AssistantTestApiImpl::window() {
   return main_view()->GetWidget()->GetNativeWindow();
 }
 
+views::View* AssistantTestApiImpl::app_list_view() {
+  return static_cast<views::View*>(contents_view()->app_list_view());
+}
+
 void AssistantTestApiImpl::EnableAssistant() {
   Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetBoolean(
       chromeos::assistant::prefs::kAssistantEnabled, true);

ash/app_list/views/assistant/assistant_test_api_impl.h

@@ -6,6 +6,7 @@
 #define ASH_APP_LIST_VIEWS_ASSISTANT_ASSISTANT_TEST_API_IMPL_H_
 
 #include <memory>
+#include <string>
 
 #include "ash/public/cpp/test/assistant_test_api.h"
 #include "base/macros.h"
@@ -43,6 +44,7 @@ class AssistantTestApiImpl : public AssistantTestApi {
   views::View* voice_input_toggle() override;
   views::View* keyboard_input_toggle() override;
   aura::Window* window() override;
+  views::View* app_list_view() override;
 
  private:
   void EnableAnimations();

ash/assistant/test/assistant_ash_test_base.cc

@@ -126,6 +126,10 @@ void AssistantAshTestBase::SetPreferVoice(bool prefer_voice) {
   test_api_->SetPreferVoice(prefer_voice);
 }
 
+bool AssistantAshTestBase::IsVisible() {
+  return test_api_->IsVisible();
+}
+
 views::View* AssistantAshTestBase::main_view() {
   return test_api_->main_view();
 }
@@ -134,6 +138,10 @@ views::View* AssistantAshTestBase::page_view() {
   return test_api_->page_view();
 }
 
+views::View* AssistantAshTestBase::app_list_view() {
+  return test_api_->app_list_view();
+}
+
 void AssistantAshTestBase::MockAssistantInteractionWithResponse(
     const std::string& response_text) {
   MockAssistantInteractionWithQueryAndResponse(/*query=*/"input text",

ash/assistant/test/assistant_ash_test_base.h

@@ -53,6 +53,9 @@ class AssistantAshTestBase : public AshTestBase {
   // keyboard.
   void SetPreferVoice(bool value);
 
+  // Return true if the Assistant UI is visible.
+  bool IsVisible();
+
   // Return the actual displayed Assistant main view.
   // Can only be used after |ShowAssistantUi| has been called.
   views::View* main_view();
@@ -61,6 +64,10 @@ class AssistantAshTestBase : public AshTestBase {
   // Can only be used after |ShowAssistantUi| has been called.
   views::View* page_view();
 
+  // Return the app list view hosting the Assistant page view.
+  // Can only be used after |ShowAssistantUi| has been called.
+  views::View* app_list_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);

ash/public/cpp/test/assistant_test_api.h

@@ -6,6 +6,7 @@
 #define ASH_PUBLIC_CPP_TEST_ASSISTANT_TEST_API_H_
 
 #include <memory>
+#include <string>
 
 #include "ash/ash_export.h"
 
@@ -78,6 +79,10 @@ class ASH_EXPORT AssistantTestApi {
   // Returns the window containing the Assistant UI.
   // Note that this window is shared for all components of the |AppList|.
   virtual aura::Window* window() = 0;
+
+  // Returns the app list view hosting the Assistant UI.
+  // Can only be used after the Assistant UI has been shown at least once.
+  virtual views::View* app_list_view() = 0;
 };
 
 }  // namespace ash