[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}
diff --git a/ash/app_list/app_list_presenter_delegate_impl.cc b/ash/app_list/app_list_presenter_delegate_impl.cc
index 093636d..944884a 100644
--- a/ash/app_list/app_list_presenter_delegate_impl.cc
+++ b/ash/app_list/app_list_presenter_delegate_impl.cc
@@ -262,20 +262,6 @@
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);
- }
}
////////////////////////////////////////////////////////////////////////////////
diff --git a/ash/app_list/app_list_presenter_impl.cc b/ash/app_list/app_list_presenter_impl.cc
index 5618a7c..956ac14 100644
--- a/ash/app_list/app_list_presenter_impl.cc
+++ b/ash/app_list/app_list_presenter_impl.cc
@@ -172,10 +172,6 @@
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,51 +419,56 @@
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();
+ if (!view_ || !is_target_visibility_show_)
+ return;
- // 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);
+ 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();
- const bool app_list_lost_focus =
- gained_focus ? gained_focus_hides_app_list
- : (lost_focus && applist_container->Contains(lost_focus));
+ // 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);
- if (delegate_->IsTabletMode()) {
- const bool is_shown = !app_list_lost_focus;
- if (is_shown != delegate_->IsVisible()) {
- if (is_shown)
- view_->OnHomeLauncherGainingFocusWithoutAnimation();
- else
- HandleCloseOpenSearchBox();
+ const bool app_list_lost_focus =
+ gained_focus ? gained_focus_hides_app_list
+ : (lost_focus && applist_container->Contains(lost_focus));
- OnVisibilityWillChange(is_shown, GetDisplayId());
- OnVisibilityChanged(is_shown, GetDisplayId());
+ 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();
}
- }
- if (applist_window->Contains(gained_focus))
- base::RecordAction(base::UserMetricsAction("AppList_WindowFocused"));
-
- if (app_list_lost_focus && !switches::ShouldNotDismissOnBlur() &&
- !delegate_->IsTabletMode()) {
- Dismiss(base::TimeTicks());
+ OnVisibilityWillChange(is_shown, GetDisplayId());
+ OnVisibilityChanged(is_shown, GetDisplayId());
}
}
+
+ if (applist_window->Contains(gained_focus))
+ base::RecordAction(base::UserMetricsAction("AppList_WindowFocused"));
+
+ if (app_list_lost_focus && !switches::ShouldNotDismissOnBlur() &&
+ !delegate_->IsTabletMode()) {
+ Dismiss(base::TimeTicks());
+ }
}
////////////////////////////////////////////////////////////////////////////////
diff --git a/ash/app_list/app_list_presenter_impl.h b/ash/app_list/app_list_presenter_impl.h
index 22839cd0..9da0a18d 100644
--- a/ash/app_list/app_list_presenter_impl.h
+++ b/ash/app_list/app_list_presenter_impl.h
@@ -69,10 +69,6 @@
// 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.
diff --git a/ash/app_list/views/app_list_view.cc b/ash/app_list/views/app_list_view.cc
index e3e183c2..76f91a1 100644
--- a/ash/app_list/views/app_list_view.cc
+++ b/ash/app_list/views/app_list_view.cc
@@ -761,7 +761,10 @@
}
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 @@
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(
diff --git a/ash/app_list/views/assistant/assistant_page_view_unittest.cc b/ash/app_list/views/assistant/assistant_page_view_unittest.cc
index 7840a80..d44055ba 100644
--- a/ash/app_list/views/assistant/assistant_page_view_unittest.cc
+++ b/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 @@
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 @@
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
diff --git a/ash/app_list/views/assistant/assistant_test_api_impl.cc b/ash/app_list/views/assistant/assistant_test_api_impl.cc
index 71c319a..ceb574b 100644
--- a/ash/app_list/views/assistant/assistant_test_api_impl.cc
+++ b/ash/app_list/views/assistant/assistant_test_api_impl.cc
@@ -89,6 +89,10 @@
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);
diff --git a/ash/app_list/views/assistant/assistant_test_api_impl.h b/ash/app_list/views/assistant/assistant_test_api_impl.h
index 6094f31..b6b6559 100644
--- a/ash/app_list/views/assistant/assistant_test_api_impl.h
+++ b/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 @@
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();
diff --git a/ash/assistant/test/assistant_ash_test_base.cc b/ash/assistant/test/assistant_ash_test_base.cc
index c936c5f..dd052bb5 100644
--- a/ash/assistant/test/assistant_ash_test_base.cc
+++ b/ash/assistant/test/assistant_ash_test_base.cc
@@ -126,6 +126,10 @@
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 @@
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",
diff --git a/ash/assistant/test/assistant_ash_test_base.h b/ash/assistant/test/assistant_ash_test_base.h
index 3c9da74dd..84c8ca4 100644
--- a/ash/assistant/test/assistant_ash_test_base.h
+++ b/ash/assistant/test/assistant_ash_test_base.h
@@ -53,6 +53,9 @@
// 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 @@
// 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);
diff --git a/ash/public/cpp/test/assistant_test_api.h b/ash/public/cpp/test/assistant_test_api.h
index 5fe4fdbf..b1bdefb 100644
--- a/ash/public/cpp/test/assistant_test_api.h
+++ b/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 @@
// 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
