Skip to content

Commit

Permalink
Merge 115: Revert handling of transient children when bubble launcher…
Browse files Browse the repository at this point in the history
… hides

Reverts CL:4456250 and CL:4518164 that were attempting to unparent and
close transient child windows when hiding bubble launcher to ensure
uninstall dialogs parented to the app list window gets closed.

This ended up causing a crash in touch_selection_controller
implementation (which does not expect the editing handle view widget to
get closed before the associated texfield loses focus). Arguably,
TouchSelectionController could be made more robust to this cases, or we
could have app list bubble presenter close transient children later on
(after the bubble is actually hidden), but given that this needs to be
fixed on other branches, reverting to the old behaviour (which though
not ideal was around for a while) is safer.

BUG=1448682, b:264690245

(cherry picked from commit 7e17e8a)

Change-Id: Ic330407b7cc9ea9b15033356fc775ae77aa0f4fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4569315
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1149928}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4590689
Cr-Commit-Position: refs/branch-heads/5790@{#359}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Toni Barzic authored and Chromium LUCI CQ committed Jun 5, 2023
1 parent 0d05c9c commit 7d45bad
Show file tree
Hide file tree
Showing 3 changed files with 0 additions and 97 deletions.
18 changes: 0 additions & 18 deletions ash/app_list/app_list_bubble_presenter.cc
Expand Up @@ -7,7 +7,6 @@
#include <algorithm>
#include <memory>
#include <utility>
#include <vector>

#include "ash/app_list/app_list_bubble_event_filter.h"
#include "ash/app_list/app_list_controller_impl.h"
Expand Down Expand Up @@ -43,7 +42,6 @@
#include "ui/views/view.h"
#include "ui/views/widget/widget.h"
#include "ui/wm/core/coordinate_conversion.h"
#include "ui/wm/core/transient_window_manager.h"
#include "ui/wm/public/activation_client.h"

namespace ash {
Expand Down Expand Up @@ -316,22 +314,6 @@ void AppListBubblePresenter::Dismiss() {
if (bubble_view_) {
aura::Window* bubble_window = bubble_view_->GetWidget()->GetNativeWindow();
DCHECK(bubble_window);

// Close all transient child windows in the app list (e.g. uninstall dialog)
// when the app list is dismissed.
auto* manager = ::wm::TransientWindowManager::GetOrCreate(bubble_window);
if (manager) {
std::vector<aura::Window*> children = manager->transient_children();
for (auto* child : children) {
views::Widget* child_widget =
views::Widget::GetWidgetForNativeWindow(child);
if (child_widget) {
child_widget->CloseWithReason(
views::Widget::ClosedReason::kUnspecified);
}
}
}

Shelf* shelf = Shelf::ForWindow(bubble_window);
const bool is_side_shelf = !shelf->IsHorizontalAlignment();
bubble_view_->StartHideAnimation(
Expand Down
30 changes: 0 additions & 30 deletions ash/app_list/app_list_bubble_presenter_unittest.cc
Expand Up @@ -41,7 +41,6 @@
#include "ui/gfx/geometry/vector2d.h"
#include "ui/views/controls/textfield/textfield.h"
#include "ui/views/widget/widget.h"
#include "ui/wm/core/window_util.h"

using views::Widget;

Expand Down Expand Up @@ -636,35 +635,6 @@ TEST_F(AppListBubblePresenterTest, CreatingChildWidgetDoesNotCloseBubble) {
EXPECT_TRUE(presenter->IsShowing());
}

TEST_F(AppListBubblePresenterTest, ClosingBubbleAlsoCloseChildWidget) {
AppListBubblePresenter* presenter = GetBubblePresenter();
presenter->Show(GetPrimaryDisplay().id());

// Create a new widget parented to the bubble, similar to an app uninstall
// confirmation dialog.
aura::Window* bubble_window =
presenter->bubble_widget_for_test()->GetNativeWindow();
std::unique_ptr<views::Widget> widget = TestWidgetBuilder()
.SetShow(true)
.SetParent(bubble_window)
.BuildOwnsNativeWidget();

// Open the bubble launcher and check the widget is showing.
EXPECT_TRUE(presenter->IsShowing());
EXPECT_TRUE(base::Contains(wm::GetTransientChildren(bubble_window),
widget->GetNativeWindow()));

// Close the bubble and reopen it.
presenter->Dismiss();
base::RunLoop().RunUntilIdle();

presenter->Show(GetPrimaryDisplay().id());
// The child widget is closed now.
EXPECT_TRUE(presenter->IsShowing());
EXPECT_FALSE(base::Contains(wm::GetTransientChildren(bubble_window),
widget->GetNativeWindow()));
}

// Regression test for https://crbug.com/1285443.
TEST_F(AppListBubblePresenterTest, CanOpenBubbleThenOpenSystemTray) {
// Enable animations.
Expand Down
49 changes: 0 additions & 49 deletions chrome/browser/ash/app_list/app_list_client_impl_browsertest.cc
Expand Up @@ -82,7 +82,6 @@
#include "ui/display/scoped_display_for_new_windows.h"
#include "ui/display/screen.h"
#include "ui/display/test/display_manager_test_api.h"
#include "ui/views/native_window_tracker.h"
#include "ui/wm/core/window_util.h"
#include "ui/wm/public/activation_change_observer.h"
#include "ui/wm/public/activation_client.h"
Expand Down Expand Up @@ -214,54 +213,6 @@ IN_PROC_BROWSER_TEST_F(AppListClientImplBrowserTest, UninstallApp) {
EXPECT_TRUE(client->GetAppListWindow());
}

// TODO(crbug.com/1444649): Test is flaky.
IN_PROC_BROWSER_TEST_F(AppListClientImplBrowserTest,
DISABLED_HidingBubbleLauncherCancelsAppUninstall) {
AppListClientImpl* client = AppListClientImpl::GetInstance();
const extensions::Extension* app = InstallPlatformApp("minimal");

// Show the app list and request app uninstall.
EXPECT_FALSE(client->GetAppListWindow());
client->ShowAppList(ash::AppListShowSource::kSearchKey);
ash::AppListTestApi().WaitForBubbleWindow(
/*wait_for_opening_animation=*/false);
// Open the uninstall dialog.
client->UninstallApp(profile(), app->id());
base::RunLoop().RunUntilIdle();

aura::Window* app_list_window = client->GetAppListWindow();
std::unique_ptr<views::NativeWindowTracker> app_list_tracker =
views::NativeWindowTracker::Create(app_list_window);
auto transient_children = wm::GetTransientChildren(app_list_window);
ASSERT_FALSE(transient_children.empty());

aura::Window* dialog_window = transient_children[0];
std::unique_ptr<views::NativeWindowTracker> dialog_tracker =
views::NativeWindowTracker::Create(dialog_window);

// Hide the app list, and verify that the dialog window was closed.
ASSERT_TRUE(client->app_list_visible());
client->DismissView();
base::RunLoop().RunUntilIdle();

EXPECT_FALSE(client->app_list_visible());

ASSERT_FALSE(app_list_tracker->WasNativeWindowDestroyed());
EXPECT_TRUE(wm::GetTransientChildren(app_list_window).empty());

EXPECT_TRUE(dialog_tracker->WasNativeWindowDestroyed());

// Reopen the app list, and verify that request to uninstall the app shows the
// uninstall dialog.
client->ShowAppList(ash::AppListShowSource::kSearchKey);
ash::AppListTestApi().WaitForBubbleWindow(
/*wait_for_opening_animation=*/false);
EXPECT_TRUE(client->GetAppListWindow());
client->UninstallApp(profile(), app->id());
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(wm::GetTransientChildren(client->GetAppListWindow()).empty());
}

IN_PROC_BROWSER_TEST_F(AppListClientImplBrowserTest, ShowAppInfo) {
ash::SystemWebAppManager::GetForTest(profile())
->InstallSystemAppsForTesting();
Expand Down

0 comments on commit 7d45bad

Please sign in to comment.