Skip to content

Commit

Permalink
[AppList] Fix positioning of close button when using search box
Browse files Browse the repository at this point in the history 
When activating the search box in tablet mode, the close button can
sometimes end up positioned incorrectly because the close button is
laid out next to the assistant button during the fade in/out
animations. To fix this, put the assistant button and the close+filter
buttons inside their own container views. These container views are then
parented by a fill layout manager view, so that the button containers
overlap are laid out on top of each other instead of side by side. This
makes it so that the button fade in/out animations overlap correctly.

Also, to simplify the fade in/out animations, animate the opacity of the
button containers instead of the buttons themselves. This also allows
button press events to pass to the correct button once the overlapping
containers are set to !visible after the fade out animations complete.

Finally increment pixel test revisions, and simplify some of the
numbering by skipping a revision for the case when IsJellyEnabled()
is false. Some revisions also had to be skipped because these pixel
tests were incremented previously but the change was reverted. This
simplification should make these revision changes easier to deal with
moving forward.

Bug: b/302376415
Change-Id: I820ecd3ece99736d69172b2e3a15171117f95bc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4914639
Commit-Queue: Matthew Mourgos <mmourgos@chromium.org>
Reviewed-by: Wen-Chien Wang <wcwang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1211916}
  • Loading branch information
Matthew Mourgos authored and Chromium LUCI CQ committed Oct 19, 2023
1 parent 0ad87e6 commit e14a104
Show file tree
Hide file tree
Showing 7 changed files with 124 additions and 95 deletions.
3 changes: 2 additions & 1 deletion ash/app_list/app_list_presenter_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1680,7 +1680,8 @@ TEST_P(AppListBubbleAndTabletTest, ClearSearchButtonClearsSearch) {

SearchBoxView* search_box_view = GetSearchBoxView();
search_box_view->GetWidget()->LayoutRootViewIfNecessary();
EXPECT_TRUE(search_box_view->close_button()->GetVisible());
EXPECT_TRUE(
search_box_view->filter_and_close_button_container()->GetVisible());
LeftClickOn(search_box_view->close_button());

EXPECT_EQ(std::vector<std::u16string>({u""}),
Expand Down
20 changes: 12 additions & 8 deletions ash/app_list/views/app_list_bubble_view_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,13 +543,15 @@ TEST_F(AppListBubbleViewTest, SearchBoxCloseButtonVisibleLongQuery) {
PressAndReleaseKey(ui::VKEY_A);

EXPECT_TRUE(GetSearchPage()->GetVisible());
EXPECT_TRUE(search_box_view->close_button()->GetVisible());
EXPECT_TRUE(
search_box_view->filter_and_close_button_container()->GetVisible());
for (int i = 0; i < 100; ++i) {
PressAndReleaseKey(ui::VKEY_A);
}
// Close button should be visible for long queries and within search box
// view bounds.
EXPECT_TRUE(search_box_view->close_button()->GetVisible());
EXPECT_TRUE(
search_box_view->filter_and_close_button_container()->GetVisible());
EXPECT_TRUE(search_box_view->GetBoundsInScreen().Contains(
search_box_view->close_button()->GetBoundsInScreen()));
}
Expand Down Expand Up @@ -623,13 +625,13 @@ TEST_F(AppListBubbleViewTest, SearchBoxShowsAssistantButton) {

// By default the assistant button is visible.
SearchBoxView* view = GetSearchBoxView();
EXPECT_TRUE(view->assistant_button()->GetVisible());
EXPECT_FALSE(view->close_button()->GetVisible());
EXPECT_TRUE(view->assistant_button_container()->GetVisible());
EXPECT_FALSE(view->filter_and_close_button_container()->GetVisible());

// Typing text shows the close button instead.
PressAndReleaseKey(ui::VKEY_A);
EXPECT_FALSE(view->assistant_button()->GetVisible());
EXPECT_TRUE(view->close_button()->GetVisible());
EXPECT_FALSE(view->assistant_button_container()->GetVisible());
EXPECT_TRUE(view->filter_and_close_button_container()->GetVisible());
}

TEST_F(AppListBubbleViewTest, ClickingAssistantButtonShowsAssistantPage) {
Expand Down Expand Up @@ -678,15 +680,17 @@ TEST_F(AppListBubbleViewTest, SearchBoxCloseButton) {
// Close button is visible after typing text.
SearchBoxView* search_box_view = GetSearchBoxView();
search_box_view->GetWidget()->LayoutRootViewIfNecessary();
EXPECT_TRUE(search_box_view->close_button()->GetVisible());
EXPECT_TRUE(
search_box_view->filter_and_close_button_container()->GetVisible());
EXPECT_FALSE(search_box_view->search_box()->GetText().empty());

// Clicking the close button clears the search, but the search box is still
// focused/active.
LeftClickOn(search_box_view->close_button());
EXPECT_EQ(std::vector<std::u16string>({u""}),
app_list_client->GetAndResetPastSearchQueries());
EXPECT_FALSE(search_box_view->close_button()->GetVisible());
EXPECT_FALSE(
search_box_view->filter_and_close_button_container()->GetVisible());
EXPECT_TRUE(search_box_view->search_box()->GetText().empty());
EXPECT_TRUE(search_box_view->search_box()->HasFocus());
EXPECT_TRUE(search_box_view->is_search_box_active());
Expand Down
3 changes: 2 additions & 1 deletion ash/app_list/views/app_list_main_view_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ INSTANTIATE_TEST_SUITE_P(All, AppListMainViewTest, testing::Bool());
TEST_P(AppListMainViewTest, CloseButtonInvisibleAfterCloseButtonClicked) {
PressAndReleaseKey(ui::VKEY_A);
ClickButton(search_box_view()->close_button());
EXPECT_FALSE(search_box_view()->close_button()->GetVisible());
EXPECT_FALSE(
search_box_view()->filter_and_close_button_container()->GetVisible());
}

// Tests that the search box becomes empty after close button is clicked.
Expand Down
9 changes: 3 additions & 6 deletions ash/app_list/views/app_list_view_pixeltest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -426,8 +426,7 @@ INSTANTIATE_TEST_SUITE_P(RTL,
// Verifies the default layout for tablet mode launcher.
TEST_P(AppListViewTabletPixelTest, Basic) {
EXPECT_TRUE(GetPixelDiffer()->CompareUiComponentsOnPrimaryScreen(
"tablet_launcher_basics",
/*revision_number=*/IsJellyEnabled() ? 7 : 6,
"tablet_launcher_basics", 9,
GetAppListTestHelper()->GetAppsContainerView()));
}

Expand All @@ -448,8 +447,7 @@ TEST_P(AppListViewTabletPixelTest, TopGradientZone) {
generator->MoveTouchBy(0, -40);

EXPECT_TRUE(GetPixelDiffer()->CompareUiComponentsOnPrimaryScreen(
"tablet_launcher_top_gradient_zone",
/*revision_number=*/IsJellyEnabled() ? 6 : 5,
"tablet_launcher_top_gradient_zone", 7,
GetAppListTestHelper()->GetAppsContainerView()));
}

Expand All @@ -470,8 +468,7 @@ TEST_P(AppListViewTabletPixelTest, BottomGradientZone) {
generator->MoveTouchBy(0, -90);

EXPECT_TRUE(GetPixelDiffer()->CompareUiComponentsOnPrimaryScreen(
"tablet_launcher_bottom_gradient_zone",
/*revision_number=*/IsJellyEnabled() ? 7 : 6,
"tablet_launcher_bottom_gradient_zone", 9,
GetAppListTestHelper()->GetAppsContainerView()));
}

Expand Down
52 changes: 27 additions & 25 deletions ash/app_list/views/search_box_view_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,9 @@ class SearchBoxViewTest : public views::test::WidgetTest,

void TearDown() override {
ui::ColorProviderManager::ResetForTesting();
if (app_list_view_)
if (app_list_view_) {
app_list_view_->GetWidget()->Close();
}
widget_->CloseNow();
views::test::WidgetTest::TearDown();
}
Expand Down Expand Up @@ -279,13 +280,13 @@ TEST_F(SearchBoxViewTest, SearchBoxTextUsesAppListSearchBoxTextColor) {

// Tests that the close button is invisible by default.
TEST_F(SearchBoxViewTest, CloseButtonInvisibleByDefault) {
EXPECT_FALSE(view()->close_button()->GetVisible());
EXPECT_FALSE(view()->filter_and_close_button_container()->GetVisible());
}

// Tests that the close button becomes visible after typing in the search box.
TEST_F(SearchBoxViewTest, CloseButtonVisibleAfterTyping) {
KeyPress(ui::VKEY_A);
EXPECT_TRUE(view()->close_button()->GetVisible());
EXPECT_TRUE(view()->filter_and_close_button_container()->GetVisible());
}

// Tests that the filter button is not created if the image search feature is
Expand All @@ -303,7 +304,7 @@ TEST_F(SearchBoxViewTest, FilterButtonNotCreatedWithDisabledImageSearch) {
// activated (in zero state).
TEST_F(SearchBoxViewTest, CloseButtonVisibleInZeroStateSearchBox) {
SetSearchBoxActive(true, ui::ET_MOUSE_PRESSED);
EXPECT_FALSE(view()->close_button()->GetVisible());
EXPECT_FALSE(view()->filter_and_close_button_container()->GetVisible());
}

// TODO(crbug.com/1446550): Re-enable this test
Expand Down Expand Up @@ -579,16 +580,16 @@ TEST_F(SearchBoxViewTest,
// Tile results are not created when testing productivity launcher.
selection = GetResultSelectionController()->selected_result();

EXPECT_EQ(u"testing almost", selection->result()->title());
EXPECT_EQ(u"testing almost", selection->result()->title());

// New result can override the default selection.
CreateSearchResultAt(0, ash::SearchResultDisplayType::kList, 1.0, u"test",
std::u16string(),
ash::AppListSearchResultCategory::kWeb);
base::RunLoop().RunUntilIdle();
// New result can override the default selection.
CreateSearchResultAt(0, ash::SearchResultDisplayType::kList, 1.0, u"test",
std::u16string(),
ash::AppListSearchResultCategory::kWeb);
base::RunLoop().RunUntilIdle();

selection = GetResultSelectionController()->selected_result();
EXPECT_EQ(u"test", selection->result()->title());
selection = GetResultSelectionController()->selected_result();
EXPECT_EQ(u"test", selection->result()->title());
}

// Tests that the default selection is reset after resetting and reactivating
Expand Down Expand Up @@ -668,18 +669,18 @@ class SearchBoxViewAssistantButtonTest : public SearchBoxViewTest {

// Tests that the assistant button is visible by default.
TEST_F(SearchBoxViewAssistantButtonTest, AssistantButtonVisibleByDefault) {
EXPECT_TRUE(view()->assistant_button()->GetVisible());
EXPECT_TRUE(view()->assistant_button_container()->GetVisible());
}

// Tests that the assistant button is invisible after typing in the search box,
// and comes back when search box is empty.
TEST_F(SearchBoxViewAssistantButtonTest,
AssistantButtonChangeVisibilityWithTyping) {
KeyPress(ui::VKEY_A);
EXPECT_FALSE(view()->assistant_button()->GetVisible());
EXPECT_FALSE(view()->assistant_button_container()->GetVisible());

KeyPress(ui::VKEY_BACK);
EXPECT_TRUE(view()->assistant_button()->GetVisible());
EXPECT_TRUE(view()->assistant_button_container()->GetVisible());
}

class SearchBoxViewFilterButtonTest : public SearchBoxViewTest {
Expand All @@ -697,13 +698,13 @@ class SearchBoxViewFilterButtonTest : public SearchBoxViewTest {

// Tests that the filter button is invisible by default.
TEST_F(SearchBoxViewFilterButtonTest, FilterButtonInvisibleByDefault) {
EXPECT_FALSE(view()->filter_button()->GetVisible());
EXPECT_FALSE(view()->filter_button()->parent()->GetVisible());
}

// Tests that the filter button becomes visible after typing in the search box.
TEST_F(SearchBoxViewFilterButtonTest, FilterButtonVisibleAfterTyping) {
KeyPress(ui::VKEY_A);
EXPECT_TRUE(view()->filter_button()->GetVisible());
EXPECT_TRUE(view()->filter_button()->parent()->GetVisible());
}

class SearchBoxViewAutocompleteTest : public SearchBoxViewTest {
Expand Down Expand Up @@ -1097,24 +1098,25 @@ TEST_F(SearchBoxViewAnimationTest, SearchBoxImageButtonAnimations) {

// Initially the assistant button should be shown, and the close button
// hidden.
EXPECT_FALSE(search_box->close_button()->GetVisible());
EXPECT_TRUE(search_box->assistant_button()->GetVisible());
EXPECT_FALSE(search_box->filter_and_close_button_container()->GetVisible());
EXPECT_TRUE(search_box->assistant_button_container()->GetVisible());

// Set search box to active state.
search_box->SetSearchBoxActive(true, ui::ET_MOUSE_PRESSED);

// Close button should be fading in.
EXPECT_TRUE(search_box->close_button()->GetVisible());
auto* close_animator = search_box->close_button()->layer()->GetAnimator();
EXPECT_TRUE(search_box->filter_and_close_button_container()->GetVisible());
auto* close_animator =
search_box->filter_and_close_button_container()->layer()->GetAnimator();
ASSERT_TRUE(close_animator);
EXPECT_TRUE(close_animator->IsAnimatingProperty(
ui::LayerAnimationElement::AnimatableProperty::OPACITY));
EXPECT_EQ(close_animator->GetTargetOpacity(), 1.0f);

// Assistant button should be fading out.
EXPECT_TRUE(search_box->assistant_button()->GetVisible());
EXPECT_TRUE(search_box->assistant_button_container()->GetVisible());
auto* assistant_animator =
search_box->assistant_button()->layer()->GetAnimator();
search_box->assistant_button_container()->layer()->GetAnimator();
EXPECT_TRUE(assistant_animator->IsAnimatingProperty(
ui::LayerAnimationElement::AnimatableProperty::OPACITY));
EXPECT_EQ(assistant_animator->GetTargetOpacity(), 0.0f);
Expand All @@ -1123,13 +1125,13 @@ TEST_F(SearchBoxViewAnimationTest, SearchBoxImageButtonAnimations) {
search_box->SetSearchBoxActive(false, ui::ET_MOUSE_PRESSED);

// Close button should be fading out.
EXPECT_TRUE(search_box->close_button()->GetVisible());
EXPECT_TRUE(search_box->filter_and_close_button_container()->GetVisible());
EXPECT_TRUE(close_animator->IsAnimatingProperty(
ui::LayerAnimationElement::AnimatableProperty::OPACITY));
EXPECT_EQ(close_animator->GetTargetOpacity(), 0.0f);

// Assistant button should be fading in.
EXPECT_TRUE(search_box->assistant_button()->GetVisible());
EXPECT_TRUE(search_box->assistant_button_container()->GetVisible());
ASSERT_TRUE(assistant_animator);
EXPECT_TRUE(assistant_animator->IsAnimatingProperty(
ui::LayerAnimationElement::AnimatableProperty::OPACITY));
Expand Down

0 comments on commit e14a104

Please sign in to comment.