macviews: change menu widths from fixed to minimums

Fixed menu widths turned out not to work well in locales with longer
strings. This change changes the menu widths to be minimums instead.
See screenshots attached to the bug for examples.

Bug: 832239
Change-Id: I4cae9efb53b86b08c4cd8b56d4eb089446e872d9
Reviewed-on: https://chromium-review.googlesource.com/1026052
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553584}

ui/views/controls/menu/menu_config.cc

@@ -18,8 +18,9 @@ MenuConfig::MenuConfig()
       item_bottom_margin(3),
       item_no_icon_top_margin(4),
       item_no_icon_bottom_margin(4),
-      fixed_text_item_height(0),
-      fixed_menu_width(0),
+      minimum_text_item_height(0),
+      minimum_container_item_height(0),
+      minimum_menu_width(0),
       item_left_margin(10),
       touchable_item_left_margin(16),
       label_to_arrow_padding(10),

ui/views/controls/menu/menu_config.h

@@ -44,12 +44,12 @@ struct VIEWS_EXPORT MenuConfig {
   int item_no_icon_top_margin;
   int item_no_icon_bottom_margin;
 
-  // Fixed dimensions used for entire items. If these are nonzero, they override
-  // the vertical margin constants given above - the item's text and icon are
-  // vertically centered within these heights.
-  int fixed_text_item_height;
-  int fixed_container_item_height;
-  int fixed_menu_width;
+  // Minimum dimensions used for entire items. If these are nonzero, they
+  // override the vertical margin constants given above - the item's text and
+  // icon are vertically centered within these heights.
+  int minimum_text_item_height;
+  int minimum_container_item_height;
+  int minimum_menu_width;
 
   // Margins between the left of the item and the icon.
   int item_left_margin;

ui/views/controls/menu/menu_config_mac.mm

@@ -17,9 +17,9 @@ void InitMaterialMenuConfig(views::MenuConfig* config) {
   config->menu_vertical_border_size = 8;
   config->menu_horizontal_border_size = 0;
   config->submenu_horizontal_inset = 0;
-  config->fixed_text_item_height = 32;
-  config->fixed_container_item_height = 48;
-  config->fixed_menu_width = 320;
+  config->minimum_text_item_height = 32;
+  config->minimum_container_item_height = 48;
+  config->minimum_menu_width = 320;
   config->item_left_margin = 8;
   config->label_to_arrow_padding = 0;
   config->arrow_to_edge_padding = 16;

ui/views/controls/menu/menu_item_view.cc

@@ -1042,22 +1042,6 @@ MenuItemView::MenuItemDimensions MenuItemView::CalculateDimensions() const {
 
   const gfx::FontList& font_list = GetFontList();
   base::string16 minor_text = GetMinorText();
-  if (menu_config.fixed_text_item_height &&
-      menu_config.fixed_container_item_height && menu_config.fixed_menu_width &&
-      GetMenuController() && !GetMenuController()->is_combobox()) {
-    bool has_children = NonIconChildViewsCount() > 0;
-    dimensions.height = has_children ? menu_config.fixed_container_item_height
-                                     : menu_config.fixed_text_item_height;
-    dimensions.children_width = 0;
-    dimensions.minor_text_width =
-        minor_text.empty() ? 0 : gfx::GetStringWidth(minor_text, font_list);
-    int leave_for_minor = dimensions.minor_text_width
-                              ? dimensions.minor_text_width +
-                                    menu_config.label_to_minor_text_padding
-                              : 0;
-    dimensions.standard_width = menu_config.fixed_menu_width - leave_for_minor;
-    return dimensions;
-  }
 
   dimensions.height = child_size.height();
   // Adjust item content height if menu has both items with and without icons.
@@ -1069,8 +1053,10 @@ MenuItemView::MenuItemDimensions MenuItemView::CalculateDimensions() const {
   dimensions.height += GetBottomMargin() + GetTopMargin();
 
   // In case of a container, only the container size needs to be filled.
-  if (IsContainer())
+  if (IsContainer()) {
+    ApplyMinimumDimensions(&dimensions);
     return dimensions;
+  }
 
   // Get Icon margin overrides for this particular item.
   const MenuDelegate* delegate = GetDelegate();
@@ -1105,9 +1091,23 @@ MenuItemView::MenuItemDimensions MenuItemView::CalculateDimensions() const {
                font_list.GetHeight() + GetBottomMargin() + GetTopMargin());
   dimensions.height =
       std::max(dimensions.height, MenuConfig::instance().item_min_height);
+
+  ApplyMinimumDimensions(&dimensions);
   return dimensions;
 }
 
+void MenuItemView::ApplyMinimumDimensions(MenuItemDimensions* dims) const {
+  int used =
+      dims->standard_width + dims->children_width + dims->minor_text_width;
+  const MenuConfig& config = MenuConfig::instance();
+  if (used < config.minimum_menu_width)
+    dims->standard_width += (config.minimum_menu_width - used);
+
+  dims->height = std::max(dims->height,
+                          IsContainer() ? config.minimum_container_item_height
+                                        : config.minimum_text_item_height);
+}
+
 int MenuItemView::GetLabelStartForThisItem() const {
   const MenuConfig& config = MenuConfig::instance();
   int label_start = label_start_ + left_icon_margin_ + right_icon_margin_;

ui/views/controls/menu/menu_item_view.h

@@ -430,6 +430,14 @@ class VIEWS_EXPORT MenuItemView : public View {
   // Calculates and returns the MenuItemDimensions.
   MenuItemDimensions CalculateDimensions() const;
 
+  // Imposes MenuConfig's minimum sizes, if any, on the supplied
+  // dimensions and returns the new dimensions. It is guaranteed that:
+  //    ApplyMinimumDimensions(x).standard_width >= x.standard_width
+  //    ApplyMinimumDimensions(x).children_width == x.children_width
+  //    ApplyMinimumDimensions(x).minor_text_width == x.minor_text_width
+  //    ApplyMinimumDimensions(x).height >= x.height
+  void ApplyMinimumDimensions(MenuItemDimensions* dims) const;
+
   // Get the horizontal position at which to draw the menu item's label.
   int GetLabelStartForThisItem() const;
 

ui/views/controls/menu/menu_item_view_unittest.cc

@@ -70,17 +70,25 @@ TEST(MenuItemViewUnitTest, TestMenuItemViewWithFlexibleWidthChild) {
   ASSERT_EQ(flexible_view, submenu->GetMenuItemAt(1));
   gfx::Size flexible_size = flexible_view->GetPreferredSize();
 
-  // The flexible view's "preferred size" should be 1x1...
-  EXPECT_EQ(flexible_size, gfx::Size(1, 1));
-
-  // ...but it should use whatever space is available to make a square.
-  int flex_height = flexible_view->GetHeightForWidth(label_size.width());
-  EXPECT_EQ(label_size.width(), flex_height);
-
-  // The submenu should be tall enough to allow for both menu items at the given
-  // width.
-  EXPECT_EQ(label_size.height() + flex_height,
-            submenu->GetPreferredSize().height());
+  const MenuConfig& config = MenuConfig::instance();
+
+  if (config.minimum_menu_width)
+    EXPECT_EQ(flexible_size.width(), config.minimum_menu_width);
+  else
+    EXPECT_EQ(flexible_size.width(), 1);
+
+  if (!config.minimum_container_item_height) {
+    // ...but it should use whatever space is available to make a square.
+    int flex_height = flexible_view->GetHeightForWidth(label_size.width());
+    EXPECT_EQ(label_size.width(), flex_height);
+
+    // The submenu should be tall enough to allow for both menu items at the
+    // given width.
+    EXPECT_EQ(label_size.height() + flex_height,
+              submenu->GetPreferredSize().height());
+  } else {
+    EXPECT_EQ(flexible_size.height(), config.minimum_container_item_height);
+  }
 }
 
 // Tests that the top-level menu item with hidden children should contain the