bookmarks: show ampersands in bookmark titles in app menu

This change:
1) Moves and renames ui::EscapeWindowsStyleAccelerators to make it available on
   other platforms;
2) Has BookmarkMenuDelegate call that function if the bookmark menu is being
   shown in the app menu;
3) Has BackForwardMenuModel call that function instead of hand-rolling escaping

In MenuItemViews whose root MenuItemView has_mnemonics(), '&' is interpreted
as a mnemonic marker, which makes it necessary to escape them.

Bug: 896550,788049
Change-Id: I83f6bc6f5b71efa1d7220d8d20afa7dd833771ba
Reviewed-on: https://chromium-review.googlesource.com/c/1287325Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601744}

chrome/browser/ui/toolbar/back_forward_menu_model.cc

@@ -28,6 +28,7 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "ui/base/accelerators/menu_label_accelerator_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/window_open_disposition.h"
@@ -98,16 +99,10 @@ base::string16 BackForwardMenuModel::GetLabelAt(int index) const {
   // super long.
   NavigationEntry* entry = GetNavigationEntry(index);
   base::string16 menu_text(entry->GetTitleForDisplay());
+  menu_text = ui::EscapeMenuLabelAmpersands(menu_text);
   menu_text = gfx::ElideText(menu_text, gfx::FontList(), kMaxWidth,
                              gfx::ELIDE_TAIL, gfx::Typesetter::NATIVE);
 
-#if !defined(OS_MACOSX)
-  for (size_t i = menu_text.find('&'); i != base::string16::npos;
-       i = menu_text.find('&', i + 2)) {
-    menu_text.insert(i, 1, '&');
-  }
-#endif
-
   return menu_text;
 }
 

chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc

@@ -476,18 +476,10 @@ TEST_F(BackFwdMenuModelTest, EscapeLabel) {
 
   EXPECT_EQ(6, back_model->GetItemCount());
 
-  // On Mac ui::MenuModel::GetLabelAt should return unescaped strings.
-#if defined(OS_MACOSX)
-  EXPECT_EQ(ASCIIToUTF16("A B"), back_model->GetLabelAt(3));
-  EXPECT_EQ(ASCIIToUTF16("A & B"), back_model->GetLabelAt(2));
-  EXPECT_EQ(ASCIIToUTF16("A && B"), back_model->GetLabelAt(1));
-  EXPECT_EQ(ASCIIToUTF16("A &&& B"), back_model->GetLabelAt(0));
-#else
   EXPECT_EQ(ASCIIToUTF16("A B"), back_model->GetLabelAt(3));
   EXPECT_EQ(ASCIIToUTF16("A && B"), back_model->GetLabelAt(2));
   EXPECT_EQ(ASCIIToUTF16("A &&&& B"), back_model->GetLabelAt(1));
   EXPECT_EQ(ASCIIToUTF16("A &&&&&& B"), back_model->GetLabelAt(0));
-#endif // defined(OS_MACOSX)
 }
 
 // Test asynchronous loading of favicon from history service.

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc

@@ -23,6 +23,7 @@
 #include "components/bookmarks/managed/managed_bookmark_service.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/page_navigator.h"
+#include "ui/base/accelerators/menu_label_accelerator_util.h"
 #include "ui/base/dragdrop/os_exchange_data.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
@@ -91,6 +92,10 @@ void BookmarkMenuDelegate::Init(views::MenuDelegate* real_delegate,
   GetBookmarkModel()->AddObserver(this);
   real_delegate_ = real_delegate;
   location_ = location;
+  // Assume that the menu will only use mnemonics if there's already a parent
+  // menu and that parent uses them. In cases where the BookmarkMenuDelegate
+  // will be the root, client code does not current enable mnemonics.
+  menu_uses_mnemonics_ = parent && parent->GetRootMenuItem()->has_mnemonics();
   if (parent) {
     parent_menu_item_ = parent;
 
@@ -512,9 +517,9 @@ void BookmarkMenuDelegate::BuildMenuForPermanentNode(const BookmarkNode* node,
     menu->AppendSeparator();
   }
 
-  AddMenuToMaps(
-      menu->AppendSubMenuWithIcon(next_menu_id_++, node->GetTitle(), icon),
-      node);
+  AddMenuToMaps(menu->AppendSubMenuWithIcon(
+                    next_menu_id_++, MaybeEscapeLabel(node->GetTitle()), icon),
+                node);
 }
 
 void BookmarkMenuDelegate::BuildMenuForManagedNode(MenuItemView* menu) {
@@ -542,12 +547,12 @@ void BookmarkMenuDelegate::BuildMenu(const BookmarkNode* parent,
       const gfx::Image& image = GetBookmarkModel()->GetFavicon(node);
       const gfx::ImageSkia* icon = image.IsEmpty() ?
           rb->GetImageSkiaNamed(IDR_DEFAULT_FAVICON) : image.ToImageSkia();
-      child_menu_item =
-          menu->AppendMenuItemWithIcon(id, node->GetTitle(), *icon);
+      child_menu_item = menu->AppendMenuItemWithIcon(
+          id, MaybeEscapeLabel(node->GetTitle()), *icon);
     } else {
       DCHECK(node->is_folder());
-      child_menu_item =
-          menu->AppendSubMenuWithIcon(id, node->GetTitle(), folder_icon);
+      child_menu_item = menu->AppendSubMenuWithIcon(
+          id, MaybeEscapeLabel(node->GetTitle()), folder_icon);
     }
     AddMenuToMaps(child_menu_item, node);
   }
@@ -558,3 +563,8 @@ void BookmarkMenuDelegate::AddMenuToMaps(MenuItemView* menu,
   menu_id_to_node_map_[menu->GetCommand()] = node;
   node_to_menu_map_[node] = menu;
 }
+
+base::string16 BookmarkMenuDelegate::MaybeEscapeLabel(
+    const base::string16& label) {
+  return menu_uses_mnemonics_ ? ui::EscapeMenuLabelAmpersands(label) : label;
+}

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h

@@ -182,6 +182,10 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   void AddMenuToMaps(views::MenuItemView* menu,
                      const bookmarks::BookmarkNode* node);
 
+  // Escapes ampersands within |title| if necessary, depending on
+  // |menu_uses_mnemonics_|.
+  base::string16 MaybeEscapeLabel(const base::string16& title);
+
   Browser* const browser_;
   Profile* profile_;
 
@@ -219,6 +223,10 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   // The location where this bookmark menu will be displayed (for UMA).
   BookmarkLaunchLocation location_;
 
+  // Whether the involved menu uses mnemonics or not. If it does, ampersands
+  // inside bookmark titles need to be escaped.
+  bool menu_uses_mnemonics_;
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkMenuDelegate);
 };
 

ui/base/accelerators/menu_label_accelerator_util.cc

@@ -5,6 +5,7 @@
 #include "ui/base/accelerators/menu_label_accelerator_util.h"
 
 #include "base/i18n/case_conversion.h"
+#include "base/strings/string_util.h"
 
 namespace ui {
 
@@ -31,4 +32,12 @@ base::char16 GetMnemonic(const base::string16& label) {
   return 0;
 }
 
+base::string16 EscapeMenuLabelAmpersands(const base::string16& label) {
+  base::string16 ret;
+  static const base::char16 kAmps[] = {'&', 0};
+  static const base::char16 kTwoAmps[] = {'&', '&', 0};
+  base::ReplaceChars(label, kAmps, kTwoAmps, &ret);
+  return ret;
+}
+
 }  // namespace ui

ui/base/accelerators/menu_label_accelerator_util.h

@@ -14,6 +14,11 @@ namespace ui {
 
 UI_BASE_EXPORT base::char16 GetMnemonic(const base::string16& label);
 
+// This function escapes every '&' in label by replacing it with '&&', to avoid
+// having single ampersands in user-provided strings treated as accelerators.
+UI_BASE_EXPORT base::string16 EscapeMenuLabelAmpersands(
+    const base::string16& label);
+
 }  // namespace ui
 
 #endif  // UI_BASE_ACCELERATORS_MENU_LABEL_ACCELERATOR_UTIL_H_

ui/base/accelerators/menu_label_accelerator_util_linux.cc

@@ -6,8 +6,6 @@
 
 #include <stddef.h>
 
-#include "base/strings/string_util.h"
-
 namespace {
 
 // Common implementation of ConvertAcceleratorsFromWindowsStyle() and
@@ -51,14 +49,4 @@ std::string RemoveWindowsStyleAccelerators(const std::string& label) {
   return ConvertAmpersandsTo(label, std::string());
 }
 
-// Replaces all ampersands in |label| with two ampersands. This effectively
-// escapes strings for later processing by ConvertAmpersandsTo(), so that
-// ConvertAmpersandsTo(EscapeWindowsStyleAccelerators(x), *) is |x| with
-// underscores doubled, making the string that appears to the user just |x|.
-std::string EscapeWindowsStyleAccelerators(const std::string& label) {
-  std::string ret;
-  base::ReplaceChars(label, "&", "&&", &ret);
-  return ret;
-}
-
 }  // namespace ui

ui/base/accelerators/menu_label_accelerator_util_linux.h

@@ -20,12 +20,6 @@ UI_BASE_EXPORT std::string ConvertAcceleratorsFromWindowsStyle(
 UI_BASE_EXPORT std::string RemoveWindowsStyleAccelerators(
     const std::string& label);
 
-// Escapes "&" characters by doubling them so that later calling
-// ConvertAcceleratorsFromWindowsStyle() will return the original string (except
-// with "_" characters doubled, to escape them for GTK).
-UI_BASE_EXPORT std::string EscapeWindowsStyleAccelerators(
-    const std::string& label);
-
 }  // namespace ui
 
 #endif  // UI_BASE_ACCELERATORS_MENU_LABEL_ACCELERATOR_UTIL_LINUX_H_

ui/base/accelerators/menu_label_accelerator_util_linux_unittest.cc

@@ -49,29 +49,4 @@ TEST(MenuLabelAcceleratorTest, RemoveWindowsStyleAccelerators) {
   }
 }
 
-TEST(MenuLabelAcceleratorTest, EscapeWindowsStyleAccelerators) {
-  static const struct {
-    const char* input;
-    const char* output;
-  } cases[] = {
-    { "nothing", "nothing" },
-    { "foo &bar", "foo &&bar" },
-    { "foo &&bar", "foo &&&&bar" },
-    { "foo &&&bar", "foo &&&&&&bar" },
-    { "&foo bar", "&&foo bar" },
-    { "&&foo bar", "&&&&foo bar" },
-    { "&&&foo bar", "&&&&&&foo bar" },
-    { "&foo &bar", "&&foo &&bar" },
-    { "&&foo &&bar", "&&&&foo &&&&bar" },
-    { "f&o&o ba&r", "f&&o&&o ba&&r" },
-    { "foo_&_bar", "foo_&&_bar" },
-    { "&_foo_bar_&", "&&_foo_bar_&&" },
-  };
-
-  for (size_t i = 0; i < arraysize(cases); ++i) {
-    std::string result = EscapeWindowsStyleAccelerators(cases[i].input);
-    EXPECT_EQ(cases[i].output, result);
-  }
-}
-
 }  // namespace ui

ui/base/accelerators/menu_label_accelerator_util_unittest.cc

@@ -22,4 +22,30 @@ TEST(MenuLabelAcceleratorTest, GetMnemonic) {
     EXPECT_EQ(GetMnemonic(test.label), test.mneumonic);
 }
 
+TEST(MenuLabelAcceleratorTest, EscapeMenuLabelAmpersands) {
+  static const struct {
+    const char* input;
+    const char* output;
+  } cases[] = {
+      {"nothing", "nothing"},
+      {"foo &bar", "foo &&bar"},
+      {"foo &&bar", "foo &&&&bar"},
+      {"foo &&&bar", "foo &&&&&&bar"},
+      {"&foo bar", "&&foo bar"},
+      {"&&foo bar", "&&&&foo bar"},
+      {"&&&foo bar", "&&&&&&foo bar"},
+      {"&foo &bar", "&&foo &&bar"},
+      {"&&foo &&bar", "&&&&foo &&&&bar"},
+      {"f&o&o ba&r", "f&&o&&o ba&&r"},
+      {"foo_&_bar", "foo_&&_bar"},
+      {"&_foo_bar_&", "&&_foo_bar_&&"},
+  };
+
+  for (const auto& test : cases) {
+    base::string16 in = base::ASCIIToUTF16(test.input);
+    base::string16 out = base::ASCIIToUTF16(test.output);
+    EXPECT_EQ(out, EscapeMenuLabelAmpersands(in));
+  }
+}
+
 }  // namespace ui