MacViews: Fix accelerator handling while Omnibox is in focus.

ui/views/controls/textfield/textfield.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/textfield/textfield.h"
 
 #include <string>
 #include <utility>
 
 #include "base/strings/utf_string_conversions.h"
@@ skipping 74 matching lines @@
   View::ConvertPointToScreen(src, &new_origin);
   r->set_origin(new_origin);
 }
 
 // Get the default command for a given key |event|.
 ui::TextEditCommand GetCommandForKeyEvent(const ui::KeyEvent& event) {
   if (event.type() != ui::ET_KEY_PRESSED || event.IsUnicodeKeyCode())
     return ui::TextEditCommand::INVALID_COMMAND;
 
   const bool shift = event.IsShiftDown();
+#if defined(OS_MACOSX)
+  const bool control = event.IsCommandDown();

[themblsha, 2016/11/22 14:33:48]

Fixes TextfieldTest.CutCopyPaste. "Ensure [Ctrl]+[Shift]+[Insert] is a no-op"
triggered Paste because it looked at Control modifier on Mac and actually sent
Cmd+Shift+Insert.

[tapted, 2016/11/23 02:09:30]

This doesn't look right -- E.g. -[BridgedContentView copy:] uses
EF_CONTROL_DOWN, (but also an edit command so it won't end up here).

I'd suggest something like

const bool control = event.IsControlDown() || event.IsCommandDown();

(i.e. no #ifdef guard - do that on all platforms - IsCommandDown should only
come up on mac anyway)


It might also be possible that the test doesn't make sense on Mac, or should
really be dispatching the event differently (e.g. EventGenerator::WINDOW), since
in "real" code (outside of tests) this is handled elsewhere on Mac.

[themblsha, 2016/11/23 15:16:39]

Just EventGenerator::WINDOW is not enough, but in conjunction with this fix and
Textfield::GetAcceleratorForCommandId() it works fine.

+#else
   const bool control = event.IsControlDown();
+#endif
   const bool alt = event.IsAltDown() || event.IsAltGrDown();
   switch (event.key_code()) {
     case ui::VKEY_Z:
       if (control && !shift && !alt)
         return ui::TextEditCommand::UNDO;
       return (control && shift && !alt) ? ui::TextEditCommand::REDO
                                         : ui::TextEditCommand::INVALID_COMMAND;
     case ui::VKEY_Y:
       return (control && !alt) ? ui::TextEditCommand::REDO
                                : ui::TextEditCommand::INVALID_COMMAND;
@@ skipping 1151 matching lines @@
 
   OnBeforeUserAction();
   skip_input_method_cancel_composition_ = true;
   model_->InsertText(new_text);
   skip_input_method_cancel_composition_ = false;
   UpdateAfterChange(true, true);
   OnAfterUserAction();
 }
 
 void Textfield::InsertChar(const ui::KeyEvent& event) {
+#if defined(OS_MACOSX)
+  // MockInputMethod::DispatchKeyEvent() will try to incorrectly call
+  // TestTextfield::InsertChar(), even though when Command is pressed text input
+  // shouldn't work at all.
+  if (event.IsCommandDown())
+    return;
+#endif

[themblsha, 2016/11/22 14:33:48]

Fixes TextfieldTest.KeysWithModifiersTest. MockInputMethod dispatches Cmd+2
KeyDown event directly to InsertChar().

[tapted, 2016/11/23 02:09:30]

This test already behaves correctly on CrOS. I think the right fix for this is
to change ui/events/base_event_utils.cc and add a `kSystemKeyModifierMask` for
Mac that is just EF_COMMAND_DOWN  -- (Mac *shouldn't* include EF_ALT_DOWN for
the same reasons as described in the comment for IsSystemKeyModifier - i.e. Alt
is used to input extended characters on Mac).

[themblsha, 2016/11/23 15:16:39]

Whoa, this really fixes it. Thanks!

+
   if (read_only()) {
     OnEditFailed();
     return;
   }
 
   // Filter out all control characters, including tab and new line characters,
   // and all characters with Alt modifier (and Search on ChromeOS). But allow
   // characters with the AltGr modifier. On Windows AltGr is represented by
   // Alt+Ctrl or Right Alt, and on Linux it's a different flag that we don't
   // care about.
@@ skipping 770 matching lines @@
 }
 
 void Textfield::OnCursorBlinkTimerFired() {
   DCHECK(ShouldBlinkCursor());
   gfx::RenderText* render_text = GetRenderText();
   render_text->set_cursor_visible(!render_text->cursor_visible());
   RepaintCursor();
 }
 
 }  // namespace views