Highlight submenu items when the submenu contains a highlighted item.

If an item/command in a submenu is highlighted as part of a feature
promo, this CL ensures that the top-level item that opens the submenu is
also highlighted.

It also ensures that if the primary ("move to new ____") item of an
"move to existing or new ____" tab submenu is highlighted if the same
item would be highlighted in the top-level menu.

This affects highlighting for the "create tab group" promo/tutorial.

Change-Id: I312975f961c988bf565fab1abf6cf1e53d993aa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3498654
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/main@{#985022}

chrome/browser/ui/tabs/existing_base_sub_menu_model.cc

@@ -11,12 +11,14 @@ ExistingBaseSubMenuModel::ExistingBaseSubMenuModel(
     ui::SimpleMenuModel::Delegate* parent_delegate,
     TabStripModel* model,
     int context_index,
-    int min_command_id)
+    int min_command_id,
+    int parent_new_command_id)
     : SimpleMenuModel(this),
       parent_delegate_(parent_delegate),
       model_(model),
       context_contents_(model->GetWebContentsAt(context_index)),
-      min_command_id_(min_command_id) {}
+      min_command_id_(min_command_id),
+      parent_new_command_id_(parent_new_command_id) {}
 
 bool ExistingBaseSubMenuModel::GetAcceleratorForCommandId(
     int command_id,
@@ -33,6 +35,11 @@ const gfx::FontList* ExistingBaseSubMenuModel::GetLabelFontListAt(
   return nullptr;
 }
 
+bool ExistingBaseSubMenuModel::IsCommandIdAlerted(int command_id) const {
+  return IsNewCommand(command_id) &&
+         parent_delegate()->IsCommandIdAlerted(parent_new_command_id_);
+}
+
 bool ExistingBaseSubMenuModel::IsCommandIdChecked(int command_id) const {
   return false;
 }
@@ -46,7 +53,7 @@ constexpr int ExistingBaseSubMenuModel::kMinExistingTabGroupCommandId;
 
 void ExistingBaseSubMenuModel::ExecuteCommand(int command_id, int event_flags) {
   if (IsNewCommand(command_id)) {
-    ExecuteNewCommand(event_flags);
+    parent_delegate()->ExecuteCommand(parent_new_command_id_, event_flags);
     return;
   }
   ExecuteExistingCommand(command_id_to_target_index_[command_id]);
@@ -110,7 +117,9 @@ void ExistingBaseSubMenuModel::ClearMenu() {
   command_id_to_target_index_.clear();
 }
 
-void ExistingBaseSubMenuModel::ExecuteNewCommand(int event_flags) {}
+bool ExistingBaseSubMenuModel::IsNewCommand(int command_id) const {
+  return command_id == min_command_id_;
+}
 
 void ExistingBaseSubMenuModel::ExecuteExistingCommand(int target_index) {}
 

chrome/browser/ui/tabs/existing_base_sub_menu_model.h

@@ -32,14 +32,16 @@ class ExistingBaseSubMenuModel : public ui::SimpleMenuModel,
   ExistingBaseSubMenuModel(ui::SimpleMenuModel::Delegate* parent_delegate,
                            TabStripModel* model,
                            int context_index,
-                           int min_command_id);
+                           int min_command_id,
+                           int parent_new_command_id_);
 
   // ui::SimpleMenuModel
   bool GetAcceleratorForCommandId(int command_id,
                                   ui::Accelerator* accelerator) const override;
   const gfx::FontList* GetLabelFontListAt(int index) const override;
 
   // ui::SimpleMenuModel::Delegate
+  bool IsCommandIdAlerted(int command_id) const override;
   bool IsCommandIdChecked(int command_id) const override;
   bool IsCommandIdEnabled(int command_id) const override;
   void ExecuteCommand(int command_id, int event_flags) final;
@@ -94,13 +96,7 @@ class ExistingBaseSubMenuModel : public ui::SimpleMenuModel,
 
   // Helper method for checking if the command is to add a tab to a new object
   // model.
-  bool IsNewCommand(int command_id) const {
-    return command_id == min_command_id_;
-  }
-
-  // Performs the action for adding the tab to a new object model (e.g. group or
-  // window).
-  virtual void ExecuteNewCommand(int event_flags);
+  bool IsNewCommand(int command_id) const;
 
   // Performs the action for adding the tab to an existing object model (e.g.
   // group or window) at |target_index|.
@@ -120,6 +116,7 @@ class ExistingBaseSubMenuModel : public ui::SimpleMenuModel,
   const raw_ptr<TabStripModel> model_;
   const raw_ptr<const content::WebContents> context_contents_;
   const int min_command_id_;
+  const int parent_new_command_id_;
 
   // Stores a mapping from a menu item's command id to its target index (e.g.
   // tab-group index or browser index).

chrome/browser/ui/tabs/existing_base_sub_menu_model_unittest.cc

@@ -0,0 +1,131 @@
+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/tabs/existing_base_sub_menu_model.h"
+
+#include <memory>
+#include <vector>
+
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/grit/generated_resources.h"
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "ui/base/models/simple_menu_model.h"
+#include "url/gurl.h"
+
+namespace {
+
+constexpr int kMinCommandId = 1000;
+constexpr int kParentNewCommandId = 999;
+constexpr int kExpectedFlags = 0xABCD;
+constexpr char16_t kItem1Text[] = u"Item1";
+constexpr char16_t kTitleText[] = u"Title";
+constexpr char16_t kItem2Text[] = u"Item2";
+
+class TestDelegate : public ui::SimpleMenuModel::Delegate {
+ public:
+  ~TestDelegate() override = default;
+
+  bool new_command_alerted() const { return new_command_alerted_; }
+  void set_new_command_alerted(bool alerted) { new_command_alerted_ = alerted; }
+  int execute_count() { return execute_count_; }
+
+  bool IsCommandIdAlerted(int command_id) const override {
+    EXPECT_EQ(kParentNewCommandId, command_id);
+    return new_command_alerted();
+  }
+
+  void ExecuteCommand(int command_id, int event_flags) override {
+    EXPECT_EQ(kParentNewCommandId, command_id);
+    EXPECT_EQ(kExpectedFlags, event_flags);
+    ++execute_count_;
+  }
+
+ private:
+  bool new_command_alerted_ = false;
+  int execute_count_ = 0;
+};
+
+class TestModel : public ExistingBaseSubMenuModel {
+ public:
+  TestModel(ui::SimpleMenuModel::Delegate* test_delegate, TabStripModel* model)
+      : ExistingBaseSubMenuModel(test_delegate,
+                                 model,
+                                 0,
+                                 kMinCommandId,
+                                 kParentNewCommandId) {
+    MenuItemInfo info1(kItem1Text);
+    info1.target_index = 1;
+    infos_.push_back(info1);
+    MenuItemInfo title(kTitleText);
+    infos_.push_back(title);
+    MenuItemInfo info2(kItem2Text);
+    info2.target_index = 2;
+    infos_.push_back(info2);
+    Build(IDS_TAB_CXMENU_SUBMENU_NEW_GROUP, infos_);
+  }
+
+  const std::vector<int> existing_commands() const {
+    return existing_commands_;
+  }
+
+ protected:
+  void ExecuteExistingCommand(int target_index) override {
+    existing_commands_.push_back(target_index);
+  }
+
+ private:
+  std::vector<int> existing_commands_;
+  std::vector<MenuItemInfo> infos_;
+};
+
+}  // namespace
+
+class ExistingBaseSubMenuModelTest : public BrowserWithTestWindowTest {
+ public:
+  ExistingBaseSubMenuModelTest() = default;
+  ~ExistingBaseSubMenuModelTest() override = default;
+
+  void SetUp() override {
+    BrowserWithTestWindowTest::SetUp();
+    AddTab(browser(), GURL("chrome://newtab"));
+    test_delegate_ = std::make_unique<TestDelegate>();
+    test_model_ = std::make_unique<TestModel>(test_delegate_.get(),
+                                              browser()->tab_strip_model());
+  }
+
+  TestDelegate* test_delegate() const { return test_delegate_.get(); }
+  TestModel* test_model() const { return test_model_.get(); }
+
+ private:
+  std::unique_ptr<TestDelegate> test_delegate_;
+  std::unique_ptr<TestModel> test_model_;
+};
+
+// TODO(dfried): Verify that label font list is overridden for title elements.
+// Note that we can't test this because UI style info isn't loaded for unit
+// tests so it still returns null.
+
+TEST_F(ExistingBaseSubMenuModelTest, IsCommandIdAlerted) {
+  EXPECT_FALSE(test_model()->IsCommandIdAlerted(kMinCommandId));
+  EXPECT_FALSE(test_model()->IsCommandIdAlerted(kMinCommandId + 1));
+  EXPECT_FALSE(test_model()->IsCommandIdAlerted(kMinCommandId + 2));
+
+  test_delegate()->set_new_command_alerted(true);
+  EXPECT_TRUE(test_model()->IsCommandIdAlerted(kMinCommandId));
+  EXPECT_FALSE(test_model()->IsCommandIdAlerted(kMinCommandId + 1));
+  EXPECT_FALSE(test_model()->IsCommandIdAlerted(kMinCommandId + 2));
+}
+
+TEST_F(ExistingBaseSubMenuModelTest, ExecuteCommand_New) {
+  EXPECT_EQ(0, test_delegate()->execute_count());
+  test_model()->ExecuteCommand(kMinCommandId, kExpectedFlags);
+  EXPECT_EQ(1, test_delegate()->execute_count());
+}
+
+TEST_F(ExistingBaseSubMenuModelTest, ExecuteCommand_Existing) {
+  test_model()->ExecuteCommand(kMinCommandId + 1, kExpectedFlags);
+  test_model()->ExecuteCommand(kMinCommandId + 2, kExpectedFlags);
+  EXPECT_THAT(test_model()->existing_commands(), testing::ElementsAre(1, 2));
+}

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.cc

@@ -32,7 +32,8 @@ ExistingTabGroupSubMenuModel::ExistingTabGroupSubMenuModel(
     : ExistingBaseSubMenuModel(parent_delegate,
                                model,
                                context_index,
-                               kMinExistingTabGroupCommandId) {
+                               kMinExistingTabGroupCommandId,
+                               TabStripModel::CommandAddToNewGroup) {
   DCHECK(model->SupportsTabGroups());
   const ui::ColorProvider& color_provider =
       model->GetWebContentsAt(context_index)->GetColorProvider();
@@ -93,11 +94,6 @@ bool ExistingTabGroupSubMenuModel::ShouldShowSubmenu(TabStripModel* model,
   return false;
 }
 
-void ExistingTabGroupSubMenuModel::ExecuteNewCommand(int event_flags) {
-  parent_delegate()->ExecuteCommand(TabStripModel::CommandAddToNewGroup,
-                                    event_flags);
-}
-
 void ExistingTabGroupSubMenuModel::ExecuteExistingCommand(int target_index) {
   TabGroupModel* group_model = model()->group_model();
   if (!group_model)

chrome/browser/ui/tabs/existing_tab_group_sub_menu_model.h

@@ -32,7 +32,6 @@ class ExistingTabGroupSubMenuModel : public ExistingBaseSubMenuModel {
 
  private:
   // ExistingBaseSubMenuModel
-  void ExecuteNewCommand(int event_flags) override;
   void ExecuteExistingCommand(int target_index) override;
 
   // Returns the group ids that appear in the submenu in the order that they

chrome/browser/ui/tabs/existing_window_sub_menu_model.cc

@@ -50,7 +50,8 @@ ExistingWindowSubMenuModel::ExistingWindowSubMenuModel(
     : ExistingBaseSubMenuModel(parent_delegate,
                                model,
                                context_index,
-                               kMinExistingWindowCommandId) {
+                               kMinExistingWindowCommandId,
+                               TabStripModel::CommandMoveTabsToNewWindow) {
   Build(IDS_TAB_CXMENU_MOVETOANOTHERNEWWINDOW,
         BuildMenuItemInfoVectorForBrowsers(
             tab_menu_model_delegate->GetExistingWindowsForMoveMenu()));
@@ -111,11 +112,6 @@ ExistingWindowSubMenuModel::BuildMenuItemInfoVectorForBrowsers(
   return menu_item_infos;
 }
 
-void ExistingWindowSubMenuModel::ExecuteNewCommand(int event_flags) {
-  parent_delegate()->ExecuteCommand(TabStripModel::CommandMoveTabsToNewWindow,
-                                    event_flags);
-}
-
 void ExistingWindowSubMenuModel::ExecuteExistingCommand(int target_index) {
   model()->ExecuteAddToExistingWindowCommand(GetContextIndex(), target_index);
 }

chrome/browser/ui/tabs/existing_window_sub_menu_model.h

@@ -66,7 +66,6 @@ class ExistingWindowSubMenuModel : public ExistingBaseSubMenuModel {
 
  private:
   // ExistingBaseSubMenuModel:
-  void ExecuteNewCommand(int event_flags) override;
   void ExecuteExistingCommand(int target_index) override;
 };
 

chrome/test/BUILD.gn

@@ -6468,6 +6468,7 @@ test("unit_tests") {
       "../browser/ui/tab_contents/chrome_web_contents_view_handle_drop_unittest.cc",
       "../browser/ui/tab_contents/tab_contents_iterator_unittest.cc",
       "../browser/ui/tab_sharing/tab_sharing_infobar_delegate_unittest.cc",
+      "../browser/ui/tabs/existing_base_sub_menu_model_unittest.cc",
       "../browser/ui/tabs/existing_tab_group_sub_menu_model_unittest.cc",
       "../browser/ui/tabs/existing_window_sub_menu_model_unittest.cc",
       "../browser/ui/tabs/pinned_tab_codec_unittest.cc",

ui/base/models/simple_menu_model.cc

@@ -469,6 +469,15 @@ bool SimpleMenuModel::IsAlertedAt(int index) const {
   if (!delegate_ || command_id == kSeparatorId || command_id == kTitleId)
     return false;
 
+  // This method needs to be recursive, because if the highlighted command is
+  // in a submenu then the submenu item should also be highlighted.
+  if (auto* const submenu = GetSubmenuModelAt(index)) {
+    for (int i = 0; i < submenu->GetItemCount(); ++i) {
+      if (submenu->IsAlertedAt(i))
+        return true;
+    }
+  }
+
   return delegate_->IsCommandIdAlerted(command_id);
 }
 

ui/base/models/simple_menu_model_unittest.cc

@@ -223,6 +223,23 @@ TEST(SimpleMenuModelTest, HasIconsViaVectorIcon) {
   EXPECT_TRUE(simple_menu_model.HasIcons());
 }
 
+TEST(SimpleMenuModelTest, InheritsSubMenuAlert) {
+  DelegateBase delegate;
+  SimpleMenuModel submenu_model(&delegate);
+  submenu_model.AddItem(kAlertedCommandId + 1, u"menu item");
+
+  // The alerted menu item is not present in the submenu.
+  SimpleMenuModel parent_menu_model(&delegate);
+  parent_menu_model.AddSubMenu(/*command_id*/ 10, u"submenu", &submenu_model);
+  EXPECT_FALSE(parent_menu_model.IsAlertedAt(0));
+
+  // Add the alerted menu item to the submenu. Now both the submenu item and
+  // the item in the submenu should show as alerted.
+  submenu_model.AddItem(kAlertedCommandId, u"alerted item");
+  EXPECT_TRUE(submenu_model.IsAlertedAt(1));
+  EXPECT_TRUE(parent_menu_model.IsAlertedAt(0));
+}
+
 }  // namespace
 
 }  // namespace ui