Fix CJK menu accelerator stripping

On platforms where accelerators are not shown in menus,
and in circumstances where the underline is not drawn,
fully strip the CJK style of accelerators "(&x)".

Note that this was already done for Mac native menus
and associated Mac native code in FixUpWindowsStyleLabel().

Fixed: 974843
Change-Id: Ifc89fc574213cd350da8e992ac02683afec3718c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2546471
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829259}

chrome/browser/ui/views/toolbar/app_menu.cc

@@ -315,8 +315,7 @@ class AppMenuView : public views::View {
     DCHECK(menu_);
     InMenuButton* button = new InMenuButton(
         std::move(callback),
-        gfx::RemoveAcceleratorChar(l10n_util::GetStringUTF16(string_id), '&',
-                                   nullptr, nullptr));
+        gfx::RemoveAccelerator(l10n_util::GetStringUTF16(string_id)));
     button->Init(type);
     button->SetAccessibleName(GetAccessibleNameForAppMenuItem(
         menu_model_, index, acc_string_id, add_accelerator_text));

ui/gfx/canvas_skia.cc

@@ -25,13 +25,16 @@ namespace {
 // Returns a range in |text| to underline or Range::InvalidRange() if
 // underlining is not needed.
 Range StripAcceleratorChars(int flags, base::string16* text) {
-  if (flags & (Canvas::SHOW_PREFIX | Canvas::HIDE_PREFIX)) {
+  if (flags & Canvas::SHOW_PREFIX) {
     int char_pos = -1;
     int char_span = 0;
-    *text = RemoveAcceleratorChar(*text, '&', &char_pos, &char_span);
-    if ((flags & Canvas::SHOW_PREFIX) && char_pos != -1)
+    *text = LocateAndRemoveAcceleratorChar(*text, &char_pos, &char_span);
+    if (char_pos != -1)
       return Range(char_pos, char_pos + char_span);
+  } else if (flags & Canvas::HIDE_PREFIX) {
+    *text = RemoveAccelerator(*text);
   }
+
   return Range::InvalidRange();
 }
 

ui/gfx/text_utils.cc

@@ -22,6 +22,10 @@ using base::i18n::UTF16CharIterator;
 
 namespace {
 
+constexpr base::char16 kAcceleratorChar = '&';
+constexpr base::char16 kOpenParenthesisChar = '(';
+constexpr base::char16 kCloseParenthesisChar = ')';
+
 // Returns true if the specified character must be elided from a string.
 // Examples are combining marks and whitespace.
 bool IsCombiningMark(UChar32 c) {
@@ -39,10 +43,8 @@ bool IsSpace(UChar32 c) {
          char_type == U_PARAGRAPH_SEPARATOR || char_type == U_CONTROL_CHAR;
 }
 
-}  // namespace
-
-base::string16 RemoveAcceleratorChar(const base::string16& s,
-                                     base::char16 accelerator_char,
+base::string16 RemoveAcceleratorChar(bool full_removal,
+                                     const base::string16& s,
                                      int* accelerated_char_pos,
                                      int* accelerated_char_span) {
   bool escaped = false;
@@ -51,15 +53,47 @@ base::string16 RemoveAcceleratorChar(const base::string16& s,
   UTF16CharIterator chars(s);
   base::string16 accelerator_removed;
 
+  // The states of a state machine looking for a CJK-style accelerator (i.e.
+  // "(&x)"). |cjk_state| proceeds up from |kFoundNothing| through these states,
+  // resetting either when it sees a complete accelerator, or gives up because
+  // the current character doesn't match.
+  enum {
+    kFoundNothing,
+    kFoundOpenParen,
+    kFoundAcceleratorChar,
+    kFoundAccelerator
+  } cjk_state = kFoundNothing;
+  size_t pre_cjk_size = 0;
+
   accelerator_removed.reserve(s.size());
   while (!chars.end()) {
     int32_t c = chars.get();
     int array_pos = chars.array_pos();
     chars.Advance();
 
-    if (c != accelerator_char || escaped) {
+    if (full_removal) {
+      if (cjk_state == kFoundNothing && c == kOpenParenthesisChar) {
+        pre_cjk_size = array_pos;
+        cjk_state = kFoundOpenParen;
+      } else if (cjk_state == kFoundOpenParen && c == kAcceleratorChar) {
+        cjk_state = kFoundAcceleratorChar;
+      } else if (cjk_state == kFoundAcceleratorChar) {
+        // Accept any character as the accelerator.
+        cjk_state = kFoundAccelerator;
+      } else if (cjk_state == kFoundAccelerator && c == kCloseParenthesisChar) {
+        cjk_state = kFoundNothing;
+        accelerator_removed.resize(pre_cjk_size);
+        pre_cjk_size = 0;
+        escaped = false;
+        continue;
+      } else {
+        cjk_state = kFoundNothing;
+      }
+    }
+
+    if (c != kAcceleratorChar || escaped) {
       int span = chars.array_pos() - array_pos;
-      if (escaped && c != accelerator_char) {
+      if (escaped && c != kAcceleratorChar) {
         last_char_pos = accelerator_removed.size();
         last_char_span = span;
       }
@@ -71,14 +105,27 @@ base::string16 RemoveAcceleratorChar(const base::string16& s,
     }
   }
 
-  if (accelerated_char_pos)
+  if (accelerated_char_pos && !full_removal)
     *accelerated_char_pos = last_char_pos;
-  if (accelerated_char_span)
+  if (accelerated_char_span && !full_removal)
     *accelerated_char_span = last_char_span;
 
   return accelerator_removed;
 }
 
+}  // namespace
+
+base::string16 LocateAndRemoveAcceleratorChar(const base::string16& s,
+                                              int* accelerated_char_pos,
+                                              int* accelerated_char_span) {
+  return RemoveAcceleratorChar(false, s, accelerated_char_pos,
+                               accelerated_char_span);
+}
+
+base::string16 RemoveAccelerator(const base::string16& s) {
+  return RemoveAcceleratorChar(true, s, nullptr, nullptr);
+}
+
 size_t FindValidBoundaryBefore(const base::string16& text,
                                size_t index,
                                bool trim_whitespace) {

ui/gfx/text_utils.h

@@ -17,15 +17,27 @@ class FontList;
 class Insets;
 class Size;
 
-// Strip the accelerator char (typically '&') from a menu string.  A double
-// accelerator char ('&&') will be converted to a single char.  The out params
+// Strips the accelerator char ('&') from a menu string. Useful for platforms
+// which use underlining to indicate accelerators.
+//
+// Single accelerator chars ('&') will be stripped from the string. Double
+// accelerator chars ('&&') will be converted to a single '&'. The out params
 // |accelerated_char_pos| and |accelerated_char_span| will be set to the index
 // and span of the last accelerated character, respectively, or -1 and 0 if
 // there was none.
-GFX_EXPORT base::string16 RemoveAcceleratorChar(const base::string16& s,
-                                                base::char16 accelerator_char,
-                                                int* accelerated_char_pos,
-                                                int* accelerated_char_span);
+GFX_EXPORT base::string16 LocateAndRemoveAcceleratorChar(
+    const base::string16& s,
+    int* accelerated_char_pos,
+    int* accelerated_char_span);
+
+// Strips all accelerator notation from a menu string. Useful for platforms
+// which use underlining to indicate accelerators, as well as situations where
+// accelerators are not indicated.
+//
+// Single accelerator chars ('&') will be stripped from the string. Double
+// accelerator chars ('&&') will be converted to a single '&'. CJK language
+// accelerators, specified as "(&x)", will be entirely removed too.
+GFX_EXPORT base::string16 RemoveAccelerator(const base::string16& s);
 
 // Returns the number of horizontal pixels needed to display the specified
 // |text| with |font_list|. |typesetter| indicates where the text will be

ui/gfx/text_utils_unittest.cc

@@ -21,16 +21,6 @@
 namespace gfx {
 namespace {
 
-const base::char16 kAcceleratorChar = '&';
-
-struct RemoveAcceleratorCharData {
-  const char* input;
-  int accelerated_char_pos;
-  int accelerated_char_span;
-  const char* output;
-  const char* name;
-};
-
 TEST(TextUtilsTest, GetStringWidth) {
   FontList font_list;
   EXPECT_EQ(GetStringWidth(base::string16(), font_list), 0);
@@ -117,6 +107,15 @@ TEST(TextUtilsTest, GetFontCapHeightCenterOffset_SameSize) {
   EXPECT_EQ(0, GetFontCapHeightCenterOffset(original_font, original_font));
 }
 
+struct RemoveAcceleratorCharData {
+  const char* input;
+  int accelerated_char_pos;
+  int accelerated_char_span;
+  const char* output_locate_and_strip;
+  const char* output_full_strip;
+  const char* name;
+};
+
 class RemoveAcceleratorCharTest
     : public testing::TestWithParam<RemoveAcceleratorCharData> {
  public:
@@ -124,37 +123,62 @@ class RemoveAcceleratorCharTest
 };
 
 const RemoveAcceleratorCharData RemoveAcceleratorCharTest::kCases[] = {
-    {"", -1, 0, "", "EmptyString"},
-    {"&", -1, 0, "", "AcceleratorCharOnly"},
-    {"no accelerator", -1, 0, "no accelerator", "NoAccelerator"},
-    {"&one accelerator", 0, 1, "one accelerator", "OneAccelerator_Start"},
-    {"one &accelerator", 4, 1, "one accelerator", "OneAccelerator_Middle"},
-    {"one_accelerator&", -1, 0, "one_accelerator", "OneAccelerator_End"},
-    {"&two &accelerators", 4, 1, "two accelerators",
+    {"", -1, 0, "", "", "EmptyString"},
+    {"&", -1, 0, "", "", "AcceleratorCharOnly"},
+    {"no accelerator", -1, 0, "no accelerator", "no accelerator",
+     "NoAccelerator"},
+    {"&one accelerator", 0, 1, "one accelerator", "one accelerator",
+     "OneAccelerator_Start"},
+    {"one &accelerator", 4, 1, "one accelerator", "one accelerator",
+     "OneAccelerator_Middle"},
+    {"one accelerator&", -1, 0, "one accelerator", "one accelerator",
+     "OneAccelerator_End"},
+    {"&two &accelerators", 4, 1, "two accelerators", "two accelerators",
      "TwoAccelerators_OneAtStart"},
-    {"two &accelerators&", 4, 1, "two accelerators",
+    {"two &accelerators&", 4, 1, "two accelerators", "two accelerators",
      "TwoAccelerators_OneAtEnd"},
-    {"two& &accelerators", 4, 1, "two accelerators",
+    {"two& &accelerators", 4, 1, "two accelerators", "two accelerators",
      "TwoAccelerators_SpaceBetween"},
-    {"&&escaping", -1, 0, "&escaping", "Escape_Start"},
-    {"escap&&ing", -1, 0, "escap&ing", "Escape_Middle"},
-    {"escaping&&", -1, 0, "escaping&", "Escape_End"},
-    {"&mix&&ed", 0, 1, "mix&ed", "Mixed_EscapeAfterAccelerator"},
-    {"&&m&ix&&e&d&", 6, 1, "&mix&ed", "Mixed_MiddleAcceleratorSkipped"},
-    {"&&m&&ix&ed&&", 5, 1, "&m&ixed&", "Mixed_OneAccelerator"},
-    {"&m&&ix&ed&&", 4, 1, "m&ixed&", "Mixed_InitialAcceleratorSkipped"},
+    {"&&escaping", -1, 0, "&escaping", "&escaping", "Escape_Start"},
+    {"escap&&ing", -1, 0, "escap&ing", "escap&ing", "Escape_Middle"},
+    {"escaping&&", -1, 0, "escaping&", "escaping&", "Escape_End"},
+    {"accelerator(&A)", 12, 1, "accelerator(A)", "accelerator", "CJK_Style"},
+    {"accelerator(&A)...", 12, 1, "accelerator(A)...", "accelerator...",
+     "CJK_StyleEllipsis"},
+    {"accelerator(paren", -1, 0, "accelerator(paren", "accelerator(paren",
+     "CJK_OpenParen"},
+    {"accelerator(&paren", 12, 1, "accelerator(paren", "accelerator(paren",
+     "CJK_NoClosingParen"},
+    {"accelerator(&paren)", 12, 1, "accelerator(paren)", "accelerator(paren)",
+     "CJK_LateClosingParen"},
+    {"accelerator&paren)", 11, 1, "acceleratorparen)", "acceleratorparen)",
+     "CJK_NoOpeningParen"},
+    {"accelerator(P)", -1, 0, "accelerator(P)", "accelerator(P)",
+     "CJK_JustParens"},
+    {"accelerator(&)", 12, 1, "accelerator()", "accelerator()",
+     "CJK_MissingAccelerator"},
+    {"&mix&&ed", 0, 1, "mix&ed", "mix&ed", "Mixed_EscapeAfterAccelerator"},
+    {"&&m&ix&&e&d&", 6, 1, "&mix&ed", "&mix&ed",
+     "Mixed_MiddleAcceleratorSkipped"},
+    {"&&m&&ix&ed&&", 5, 1, "&m&ixed&", "&m&ixed&", "Mixed_OneAccelerator"},
+    {"&m&&ix&ed&&", 4, 1, "m&ixed&", "m&ixed&",
+     "Mixed_InitialAcceleratorSkipped"},
     // U+1D49C MATHEMATICAL SCRIPT CAPITAL A, which occupies two |char16|'s.
-    {"&\U0001D49C", 0, 2, "\U0001D49C", "MultibyteAccelerator_Start"},
-    {"Test&\U0001D49Cing", 4, 2, "Test\U0001D49Cing",
+    {"&\U0001D49C", 0, 2, "\U0001D49C", "\U0001D49C",
+     "MultibyteAccelerator_Start"},
+    {"Test&\U0001D49Cing", 4, 2, "Test\U0001D49Cing", "Test\U0001D49Cing",
      "MultibyteAccelerator_Middle"},
-    {"Test\U0001D49C&ing", 6, 1, "Test\U0001D49Cing",
+    {"Test\U0001D49C&ing", 6, 1, "Test\U0001D49Cing", "Test\U0001D49Cing",
      "OneAccelerator_AfterMultibyte"},
-    {"Test&\U0001D49C&ing", 6, 1, "Test\U0001D49Cing",
+    {"Test&\U0001D49C&ing", 6, 1, "Test\U0001D49Cing", "Test\U0001D49Cing",
      "MultibyteAccelerator_Skipped"},
-    {"Test&\U0001D49C&&ing", 4, 2, "Test\U0001D49C&ing",
+    {"Test&\U0001D49C&&ing", 4, 2, "Test\U0001D49C&ing", "Test\U0001D49C&ing",
      "MultibyteAccelerator_EscapeAfter"},
     {"Test&\U0001D49C&\U0001D49Cing", 6, 2, "Test\U0001D49C\U0001D49Cing",
+     "Test\U0001D49C\U0001D49Cing",
      "MultibyteAccelerator_AfterMultibyteAccelerator"},
+    {"accelerator(&\U0001D49C)", 12, 2, "accelerator(\U0001D49C)",
+     "accelerator", "MultibyteAccelerator_CJK"},
 };
 
 INSTANTIATE_TEST_SUITE_P(
@@ -169,10 +193,14 @@ TEST_P(RemoveAcceleratorCharTest, RemoveAcceleratorChar) {
   RemoveAcceleratorCharData data = GetParam();
   int accelerated_char_pos;
   int accelerated_char_span;
-  base::string16 result =
-      RemoveAcceleratorChar(base::UTF8ToUTF16(data.input), kAcceleratorChar,
-                            &accelerated_char_pos, &accelerated_char_span);
-  EXPECT_EQ(result, base::UTF8ToUTF16(data.output));
+  base::string16 result_locate_and_strip = LocateAndRemoveAcceleratorChar(
+      base::UTF8ToUTF16(data.input), &accelerated_char_pos,
+      &accelerated_char_span);
+  base::string16 result_full_strip =
+      RemoveAccelerator(base::UTF8ToUTF16(data.input));
+  EXPECT_EQ(result_locate_and_strip,
+            base::UTF8ToUTF16(data.output_locate_and_strip));
+  EXPECT_EQ(result_full_strip, base::UTF8ToUTF16(data.output_full_strip));
   EXPECT_EQ(accelerated_char_pos, data.accelerated_char_pos);
   EXPECT_EQ(accelerated_char_span, data.accelerated_char_span);
 }

ui/views/touchui/touch_selection_menu_views.cc

@@ -147,8 +147,7 @@ void TouchSelectionMenuViews::CreateButtons() {
 LabelButton* TouchSelectionMenuViews::CreateButton(
     const base::string16& title,
     Button::PressedCallback callback) {
-  base::string16 label =
-      gfx::RemoveAcceleratorChar(title, '&', nullptr, nullptr);
+  base::string16 label = gfx::RemoveAccelerator(title);
   auto* button = AddChildView(std::make_unique<LabelButton>(
       std::move(callback), label, style::CONTEXT_TOUCH_MENU));
   constexpr gfx::Size kMenuButtonMinSize = gfx::Size(63, 38);