Commit 55d6fd8c authored by Mohamed Amir Yosef's avatar Mohamed Amir Yosef Committed by Commit Bot

[UI] Support secondary labels in dropdown menus

Secondary labels are displayed below the labels of each menu item.

This and the CLs in the same chain are introduced to support the
use case in this mock:
https://screenshot.googleplex.com/si2PPQSzV1w


Bug: 1044038
Change-Id: I17535582c700ee83d95e077a15066542de13d925
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2208976
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770798}
parent 130ed90c
......@@ -14,14 +14,13 @@ using base::ASCIIToUTF16;
// Simple test for clicking a menu item. This template class clicks on an
// item and checks that the returned id matches. The index of the item
// is the template argument.
template<int INDEX>
template <int INDEX>
class MenuItemViewTestBasic : public MenuTestBase {
public:
MenuItemViewTestBasic() {
}
~MenuItemViewTestBasic() override {
}
MenuItemViewTestBasic() = default;
MenuItemViewTestBasic(const MenuItemViewTestBasic&) = delete;
MenuItemViewTestBasic& operator=(const MenuItemViewTestBasic&) = delete;
~MenuItemViewTestBasic() override = default;
// MenuTestBase implementation
void BuildMenu(views::MenuItemView* menu) override {
......@@ -50,15 +49,12 @@ class MenuItemViewTestBasic : public MenuTestBase {
ASSERT_EQ(INDEX + 1, last_command());
Done();
}
private:
DISALLOW_COPY_AND_ASSIGN(MenuItemViewTestBasic);
};
// Click each item of a 3-item menu (with separator).
typedef MenuItemViewTestBasic<0> MenuItemViewTestBasic0;
typedef MenuItemViewTestBasic<1> MenuItemViewTestBasic1;
typedef MenuItemViewTestBasic<2> MenuItemViewTestBasic2;
using MenuItemViewTestBasic0 = MenuItemViewTestBasic<0>;
using MenuItemViewTestBasic1 = MenuItemViewTestBasic<1>;
using MenuItemViewTestBasic2 = MenuItemViewTestBasic<2>;
// If this flakes, disable and log details in http://crbug.com/523255.
VIEW_TEST(MenuItemViewTestBasic0, SelectItem0)
VIEW_TEST(MenuItemViewTestBasic1, SelectItem1)
......@@ -67,14 +63,13 @@ VIEW_TEST(MenuItemViewTestBasic1, SelectItem1)
VIEW_TEST(MenuItemViewTestBasic2, SelectItem2)
// Test class for inserting a menu item while the menu is open.
template<int INSERT_INDEX, int SELECT_INDEX>
template <int INSERT_INDEX, int SELECT_INDEX>
class MenuItemViewTestInsert : public MenuTestBase {
public:
MenuItemViewTestInsert() : inserted_item_(NULL) {
}
~MenuItemViewTestInsert() override {
}
MenuItemViewTestInsert() = default;
MenuItemViewTestInsert(const MenuItemViewTestInsert&) = delete;
MenuItemViewTestInsert& operator=(const MenuItemViewTestInsert&) = delete;
~MenuItemViewTestInsert() override = default;
// MenuTestBase implementation
void BuildMenu(views::MenuItemView* menu) override {
......@@ -92,8 +87,9 @@ class MenuItemViewTestInsert : public MenuTestBase {
inserted_item_ = menu()->AddMenuItemAt(
INSERT_INDEX, 1000, ASCIIToUTF16("inserted item"), base::string16(),
ui::ThemedVectorIcon(), gfx::ImageSkia(), ui::ThemedVectorIcon(),
views::MenuItemView::Type::kNormal, ui::NORMAL_SEPARATOR);
base::string16(), ui::ThemedVectorIcon(), gfx::ImageSkia(),
ui::ThemedVectorIcon(), views::MenuItemView::Type::kNormal,
ui::NORMAL_SEPARATOR);
ASSERT_TRUE(inserted_item_);
menu()->ChildrenChanged();
......@@ -123,21 +119,19 @@ class MenuItemViewTestInsert : public MenuTestBase {
}
private:
views::MenuItemView* inserted_item_;
DISALLOW_COPY_AND_ASSIGN(MenuItemViewTestInsert);
views::MenuItemView* inserted_item_ = nullptr;
};
// MenuItemViewTestInsertXY inserts an item at index X and selects the
// item at index Y (after the insertion). The tests here cover
// inserting at the beginning, middle, and end, crossbarred with
// selecting the first and last item.
typedef MenuItemViewTestInsert<0,0> MenuItemViewTestInsert00;
typedef MenuItemViewTestInsert<0,2> MenuItemViewTestInsert02;
typedef MenuItemViewTestInsert<1,0> MenuItemViewTestInsert10;
typedef MenuItemViewTestInsert<1,2> MenuItemViewTestInsert12;
typedef MenuItemViewTestInsert<2,0> MenuItemViewTestInsert20;
typedef MenuItemViewTestInsert<2,2> MenuItemViewTestInsert22;
using MenuItemViewTestInsert00 = MenuItemViewTestInsert<0, 0>;
using MenuItemViewTestInsert02 = MenuItemViewTestInsert<0, 2>;
using MenuItemViewTestInsert10 = MenuItemViewTestInsert<1, 0>;
using MenuItemViewTestInsert12 = MenuItemViewTestInsert<1, 2>;
using MenuItemViewTestInsert20 = MenuItemViewTestInsert<2, 0>;
using MenuItemViewTestInsert22 = MenuItemViewTestInsert<2, 2>;
// If this flakes, disable and log details in http://crbug.com/523255.
VIEW_TEST(MenuItemViewTestInsert00, InsertItem00)
......@@ -158,16 +152,15 @@ VIEW_TEST(MenuItemViewTestInsert20, InsertItem20)
VIEW_TEST(MenuItemViewTestInsert22, InsertItem22)
// Test class for inserting a menu item while a submenu is open.
template<int INSERT_INDEX>
template <int INSERT_INDEX>
class MenuItemViewTestInsertWithSubmenu : public MenuTestBase {
public:
MenuItemViewTestInsertWithSubmenu() :
submenu_(NULL),
inserted_item_(NULL) {
}
~MenuItemViewTestInsertWithSubmenu() override {
}
MenuItemViewTestInsertWithSubmenu() = default;
MenuItemViewTestInsertWithSubmenu(const MenuItemViewTestInsertWithSubmenu&) =
delete;
MenuItemViewTestInsertWithSubmenu& operator=(
const MenuItemViewTestInsertWithSubmenu&) = delete;
~MenuItemViewTestInsertWithSubmenu() override = default;
// MenuTestBase implementation
void BuildMenu(views::MenuItemView* menu) override {
......@@ -187,8 +180,9 @@ class MenuItemViewTestInsertWithSubmenu : public MenuTestBase {
void Step2() {
inserted_item_ = menu()->AddMenuItemAt(
INSERT_INDEX, 1000, ASCIIToUTF16("inserted item"), base::string16(),
ui::ThemedVectorIcon(), gfx::ImageSkia(), ui::ThemedVectorIcon(),
views::MenuItemView::Type::kNormal, ui::NORMAL_SEPARATOR);
base::string16(), ui::ThemedVectorIcon(), gfx::ImageSkia(),
ui::ThemedVectorIcon(), views::MenuItemView::Type::kNormal,
ui::NORMAL_SEPARATOR);
ASSERT_TRUE(inserted_item_);
menu()->ChildrenChanged();
......@@ -196,22 +190,17 @@ class MenuItemViewTestInsertWithSubmenu : public MenuTestBase {
CreateEventTask(this, &MenuItemViewTestInsertWithSubmenu::Step3));
}
void Step3() {
Done();
}
void Step3() { Done(); }
private:
views::MenuItemView* submenu_;
views::MenuItemView* inserted_item_;
DISALLOW_COPY_AND_ASSIGN(MenuItemViewTestInsertWithSubmenu);
views::MenuItemView* submenu_ = nullptr;
views::MenuItemView* inserted_item_ = nullptr;
};
// MenuItemViewTestInsertWithSubmenuX posts a menu and its submenu,
// then inserts an item in the top-level menu at X.
typedef MenuItemViewTestInsertWithSubmenu<0> MenuItemViewTestInsertWithSubmenu0;
typedef MenuItemViewTestInsertWithSubmenu<1> MenuItemViewTestInsertWithSubmenu1;
using MenuItemViewTestInsertWithSubmenu0 = MenuItemViewTestInsertWithSubmenu<0>;
using MenuItemViewTestInsertWithSubmenu1 = MenuItemViewTestInsertWithSubmenu<1>;
// If this flakes, disable and log details in http://crbug.com/523255.
VIEW_TEST(MenuItemViewTestInsertWithSubmenu0, InsertItemWithSubmenu0)
......@@ -220,14 +209,13 @@ VIEW_TEST(MenuItemViewTestInsertWithSubmenu0, InsertItemWithSubmenu0)
VIEW_TEST(MenuItemViewTestInsertWithSubmenu1, InsertItemWithSubmenu1)
// Test class for removing a menu item while the menu is open.
template<int REMOVE_INDEX, int SELECT_INDEX>
template <int REMOVE_INDEX, int SELECT_INDEX>
class MenuItemViewTestRemove : public MenuTestBase {
public:
MenuItemViewTestRemove() {
}
~MenuItemViewTestRemove() override {
}
MenuItemViewTestRemove() = default;
MenuItemViewTestRemove(const MenuItemViewTestRemove&) = delete;
MenuItemViewTestRemove& operator=(const MenuItemViewTestRemove&) = delete;
~MenuItemViewTestRemove() override = default;
// MenuTestBase implementation
void BuildMenu(views::MenuItemView* menu) override {
......@@ -267,17 +255,14 @@ class MenuItemViewTestRemove : public MenuTestBase {
Done();
}
private:
DISALLOW_COPY_AND_ASSIGN(MenuItemViewTestRemove);
};
typedef MenuItemViewTestRemove<0,0> MenuItemViewTestRemove00;
typedef MenuItemViewTestRemove<0,1> MenuItemViewTestRemove01;
typedef MenuItemViewTestRemove<1,0> MenuItemViewTestRemove10;
typedef MenuItemViewTestRemove<1,1> MenuItemViewTestRemove11;
typedef MenuItemViewTestRemove<2,0> MenuItemViewTestRemove20;
typedef MenuItemViewTestRemove<2,1> MenuItemViewTestRemove21;
using MenuItemViewTestRemove00 = MenuItemViewTestRemove<0, 0>;
using MenuItemViewTestRemove01 = MenuItemViewTestRemove<0, 1>;
using MenuItemViewTestRemove10 = MenuItemViewTestRemove<1, 0>;
using MenuItemViewTestRemove11 = MenuItemViewTestRemove<1, 1>;
using MenuItemViewTestRemove20 = MenuItemViewTestRemove<2, 0>;
using MenuItemViewTestRemove21 = MenuItemViewTestRemove<2, 1>;
// If this flakes, disable and log details in http://crbug.com/523255.
VIEW_TEST(MenuItemViewTestRemove00, RemoveItem00)
......@@ -297,14 +282,15 @@ VIEW_TEST(MenuItemViewTestRemove20, RemoveItem20)
VIEW_TEST(MenuItemViewTestRemove21, RemoveItem21)
// Test class for removing a menu item while a submenu is open.
template<int REMOVE_INDEX>
template <int REMOVE_INDEX>
class MenuItemViewTestRemoveWithSubmenu : public MenuTestBase {
public:
MenuItemViewTestRemoveWithSubmenu() : submenu_(NULL) {
}
~MenuItemViewTestRemoveWithSubmenu() override {
}
MenuItemViewTestRemoveWithSubmenu() = default;
MenuItemViewTestRemoveWithSubmenu(const MenuItemViewTestRemoveWithSubmenu&) =
delete;
MenuItemViewTestRemoveWithSubmenu& operator=(
const MenuItemViewTestRemoveWithSubmenu&) = delete;
~MenuItemViewTestRemoveWithSubmenu() override = default;
// MenuTestBase implementation
void BuildMenu(views::MenuItemView* menu) override {
......@@ -350,13 +336,11 @@ class MenuItemViewTestRemoveWithSubmenu : public MenuTestBase {
}
private:
views::MenuItemView* submenu_;
DISALLOW_COPY_AND_ASSIGN(MenuItemViewTestRemoveWithSubmenu);
views::MenuItemView* submenu_ = nullptr;
};
typedef MenuItemViewTestRemoveWithSubmenu<0> MenuItemViewTestRemoveWithSubmenu0;
typedef MenuItemViewTestRemoveWithSubmenu<1> MenuItemViewTestRemoveWithSubmenu1;
using MenuItemViewTestRemoveWithSubmenu0 = MenuItemViewTestRemoveWithSubmenu<0>;
using MenuItemViewTestRemoveWithSubmenu1 = MenuItemViewTestRemoveWithSubmenu<1>;
// If this flakes, disable and log details in http://crbug.com/523255.
VIEW_TEST(MenuItemViewTestRemoveWithSubmenu0, RemoveItemWithSubmenu0)
......
......@@ -52,6 +52,10 @@ base::string16 MenuModel::GetMinorTextAt(int index) const {
return base::string16();
}
base::string16 MenuModel::GetSecondaryLabelAt(int index) const {
return base::string16();
}
ImageModel MenuModel::GetMinorIconAt(int index) const {
return ImageModel();
}
......
......@@ -65,6 +65,10 @@ class UI_BASE_EXPORT MenuModel : public base::SupportsWeakPtr<MenuModel> {
// Returns the label of the item at the specified index.
virtual base::string16 GetLabelAt(int index) const = 0;
// Returns the secondary label of the item at the specified index. Secondary
// label is shown below the label.
virtual base::string16 GetSecondaryLabelAt(int index) const;
// Returns the minor text of the item at the specified index. The minor text
// is rendered to the right of the label and using the font GetLabelFontAt().
virtual base::string16 GetMinorTextAt(int index) const;
......
......@@ -261,6 +261,7 @@ MenuItemView* MenuItemView::AddMenuItemAt(
int index,
int item_id,
const base::string16& label,
const base::string16& secondary_label,
const base::string16& minor_text,
const ui::ThemedVectorIcon& minor_icon,
const gfx::ImageSkia& icon,
......@@ -282,6 +283,7 @@ MenuItemView* MenuItemView::AddMenuItemAt(
item->SetTitle(GetDelegate()->GetLabel(item_id));
else
item->SetTitle(label);
item->SetSecondaryTitle(secondary_label);
item->SetMinorText(minor_text);
item->SetMinorIcon(minor_icon);
if (!vector_icon.empty()) {
......@@ -337,6 +339,7 @@ void MenuItemView::AppendSeparator() {
void MenuItemView::AddSeparatorAt(int index) {
AddMenuItemAt(index, /*item_id=*/0, /*label=*/base::string16(),
/*secondary_label=*/base::string16(),
/*minor_text=*/base::string16(),
/*minor_icon=*/ui::ThemedVectorIcon(),
/*icon=*/gfx::ImageSkia(),
......@@ -351,8 +354,8 @@ MenuItemView* MenuItemView::AppendMenuItemImpl(int item_id,
Type type) {
const int index = submenu_ ? int{submenu_->children().size()} : 0;
return AddMenuItemAt(index, item_id, label, base::string16(),
ui::ThemedVectorIcon(), icon, ui::ThemedVectorIcon(),
type, ui::NORMAL_SEPARATOR);
base::string16(), ui::ThemedVectorIcon(), icon,
ui::ThemedVectorIcon(), type, ui::NORMAL_SEPARATOR);
}
SubmenuView* MenuItemView::CreateSubmenu() {
......@@ -383,6 +386,11 @@ void MenuItemView::SetTitle(const base::string16& title) {
invalidate_dimensions(); // Triggers preferred size recalculation.
}
void MenuItemView::SetSecondaryTitle(const base::string16& secondary_title) {
secondary_title_ = secondary_title;
invalidate_dimensions(); // Triggers preferred size recalculation.
}
void MenuItemView::SetMinorText(const base::string16& minor_text) {
minor_text_ = minor_text;
invalidate_dimensions(); // Triggers preferred size recalculation.
......@@ -904,17 +912,22 @@ void MenuItemView::PaintButton(gfx::Canvas* canvas, PaintButtonMode mode) {
// selected.
PaintBackground(canvas, mode, render_selection);
const int top_margin = GetTopMargin();
const int bottom_margin = GetBottomMargin();
const int available_height = height() - top_margin - bottom_margin;
// Calculate some colors.
MenuDelegate::LabelStyle style;
style.foreground = GetTextColor(false, render_selection);
style.foreground = GetTextColor(/*minor=*/false, render_selection);
GetLabelStyle(&style);
SkColor icon_color = color_utils::DeriveDefaultIconColor(style.foreground);
// Calculate the margins.
int top_margin = GetTopMargin();
const int bottom_margin = GetBottomMargin();
const int available_height = height() - top_margin - bottom_margin;
const int text_height = style.font_list.GetHeight();
const int total_text_height =
secondary_title().empty() ? text_height : text_height * 2;
top_margin += (available_height - total_text_height) / 2;
// Render the check.
if (type_ == Type::kCheckbox && delegate->IsItemChecked(GetCommand())) {
radio_check_image_view_->SetImage(GetMenuCheckImage(icon_color));
......@@ -937,13 +950,22 @@ void MenuItemView::PaintButton(gfx::Canvas* canvas, PaintButtonMode mode) {
(!delegate || delegate->ShouldReserveSpaceForSubmenuIndicator()
? item_right_margin_
: config.arrow_to_edge_padding);
gfx::Rect text_bounds(label_start, top_margin, width, available_height);
gfx::Rect text_bounds(label_start, top_margin, width, text_height);
text_bounds.set_x(GetMirroredXForRect(text_bounds));
int flags = GetDrawStringFlags();
if (mode == PaintButtonMode::kForDrag)
flags |= gfx::Canvas::NO_SUBPIXEL_RENDERING;
canvas->DrawStringRectWithFlags(title(), style.font_list, style.foreground,
text_bounds, flags);
// The rest should be drawn with the minor foreground color.
style.foreground = GetTextColor(/*minor=*/true, render_selection);
if (!secondary_title().empty()) {
text_bounds.set_y(text_bounds.y() + text_height);
canvas->DrawStringRectWithFlags(secondary_title(), style.font_list,
style.foreground, text_bounds, flags);
}
PaintMinorIconAndText(canvas, style);
// Set the submenu indicator (arrow) image and color.
......@@ -1201,9 +1223,12 @@ MenuItemView::MenuItemDimensions MenuItemView::CalculateDimensions() const {
minor_text.empty() ? 0 : gfx::GetStringWidth(minor_text, style.font_list);
// Determine the height to use.
int label_text_height = secondary_title().empty()
? style.font_list.GetHeight()
: style.font_list.GetHeight() * 2;
dimensions.height =
std::max(dimensions.height, style.font_list.GetHeight() +
GetBottomMargin() + GetTopMargin());
std::max(dimensions.height,
label_text_height + GetBottomMargin() + GetTopMargin());
dimensions.height =
std::max(dimensions.height, MenuConfig::instance().item_min_height);
......
......@@ -148,6 +148,7 @@ class VIEWS_EXPORT MenuItemView : public View {
MenuItemView* AddMenuItemAt(int index,
int item_id,
const base::string16& label,
const base::string16& secondary_label,
const base::string16& minor_text,
const ui::ThemedVectorIcon& minor_icon,
const gfx::ImageSkia& icon,
......@@ -214,6 +215,11 @@ class VIEWS_EXPORT MenuItemView : public View {
void SetTitle(const base::string16& title);
const base::string16& title() const { return title_; }
// Sets/Gets the secondary title. When not empty, they are shown in the line
// below the title.
void SetSecondaryTitle(const base::string16& secondary_title);
const base::string16& secondary_title() const { return secondary_title_; }
// Sets the minor text.
void SetMinorText(const base::string16& minor_text);
......@@ -508,13 +514,9 @@ class VIEWS_EXPORT MenuItemView : public View {
// Submenu, created via CreateSubmenu.
SubmenuView* submenu_ = nullptr;
// Title.
base::string16 title_;
// Minor text.
base::string16 secondary_title_;
base::string16 minor_text_;
// Minor icon.
ui::ThemedVectorIcon minor_icon_;
// The icon used for |icon_view_| when a vector icon has been set instead of a
......
......@@ -320,20 +320,37 @@ class MenuItemViewPaintUnitTest : public ViewsTestBase {
DISALLOW_COPY_AND_ASSIGN(MenuItemViewPaintUnitTest);
};
// Provides assertion coverage for painting minor text and icons.
// Provides assertion coverage for painting, secondary label, minor text and
// icons.
TEST_F(MenuItemViewPaintUnitTest, MinorTextAndIconAssertionCoverage) {
auto AddItem = [this](auto label, auto minor_label, auto minor_icon) {
auto AddItem = [this](auto label, auto secondary_label, auto minor_label,
auto minor_icon) {
menu_item_view()->AddMenuItemAt(
0, 1000, base::ASCIIToUTF16(label), minor_label, minor_icon,
gfx::ImageSkia(), ui::ThemedVectorIcon(),
0, 1000, base::ASCIIToUTF16(label), secondary_label, minor_label,
minor_icon, gfx::ImageSkia(), ui::ThemedVectorIcon(),
views::MenuItemView::Type::kNormal, ui::NORMAL_SEPARATOR);
};
AddItem("No minor content", base::string16(), ui::ThemedVectorIcon());
AddItem("Minor text only", base::ASCIIToUTF16("minor text"),
AddItem("No secondary label, no minor content", base::string16(),
base::string16(), ui::ThemedVectorIcon());
AddItem("No secondary label, minor text only", base::string16(),
base::ASCIIToUTF16("minor text"), ui::ThemedVectorIcon());
AddItem("No secondary label, minor icon only", base::string16(),
base::string16(), ui::ThemedVectorIcon(&views::kMenuCheckIcon));
AddItem("No secondary label, minor text and icon", base::string16(),
base::ASCIIToUTF16("minor text"),
ui::ThemedVectorIcon(&views::kMenuCheckIcon));
AddItem("Secondary label, no minor content",
base::ASCIIToUTF16("secondary label"), base::string16(),
ui::ThemedVectorIcon());
AddItem("Minor icon only", base::string16(),
AddItem("Secondary label, minor text only",
base::ASCIIToUTF16("secondary label"),
base::ASCIIToUTF16("minor text"), ui::ThemedVectorIcon());
AddItem("Secondary label, minor icon only",
base::ASCIIToUTF16("secondary label"), base::string16(),
ui::ThemedVectorIcon(&views::kMenuCheckIcon));
AddItem("Minor text and icon", base::ASCIIToUTF16("minor text"),
AddItem("Secondary label, minor text and icon",
base::ASCIIToUTF16("secondary label"),
base::ASCIIToUTF16("minor text"),
ui::ThemedVectorIcon(&views::kMenuCheckIcon));
menu_runner()->RunMenuAt(widget(), nullptr, gfx::Rect(),
......
......@@ -99,16 +99,17 @@ MenuItemView* MenuModelAdapter::AddMenuItemFromModelAt(ui::MenuModel* model,
}
if (*type == MenuItemView::Type::kSeparator) {
return menu->AddMenuItemAt(menu_index, item_id, base::string16(),
base::string16(), ui::ThemedVectorIcon(),
gfx::ImageSkia(), ui::ThemedVectorIcon(), *type,
model->GetSeparatorTypeAt(model_index));
return menu->AddMenuItemAt(
menu_index, item_id, base::string16(), base::string16(),
base::string16(), ui::ThemedVectorIcon(), gfx::ImageSkia(),
ui::ThemedVectorIcon(), *type, model->GetSeparatorTypeAt(model_index));
}
ui::ImageModel icon = model->GetIconAt(model_index);
ui::ImageModel minor_icon = model->GetMinorIconAt(model_index);
return menu->AddMenuItemAt(
menu_index, item_id, model->GetLabelAt(model_index),
model->GetSecondaryLabelAt(model_index),
model->GetMinorTextAt(model_index),
minor_icon.IsVectorIcon()
? ui::ThemedVectorIcon(minor_icon.GetVectorIcon())
......
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