Commit 9f787eef authored by Yuta Kitamura's avatar Yuta Kitamura Committed by Commit Bot

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

This reverts commit f2baca71.

Reason for revert: This CL is triggering a CHECK failure in
a few tests in ash_unittests.

Failed tests:
ShelfApplicationMenuModelTest.VerifyContentsWithMenuItems
ShelfApplicationMenuModelTest.VerifyContentsWithNoMenuItems
ShelfApplicationMenuModelTest.VerifyHistogramBuckets
ShelfApplicationMenuModelTest.VerifyHistogramOnExecute
ShelfViewInkDropTest.ShelfButtonWithMenuPressRelease

Started to fail since:
https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg/14866
(Just look at ash_unittests, other failures are irrelevant)

Crash log sample:

[ RUN      ] ShelfViewInkDropTest.ShelfButtonWithMenuPressRelease
[15893:15893:1017/174420.948496:6297578335:FATAL:simple_menu_model.cc(561)] Check failed: item.command_id >= 0 (-2 vs. 0)

Original change's description:
> 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/+/1841583
> Reviewed-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}

TBR=tbarzic@chromium.org,sky@chromium.org,msw@chromium.org,newcomer@chromium.org,yulunwu@chromium.org

Change-Id: I39a3bebb393d70e1dbeb6da61908f7ec2e3e9b2e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1006203
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869050Reviewed-by: default avatarYuta Kitamura <yutak@chromium.org>
Commit-Queue: Yuta Kitamura <yutak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707287}
parent 0c392cd8
......@@ -20,7 +20,7 @@ ShelfApplicationMenuModel::ShelfApplicationMenuModel(
Items items,
ShelfItemDelegate* delegate)
: ui::SimpleMenuModel(this), delegate_(delegate) {
AddTitle(title);
AddItem(std::numeric_limits<int>::max(), title);
for (size_t i = 0; i < items.size(); i++)
AddItemWithIcon(i, items[i].first, items[i].second);
AddSeparator(ui::SPACING_SEPARATOR);
......
......@@ -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_TITLE, menu.GetTypeAt(0));
EXPECT_EQ(ui::MenuModel::TYPE_COMMAND, 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_TITLE, menu.GetTypeAt(0));
EXPECT_EQ(ui::MenuModel::TYPE_COMMAND, menu.GetTypeAt(0));
EXPECT_EQ(title, menu.GetLabelAt(0));
EXPECT_FALSE(menu.IsEnabledAt(0));
......
......@@ -67,7 +67,6 @@ 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:
......
......@@ -149,9 +149,6 @@ 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;
......
......@@ -39,8 +39,6 @@ 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();
......
......@@ -17,10 +17,6 @@ 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:
......@@ -146,10 +142,6 @@ 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) {
......@@ -468,9 +460,6 @@ 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;
......@@ -480,8 +469,7 @@ bool SimpleMenuModel::IsEnabledAt(int index) const {
bool SimpleMenuModel::IsVisibleAt(int index) const {
int command_id = GetCommandIdAt(index);
if (!delegate_ || command_id == kSeparatorId || command_id == kTitleId ||
GetButtonMenuItemAt(index))
if (!delegate_ || command_id == kSeparatorId || GetButtonMenuItemAt(index))
return items_[ValidateItemIndex(index)].visible;
return delegate_->IsCommandIdVisible(command_id) &&
......
......@@ -107,7 +107,6 @@ 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
......
......@@ -200,7 +200,6 @@ 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:
......@@ -1140,9 +1139,7 @@ SkColor MenuItemView::GetTextColor(bool minor, bool render_selection) const {
: style::CONTEXT_MENU;
style::TextStyle text_style = style::STYLE_PRIMARY;
if (type_ == Type::TITLE)
text_style = style::STYLE_PRIMARY;
else if (type_ == HIGHLIGHTED)
if (type_ == HIGHLIGHTED)
text_style = style::STYLE_HIGHLIGHTED;
else if (!GetEnabled())
text_style = style::STYLE_DISABLED;
......
......@@ -95,9 +95,8 @@ 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.
TITLE, // Title text, does not perform any action.
EMPTY, // EMPTY is a special type for empty menus that is only
// used internally.
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
......
......@@ -70,9 +70,6 @@ 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;
......
......@@ -213,9 +213,6 @@ 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;
......@@ -290,9 +287,6 @@ 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;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment