MacViews: Fix Chinese IME dismissal on arrow key bug

The Chinese Pinyin IME on Mac uses arrow keys to select candidate words.
Because -[NSView interpretKeyEvents:] silently consumes the arrow key
without notifying NSTextInputClient, currently MacViews textfield will
handle arrow keys as accelerators and dismiss the IME even in the
presence of composition text.

This CL fixes this bug by omitting arrow keys as accelerators when the
composition text exists. This bug has the same root cause as seen in
crbug/883952

Bug: 915924
Change-Id: I408fbb637e635c2e048a83dfbc1847820019827e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464078
Commit-Queue: Keren Zhu <kerenzhu@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816073}

components/remote_cocoa/app_shim/bridged_content_view.mm

@@ -58,12 +58,30 @@ gfx::Point MovePointToWindow(const NSPoint& point,
                     NSHeight(content_rect) - point_in_window.y);
 }
 
-// Returns true if |event| may have triggered dismissal of an IME and would
-// otherwise be ignored by a ui::TextInputClient when inserted.
-bool IsImeTriggerEvent(NSEvent* event) {
+// Some keys are silently consumed by -[NSView interpretKeyEvents:]
+// They should not be processed as accelerators.
+// See comments at |keyDown:| for details.
+bool ShouldIgnoreAcceleratorWithMarkedText(NSEvent* event) {
   ui::KeyboardCode key = ui::KeyboardCodeFromNSEvent(event);
-  return key == ui::VKEY_RETURN || key == ui::VKEY_TAB ||
-         key == ui::VKEY_ESCAPE;
+  switch (key) {
+    // crbug/883952: Kanji IME completes composition and dismisses itself.
+    case ui::VKEY_RETURN:
+    // Kanji IME: select candidate words.
+    // Pinyin IME: change tone.
+    case ui::VKEY_TAB:
+    // Dismiss IME.
+    case ui::VKEY_ESCAPE:
+    // crbug/915924: Pinyin IME selects candidate.
+    case ui::VKEY_LEFT:
+    case ui::VKEY_RIGHT:
+    case ui::VKEY_UP:
+    case ui::VKEY_DOWN:
+    case ui::VKEY_PRIOR:
+    case ui::VKEY_NEXT:
+      return true;
+    default:
+      return false;
+  }
 }
 
 ui::TextEditCommand GetTextEditCommandForMenuAction(SEL action) {
@@ -709,14 +727,14 @@ ui::TextEditCommand GetTextEditCommandForMenuAction(SEL action) {
   [self interpretKeyEvents:@[ theEvent ]];
 
   // When there is marked text, -[NSView interpretKeyEvents:] may handle the
-  // event by dismissing the IME window in a way that neither unmarks text, nor
-  // updates any composition. That is, no signal is given either to the
-  // NSTextInputClient or the NSTextInputContext that the IME changed state.
-  // However, we must ensure this key down is not processed as an accelerator.
-  // TODO(tapted): Investigate removing the IsImeTriggerEvent() check - it's
-  // probably not required, but helps tests that expect some events to always
-  // get processed (i.e. TextfieldTest.TextInputClientTest).
-  if (hadMarkedTextAtKeyDown && IsImeTriggerEvent(theEvent))
+  // event by updating the IME state without updating the composition text.
+  // That is, no signal is given either to the NSTextInputClient or the
+  // NSTextInputContext, leaving |hasUnhandledKeyDownEvent_| to be true.
+  // In such a case, the key down event should not processed as an accelerator.
+  // TODO(kerenzhu): Note it may be valid to always mark the key down event as
+  // handled by IME when there is marked text. For now, only certain keys are
+  // skipped.
+  if (hadMarkedTextAtKeyDown && ShouldIgnoreAcceleratorWithMarkedText(theEvent))
     _hasUnhandledKeyDownEvent = NO;
 
   // Even with marked text, some IMEs may follow with -insertNewLine:;

ui/views/cocoa/bridged_native_widget_unittest.mm

@@ -1599,6 +1599,92 @@ TEST_F(BridgedNativeWidgetTest, TextInput_SimulateTelexMoo) {
   EXPECT_EQ(1, enter_view->count());  // Now we see the accelerator.
 }
 
+// Simulate 'a' and candidate selection keys. This should just insert "啊",
+// suppressing accelerators.
+TEST_F(BridgedNativeWidgetTest, TextInput_NoAcceleratorPinyinSelectWord) {
+  Textfield* textfield = InstallTextField("");
+  EXPECT_TRUE([ns_view_ textInputClient]);
+
+  EnterAcceleratorView* enter_view = new EnterAcceleratorView();
+  textfield->parent()->AddChildView(enter_view);
+
+  // Sequence of calls (and corresponding keyDown events) obtained via tracing
+  // with Pinyin IME and pressing 'a', Tab, PageDown, PageUp, Right, Down, Left,
+  // and finally Up on the keyboard.
+  // Note 0 is the actual keyCode for 'a', not a placeholder.
+  NSEvent* a_in_ime = UnicodeKeyDown(0, @"a");
+  InterpretKeyEventsCallback handle_a_in_ime = base::BindRepeating([](id view) {
+    // Pinyin does not change composition text while selecting candidate words.
+    [view setMarkedText:@"a"
+           selectedRange:NSMakeRange(1, 0)
+        replacementRange:NSMakeRange(NSNotFound, 0)];
+  });
+
+  InterpretKeyEventsCallback handle_tab_in_ime =
+      base::BindRepeating([](id view) {
+        [view setMarkedText:@"ā"
+               selectedRange:NSMakeRange(0, 1)
+            replacementRange:NSMakeRange(NSNotFound, 0)];
+      });
+
+  // Composition text will not change in candidate selection.
+  InterpretKeyEventsCallback handle_candidate_select_in_ime =
+      base::BindRepeating([](id view) {});
+
+  InterpretKeyEventsCallback handle_space_in_ime =
+      base::BindRepeating([](id view) {
+        // Space will confirm the composition.
+        [view insertText:@"啊" replacementRange:NSMakeRange(NSNotFound, 0)];
+      });
+
+  InterpretKeyEventsCallback handle_enter_in_ime =
+      base::BindRepeating([](id view) {
+        // Space after Space will generate -insertNewLine:.
+        [view doCommandBySelector:@selector(insertNewLine:)];
+      });
+
+  EXPECT_EQ(base::UTF8ToUTF16(""), textfield->GetText());
+  EXPECT_EQ(0, enter_view->count());
+
+  object_setClass(ns_view_, [InterpretKeyEventMockedBridgedContentView class]);
+  g_fake_interpret_key_events = &handle_a_in_ime;
+  [ns_view_ keyDown:a_in_ime];
+  EXPECT_EQ(base::SysNSStringToUTF16(@"a"), textfield->GetText());
+  EXPECT_EQ(0, enter_view->count());
+
+  g_fake_interpret_key_events = &handle_tab_in_ime;
+  [ns_view_ keyDown:VkeyKeyDown(ui::VKEY_RETURN)];
+  EXPECT_EQ(base::SysNSStringToUTF16(@"ā"), textfield->GetText());
+  EXPECT_EQ(0, enter_view->count());  // Not seen as an accelerator.
+
+  // Pinyin changes candidate word on this sequence of keys without changing the
+  // composition text. At the end of this sequence, the word "啊" should be
+  // selected.
+  const ui::KeyboardCode key_seqence[] = {ui::VKEY_NEXT,  ui::VKEY_PRIOR,
+                                          ui::VKEY_RIGHT, ui::VKEY_DOWN,
+                                          ui::VKEY_LEFT,  ui::VKEY_UP};
+
+  g_fake_interpret_key_events = &handle_candidate_select_in_ime;
+  for (auto key : key_seqence) {
+    [ns_view_ keyDown:VkeyKeyDown(key)];
+    EXPECT_EQ(base::SysNSStringToUTF16(@"ā"),
+              textfield->GetText());  // No change.
+    EXPECT_EQ(0, enter_view->count());
+  }
+
+  // Space to confirm composition
+  g_fake_interpret_key_events = &handle_space_in_ime;
+  [ns_view_ keyDown:VkeyKeyDown(ui::VKEY_SPACE)];
+  EXPECT_EQ(base::SysNSStringToUTF16(@"啊"), textfield->GetText());
+  EXPECT_EQ(0, enter_view->count());
+
+  // The next Enter should be processed as accelerator.
+  g_fake_interpret_key_events = &handle_enter_in_ime;
+  [ns_view_ keyDown:VkeyKeyDown(ui::VKEY_RETURN)];
+  EXPECT_EQ(base::SysNSStringToUTF16(@"啊"), textfield->GetText());
+  EXPECT_EQ(1, enter_view->count());
+}
+
 // Simulate 'a', Enter in Hiragana. This should just insert "あ", suppressing
 // accelerators.
 TEST_F(BridgedNativeWidgetTest, TextInput_NoAcceleratorEnterComposition) {