Menu Model: fix bug where menu item title was greyed out because it is disabled.

The old approach of adding title as a disabled item meant that this CL:

https://chromium-review.googlesource.com/c/chromium/src/+/1788198

causes the menu title to be displayed in grey text instead of solid black text. This CL introduces a new menu type that, while still disabled and non-reactive, be displayed in solid black text.

Bug: 1006203
Change-Id: I9fa38132160228ebde5bd43d8db9c2f60d5f001c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1841583Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: Yulun Wu <yulunwu@chromium.org>
Auto-Submit: Yulun Wu <yulunwu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707147}

ash/shelf/shelf_application_menu_model.cc

@@ -20,7 +20,7 @@ ShelfApplicationMenuModel::ShelfApplicationMenuModel(
     Items items,
     ShelfItemDelegate* delegate)
     : ui::SimpleMenuModel(this), delegate_(delegate) {
-  AddItem(std::numeric_limits<int>::max(), title);
+  AddTitle(title);
   for (size_t i = 0; i < items.size(); i++)
     AddItemWithIcon(i, items[i].first, items[i].second);
   AddSeparator(ui::SPACING_SEPARATOR);

ash/shelf/shelf_application_menu_model_unittest.cc

@@ -51,7 +51,7 @@ TEST(ShelfApplicationMenuModelTest, VerifyContentsWithNoMenuItems) {
   ShelfApplicationMenuModel menu(title, {}, nullptr);
   // Expect the title and a separator.
   ASSERT_EQ(2, menu.GetItemCount());
-  EXPECT_EQ(ui::MenuModel::TYPE_COMMAND, menu.GetTypeAt(0));
+  EXPECT_EQ(ui::MenuModel::TYPE_TITLE, menu.GetTypeAt(0));
   EXPECT_EQ(title, menu.GetLabelAt(0));
   EXPECT_FALSE(menu.IsEnabledAt(0));
   EXPECT_EQ(ui::MenuModel::TYPE_SEPARATOR, menu.GetTypeAt(1));
@@ -75,7 +75,7 @@ TEST(ShelfApplicationMenuModelTest, VerifyContentsWithMenuItems) {
   ASSERT_EQ(static_cast<int>(5), menu.GetItemCount());
 
   // The label title should not be enabled.
-  EXPECT_EQ(ui::MenuModel::TYPE_COMMAND, menu.GetTypeAt(0));
+  EXPECT_EQ(ui::MenuModel::TYPE_TITLE, menu.GetTypeAt(0));
   EXPECT_EQ(title, menu.GetLabelAt(0));
   EXPECT_FALSE(menu.IsEnabledAt(0));
 

components/dbus/menu/menu_property_list.cc

@@ -67,6 +67,7 @@ MenuItemProperties ComputeMenuPropertiesForMenuItem(ui::MenuModel* menu,
   switch (menu->GetTypeAt(i)) {
     case ui::MenuModel::TYPE_COMMAND:
     case ui::MenuModel::TYPE_HIGHLIGHTED:
+    case ui::MenuModel::TYPE_TITLE:
       // Nothing special to do.
       break;
     case ui::MenuModel::TYPE_CHECK:

components/dbus/menu/menu_property_list_unittest.cc

@@ -149,6 +149,9 @@ class TestMenuModelBuilder {
       case ui::MenuModel::TYPE_COMMAND:
         menu->AddItem(0, label_);
         break;
+      case ui::MenuModel::TYPE_TITLE:
+        menu->AddTitle(label_);
+        break;
       case ui::MenuModel::TYPE_CHECK:
         menu->AddCheckItem(0, label_);
         break;

ui/base/models/menu_model.h

@@ -39,6 +39,8 @@ class UI_BASE_EXPORT MenuModel : public base::SupportsWeakPtr<MenuModel> {
     TYPE_HIGHLIGHTED,  // Performs an action when selected, and has a different
                        // colored background. When placed at the bottom, the
                        // background matches the menu's rounded corners.
+    TYPE_TITLE,        // Plain text that does not perform any action when
+                       // selected.
   };
 
   MenuModel();

ui/base/models/simple_menu_model.cc

@@ -17,6 +17,10 @@ namespace ui {
 
 const int kSeparatorId = -1;
 
+// Text items are rendered as enabled but are non-interactive with no actions
+// and cannot be highlighted.
+const int kTitleId = -2;
+
 ////////////////////////////////////////////////////////////////////////////////
 // SimpleMenuModel::Delegate, public:
 
@@ -142,6 +146,10 @@ void SimpleMenuModel::AddHighlightedItemWithIcon(int command_id,
   AppendItem(std::move(item));
 }
 
+void SimpleMenuModel::AddTitle(const base::string16& label) {
+  AppendItem(Item(kTitleId, TYPE_TITLE, label));
+}
+
 void SimpleMenuModel::AddSeparator(MenuSeparatorType separator_type) {
   if (items_.empty()) {
     if (separator_type == NORMAL_SEPARATOR) {
@@ -460,6 +468,9 @@ ButtonMenuItemModel* SimpleMenuModel::GetButtonMenuItemAt(int index) const {
 
 bool SimpleMenuModel::IsEnabledAt(int index) const {
   int command_id = GetCommandIdAt(index);
+  if (command_id == kTitleId)
+    return false;
+
   if (!delegate_ || command_id == kSeparatorId || GetButtonMenuItemAt(index))
     return items_[ValidateItemIndex(index)].enabled;
 
@@ -469,7 +480,8 @@ bool SimpleMenuModel::IsEnabledAt(int index) const {
 
 bool SimpleMenuModel::IsVisibleAt(int index) const {
   int command_id = GetCommandIdAt(index);
-  if (!delegate_ || command_id == kSeparatorId || GetButtonMenuItemAt(index))
+  if (!delegate_ || command_id == kSeparatorId || command_id == kTitleId ||
+      GetButtonMenuItemAt(index))
     return items_[ValidateItemIndex(index)].visible;
 
   return delegate_->IsCommandIdVisible(command_id) &&

ui/base/models/simple_menu_model.h

@@ -107,6 +107,7 @@ class UI_BASE_EXPORT SimpleMenuModel : public MenuModel {
   void AddHighlightedItemWithIcon(int command_id,
                                   const base::string16& label,
                                   const gfx::ImageSkia& icon);
+  void AddTitle(const base::string16& label);
 
   // Adds a separator of the specified type to the model.
   // - Adding a separator after another separator is always invalid if they

ui/views/controls/menu/menu_item_view.cc

@@ -200,6 +200,7 @@ void MenuItemView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
       node_data->SetCheckedState(is_checked ? ax::mojom::CheckedState::kTrue
                                             : ax::mojom::CheckedState::kFalse);
     } break;
+    case TITLE:
     case NORMAL:
     case SEPARATOR:
     case EMPTY:
@@ -1139,7 +1140,9 @@ SkColor MenuItemView::GetTextColor(bool minor, bool render_selection) const {
           : style::CONTEXT_MENU;
 
   style::TextStyle text_style = style::STYLE_PRIMARY;
-  if (type_ == HIGHLIGHTED)
+  if (type_ == Type::TITLE)
+    text_style = style::STYLE_PRIMARY;
+  else if (type_ == HIGHLIGHTED)
     text_style = style::STYLE_HIGHLIGHTED;
   else if (!GetEnabled())
     text_style = style::STYLE_DISABLED;

ui/views/controls/menu/menu_item_view.h

@@ -95,8 +95,9 @@ class VIEWS_EXPORT MenuItemView : public View {
     HIGHLIGHTED,         // Performs an action when selected, and has a
                          // different colored background that merges with the
                          // menu's rounded corners when placed at the bottom.
-    EMPTY,  // EMPTY is a special type for empty menus that is only used
-            // internally.
+    TITLE,               // Title text, does not perform any action.
+    EMPTY,               // EMPTY is a special type for empty menus that is only
+                         // used internally.
   };
 
   // Where the menu should be drawn, above or below the bounds (when

ui/views/controls/menu/menu_model_adapter.cc

@@ -70,6 +70,9 @@ MenuItemView* MenuModelAdapter::AddMenuItemFromModelAt(ui::MenuModel* model,
   base::Optional<MenuItemView::Type> type;
   ui::MenuModel::ItemType menu_type = model->GetTypeAt(model_index);
   switch (menu_type) {
+    case ui::MenuModel::TYPE_TITLE:
+      type = MenuItemView::TITLE;
+      break;
     case ui::MenuModel::TYPE_COMMAND:
     case ui::MenuModel::TYPE_BUTTON_ITEM:
       type = MenuItemView::NORMAL;

ui/views/controls/menu/menu_model_adapter_unittest.cc

@@ -213,6 +213,9 @@ void CheckSubmenu(const RootModel& model,
 
     // Check type.
     switch (model_item.type) {
+      case ui::MenuModel::TYPE_TITLE:
+        EXPECT_EQ(views::MenuItemView::TITLE, item->GetType());
+        break;
       case ui::MenuModel::TYPE_COMMAND:
         EXPECT_EQ(views::MenuItemView::NORMAL, item->GetType());
         break;
@@ -287,6 +290,9 @@ TEST_F(MenuModelAdapterTest, BasicTest) {
 
     // Check type.
     switch (model_item.type) {
+      case ui::MenuModel::TYPE_TITLE:
+        EXPECT_EQ(views::MenuItemView::TITLE, item->GetType());
+        break;
       case ui::MenuModel::TYPE_COMMAND:
         EXPECT_EQ(views::MenuItemView::NORMAL, item->GetType());
         break;