Make popped out extensions stay popped out if they have a context menu shown...

Make popped out extensions stay popped out if they have a context menu shown in the new extensions toolbar container.

Previously we would only check if a context menu was running when trying to hide a popped out extension. In the case where the context menu was not yet running when this happened then the popped out extension would be hidden when it shouldn't be. This change lets the ExtensionsToolbarContainer keep track of an extension with an open context menu and adjust visibility accordingly.

Bug: 943702
Change-Id: I547b16eb324852c9559238c646d189c39892fb55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2065009
Commit-Queue: Caroline Rising <corising@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744119}

chrome/browser/ui/extensions/extension_action_view_controller.cc

@@ -219,9 +219,19 @@ ui::MenuModel* ExtensionActionViewController::GetContextMenu() {
   return context_menu_model_.get();
 }
 
+void ExtensionActionViewController::OnContextMenuShown() {
+  extensions_container_->OnContextMenuShown(this);
+}
+
 void ExtensionActionViewController::OnContextMenuClosed() {
-  if (extensions_container_->GetPoppedOutAction() == this && !IsShowingPopup())
+  if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
+    extensions_container_->OnContextMenuClosed(this);
+    return;
+  }
+  if (extensions_container_->GetPoppedOutAction() == this &&
+      !view_delegate_->IsMenuRunning()) {
     extensions_container_->UndoPopOut();
+  }
 }
 
 bool ExtensionActionViewController::ExecuteAction(bool by_user) {
@@ -439,7 +449,8 @@ void ExtensionActionViewController::OnPopupClosed() {
   popup_host_ = nullptr;
   extensions_container_->SetPopupOwner(nullptr);
   if (extensions_container_->GetPoppedOutAction() == this &&
-      !view_delegate_->IsMenuRunning()) {
+      (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu) ||
+       !view_delegate_->IsMenuRunning())) {
     extensions_container_->UndoPopOut();
   }
   view_delegate_->OnPopupClosed();

chrome/browser/ui/extensions/extension_action_view_controller.h

@@ -68,6 +68,7 @@ class ExtensionActionViewController
   void HidePopup() override;
   gfx::NativeView GetPopupNativeView() override;
   ui::MenuModel* GetContextMenu() override;
+  void OnContextMenuShown() override;
   void OnContextMenuClosed() override;
   bool ExecuteAction(bool by_user) override;
   void UpdateState() override;

chrome/browser/ui/extensions/extensions_container.h

@@ -24,6 +24,14 @@ class ExtensionsContainer {
   // that relate to more than one extension.
   virtual ToolbarActionViewController* GetPoppedOutAction() const = 0;
 
+  // Called when a context menu is shown so the container can perform any
+  // necessary setup.
+  virtual void OnContextMenuShown(ToolbarActionViewController* extension) {}
+
+  // Called when a context menu is closed so the container can perform any
+  // necessary cleanup.
+  virtual void OnContextMenuClosed(ToolbarActionViewController* extension) {}
+
   // Returns true if the given |action| is visible on the toolbar.
   virtual bool IsActionVisibleOnToolbar(
       const ToolbarActionViewController* action) const = 0;

chrome/browser/ui/toolbar/toolbar_action_view_controller.h

@@ -88,6 +88,10 @@ class ToolbarActionViewController {
   // Returns the context menu model, or null if no context menu should be shown.
   virtual ui::MenuModel* GetContextMenu() = 0;
 
+  // Called when a context menu is shown so the controller can perform any
+  // necessary setup.
+  virtual void OnContextMenuShown() {}
+
   // Called when a context menu has closed so the controller can perform any
   // necessary cleanup.
   virtual void OnContextMenuClosed() {}

chrome/browser/ui/views/extensions/extension_context_menu_controller.cc

@@ -56,6 +56,7 @@ void ExtensionContextMenuController::ShowContextMenuForViewImpl(
 
   menu_runner_ = std::make_unique<views::MenuRunner>(menu_, run_types);
 
+  controller_->OnContextMenuShown();
   menu_runner_->RunMenuAt(
       parent,
       static_cast<views::MenuButtonController*>(

chrome/browser/ui/views/extensions/extensions_menu_view_browsertest.cc

@@ -215,6 +215,17 @@ class ExtensionsMenuViewBrowserTest : public ExtensionsToolbarBrowserTest {
     views::test::WaitForAnimatingLayoutManager(GetExtensionsToolbarContainer());
   }
 
+  void RightClickExtensionInToolbar(ToolbarActionView* extension) {
+    ui::MouseEvent click_down_event(ui::ET_MOUSE_PRESSED, gfx::Point(),
+                                    gfx::Point(), base::TimeTicks(),
+                                    ui::EF_RIGHT_MOUSE_BUTTON, 0);
+    ui::MouseEvent click_up_event(ui::ET_MOUSE_RELEASED, gfx::Point(),
+                                  gfx::Point(), base::TimeTicks(),
+                                  ui::EF_RIGHT_MOUSE_BUTTON, 0);
+    extension->OnMouseEvent(&click_down_event);
+    extension->OnMouseEvent(&click_up_event);
+  }
+
   void ClickExtensionsMenuButton(Browser* browser) {
     ui::MouseEvent click_event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                                base::TimeTicks(), ui::EF_LEFT_MOUSE_BUTTON, 0);
@@ -388,6 +399,46 @@ IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest, TriggerPopup) {
   EXPECT_TRUE(GetVisibleToolbarActionViews().empty());
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest,
+                       ContextMenuKeepsExtensionPoppedOut) {
+  LoadTestExtension("extensions/simple_with_popup");
+  ShowUi("");
+  VerifyUi();
+
+  ExtensionsToolbarContainer* const extensions_container =
+      GetExtensionsToolbarContainer();
+
+  EXPECT_EQ(nullptr, extensions_container->GetPoppedOutAction());
+  EXPECT_TRUE(GetVisibleToolbarActionViews().empty());
+
+  TriggerSingleExtensionButton();
+
+  // After triggering an extension with a popup, there should a popped-out
+  // action and show the view.
+  auto visible_icons = GetVisibleToolbarActionViews();
+  EXPECT_NE(nullptr, extensions_container->GetPoppedOutAction());
+  EXPECT_EQ(base::nullopt,
+            extensions_container->GetExtensionWithOpenContextMenuForTesting());
+  ASSERT_EQ(1u, visible_icons.size());
+  EXPECT_EQ(extensions_container->GetPoppedOutAction(),
+            visible_icons[0]->view_controller());
+
+  RightClickExtensionInToolbar(extensions_container->GetViewForId(
+      extensions_container->GetPoppedOutAction()->GetId()));
+  extensions_container->HideActivePopup();
+
+  // Wait for animations to finish.
+  views::test::WaitForAnimatingLayoutManager(extensions_container);
+
+  visible_icons = GetVisibleToolbarActionViews();
+  ASSERT_EQ(1u, visible_icons.size());
+  EXPECT_EQ(nullptr, extensions_container->GetPoppedOutAction());
+  EXPECT_NE(base::nullopt,
+            extensions_container->GetExtensionWithOpenContextMenuForTesting());
+  EXPECT_EQ(extensions_container->GetExtensionWithOpenContextMenuForTesting(),
+            visible_icons[0]->view_controller()->GetId());
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest,
                        RemoveExtensionShowingPopup) {
   LoadTestExtension("extensions/simple_with_popup");

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -137,6 +137,11 @@ bool ExtensionsToolbarContainer::ShouldForceVisibility(
   if (popped_out_action_ && popped_out_action_->GetId() == extension_id)
     return true;
 
+  if (extension_with_open_context_menu_id_.has_value() &&
+      extension_with_open_context_menu_id_.value() == extension_id) {
+    return true;
+  }
+
   for (const auto& anchored_widget : anchored_widgets_) {
     if (anchored_widget.extension_id == extension_id)
       return true;
@@ -227,6 +232,21 @@ ToolbarActionViewController* ExtensionsToolbarContainer::GetPoppedOutAction()
   return popped_out_action_;
 }
 
+void ExtensionsToolbarContainer::OnContextMenuShown(
+    ToolbarActionViewController* extension) {
+  extension_with_open_context_menu_id_ = extension->GetId();
+  UpdateIconVisibility(extension_with_open_context_menu_id_.value());
+}
+
+void ExtensionsToolbarContainer::OnContextMenuClosed(
+    ToolbarActionViewController* extension) {
+  DCHECK(extension_with_open_context_menu_id_.has_value());
+  base::Optional<extensions::ExtensionId> const
+      extension_with_open_context_menu = extension_with_open_context_menu_id_;
+  extension_with_open_context_menu_id_.reset();
+  UpdateIconVisibility(extension_with_open_context_menu.value());
+}
+
 bool ExtensionsToolbarContainer::IsActionVisibleOnToolbar(
     const ToolbarActionViewController* action) const {
   const std::string& extension_id = action->GetId();

chrome/browser/ui/views/extensions/extensions_toolbar_container.h

@@ -78,6 +78,11 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   views::Widget* GetAnchoredWidgetForExtensionForTesting(
       const std::string& extension_id);
 
+  base::Optional<extensions::ExtensionId>
+  GetExtensionWithOpenContextMenuForTesting() {
+    return extension_with_open_context_menu_id_;
+  }
+
   // ToolbarIconContainerView:
   void UpdateAllIcons() override;
   bool GetDropFormats(int* formats,
@@ -92,6 +97,8 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   ToolbarActionViewController* GetActionForId(
       const std::string& action_id) override;
   ToolbarActionViewController* GetPoppedOutAction() const override;
+  void OnContextMenuShown(ToolbarActionViewController* extension) override;
+  void OnContextMenuClosed(ToolbarActionViewController* extension) override;
   bool IsActionVisibleOnToolbar(
       const ToolbarActionViewController* action) const override;
   void UndoPopOut() override;
@@ -217,6 +224,8 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   ToolbarActionViewController* popped_out_action_ = nullptr;
   // The action that triggered the current popup, if any.
   ToolbarActionViewController* popup_owner_ = nullptr;
+  // Extension with an open context menu, if any.
+  base::Optional<extensions::ExtensionId> extension_with_open_context_menu_id_;
 
   // The widgets currently popped out and, for each, the extension it is
   // associated with. See AnchoredWidget.