MacViews: Convert Cocoa action messages into editing commands for text fields

MacViews: Convert Cocoa action messages into editing commands for text fields

On Mac, events need to be mapped to "Action" messages by an NSResponder
to obey platform behaviour and user customizations (e.g. a "Home"
keypress should be interpreted as beginning of document, not beginning
of line). Action messages are also the manner by which a user on Mac can
add custom keybindings, by making entries in
~/Library/KeyBindings/DefaultKeyBinding.dict

ActionMessages map well to editing commands for text fields, but not as
well for other controls. For example, MenuController needs to catch the
action message "cancel:" to handle the escape key.

The approach in this CL is for BridgedContentView, an NSResponder, to
map Action Messages to the editing command as well as the KeyCode that
toolkit-views expects for that action.

To test, textfield_unittests are updated to allow two kinds of event
dispatch. Before this CL, all the TextfieldTest views_unittests were
already passing on a toolkit-views Mac build. This is because the tests
send KeyCodes directly to the InputMethod rather than to the window.
However, this meant that action messages didn't come into play.

To test "real" keystrokes on Mac and get good coverage of the action
message overloads, the ui::test::EventGenerator is now used in
textfield_unittest.cc to generate platform-specific events that are
dispatched to the Window. Previously, all keyboard events were
redirected to the InputMethod.

BUG=378134
Committed: https://crrev.com/4a95d096d7832d7f10c174311723d529cfc35928
Cr-Commit-Position: refs/heads/master@{#314268}

[tapted, 2015-01-28 11:30:18]

tapted@chromium.org changed reviewers:
+ andresantoso@chromium.org

[tapted, 2015-01-28 11:38:25]

tapted@chromium.org changed reviewers:
+ oshima@chromium.org

[tapted, 2015-01-28 11:38:25]

oshima: please take a look at textfield_unittest.cc - there are some structural
changes. See what you think of those and the idea in general.
andre: PTAL for general review this follows on from http://crrev.com/809773006

Note this CL is based on a "boring" function-move CL in
http://crrev.com/882053002 . I want to make sure the approach here is good
before sending that out.

tryjobs for this CL are in http://crrev.com/882043003 due to the upstream CL.

for additional context, follow-up to this CL is http://crrev.com/866983006 which
(along with this CL) gets the views Menu* interactive_ui_tests passing

Thanks!

[Andre, 2015-01-28 21:49:09]

LGTM.

One question, don't we need to forward key up events to the Widget
as well?

[tapted, 2015-01-28 23:54:52]

tapted@chromium.org changed reviewers:
+ sadrul@chromium.org

[tapted, 2015-01-28 23:54:52]

+sadrul, since I will need a ui/ owner and there's some crazy event stuff here
:). Please take a look.

On 2015/01/28 21:49:09, Andre wrote:
> One question, don't we need to forward key up events to the Widget
> as well?

I poked around a bit. I don't think this changes much in this regard, or not in
a way that matters for views. For example, Menu accelerators are processed
before getting to the view, so Cmd+W still works to close the window inside a
textfield. Forwarding to the views::Widget is still done in handleAction if the
key combination isn't an editing command, or if there is no text input client.
Currently the native event details are lost for action messages - I don't know
if that will be a problem yet, so didn't try to do anything complicated (if it
does trip something up we can do something like [ChromeEventProcessingWindow
keyEventForWindow:] to create a native event, or use [NSApp currentEvent] and
modify keycodes on the ui::Event after the EventFromNative stuff).

[tapted, 2015-01-29 20:42:31]

oshima, sadrul: ping?

[oshima, 2015-01-29 23:07:04]

hi, sorry for delay. I'll review it later today.

[sadrul, 2015-01-30 15:46:40]

I am not really familiar with cocoa. So there isn't a lot I can actually review
in this code. But I feel like there could be code (e.g. IME, perhaps?) that
would break if you are sending a KEY_PRESSED event without a corresponding
KEY_RELEASED event. So I would add code to dispatch a KEY_RELEASED event unless
it breaks something. Other than that, rubberstamp lgtm

[oshima, 2015-01-30 17:44:17]

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield_unittest.cc:418:
scoped_ptr<ui::test::EventGenerator> event_generator_;
I believe the test was updated to use MockInputMethod before event generator was
introduced. I wonder if using event generator is good enough. Can we eliminate
MockInputMethod path?

[tapted, 2015-02-02 02:56:03]

Patchset #2 (id:20001) has been deleted

[tapted, 2015-02-02 02:57:04]

Patchset #2 (id:40001) has been deleted

[tapted, 2015-02-02 02:57:11]

Patchset #2 (id:60001) has been deleted

[tapted, 2015-02-02 05:38:11]

Patchset #3 (id:100001) has been deleted

[tapted, 2015-02-02 06:51:04]

Patchset #4 (id:140001) has been deleted

[tapted, 2015-02-02 06:54:46]

Patchset #5 (id:180001) has been deleted

[tapted, 2015-02-02 06:54:53]

Patchset #5 (id:200001) has been deleted

[tapted, 2015-02-02 11:55:05]

Patchset #4 (id:160001) has been deleted

[tapted, 2015-02-02 11:55:17]

Patchset #4 (id:220001) has been deleted

[tapted, 2015-02-02 12:24:02]

Patchset #4 (id:240001) has been deleted

[tapted, 2015-02-02 12:46:37]

oshima, PTAL.

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield_unittest.cc:418:
scoped_ptr<ui::test::EventGenerator> event_generator_;
On 2015/01/30 17:44:17, oshima wrote:
> I believe the test was updated to use MockInputMethod before event generator
was
> introduced. I wonder if using event generator is good enough. Can we eliminate
> MockInputMethod path?
> 

Done.

Just a couple of surprises. And of course some test regressions on MacViews
running views_unittests (filed http://crbug.com/454353 to track), but I'd need
to fix them anyway.

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield_unittest.cc:1546: SendKeyEvent('\n');
These were using the SendKeyEvent(base::char16) overload, which only works for
space and letters. For '\n' it does `ui::KeyboardCode code = '\n' + 'a', which
isn't right. The method of clearing isn't interesting for these tests, so I've
just done SetText("");

https://codereview.chromium.org/879253002/diff/280001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/879253002/diff/280001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:305: ui::KeyEvent event(ch,
ui::VKEY_UNKNOWN, ui::EF_NONE);
I removed the TODO (and added a new comment), since I think this is the sensible
way to handle these in the test. NativeWidgetAura::OnKeyEvent() actually has
logic to ignore character events created this way (so "regular" event dispatch
with the EventGenerator doesn't work).

[oshima, 2015-02-02 17:59:13]

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield_unittest.cc:1546: SendKeyEvent('\n');
On 2015/02/02 12:46:37, tapted wrote:
> These were using the SendKeyEvent(base::char16) overload, which only works for
> space and letters. For '\n' it does `ui::KeyboardCode code = '\n' + 'a', which
> isn't right. The method of clearing isn't interesting for these tests, so I've
> just done SetText("");

Can you send VKEY_DELETE instead?

https://codereview.chromium.org/879253002/diff/280001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/879253002/diff/280001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:305: ui::KeyEvent event(ch,
ui::VKEY_UNKNOWN, ui::EF_NONE);
On 2015/02/02 12:46:37, tapted wrote:
> I removed the TODO (and added a new comment), since I think this is the
sensible
> way to handle these in the test. NativeWidgetAura::OnKeyEvent() actually has
> logic to ignore character events created this way (so "regular" event dispatch
> with the EventGenerator doesn't work).

Acknowledged.

[tapted, 2015-02-02 22:26:44]

On 2015/01/30 15:46:40, sadrul wrote:
> But I feel like there could be code (e.g. IME, perhaps?) that
> would break if you are sending a KEY_PRESSED event without a corresponding
> KEY_RELEASED event. So I would add code to dispatch a KEY_RELEASED event
unless
> it breaks something.

(forgot to comment on this). This is still on the agenda :). IME on Mac is
handled a bit earlier, using the NSTextInputClient protocol implemented further
down in BridgedContentView. But it's still a good point. For keystrokes, I'm
wary of generating the KEY_RELEASED when the action message is processed, since
there is a subsequent `keyUp:` still to come. Currently BridgedContentView
intercepts keyDown: and converts them to action messages (as suggested by the
discussion in Apple's documentation for -[NSResponder keyDown:]). However,
keyUp: is not currently overridden. I want to intercept keyUp: as well, but I
want to do that with a use-case in mind (e.g. a failing test that gets fixed, or
a new one to cover it), and I haven't found the right one yet.

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield_unittest.cc (right):

https://codereview.chromium.org/879253002/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield_unittest.cc:1546: SendKeyEvent('\n');
On 2015/02/02 17:59:13, oshima wrote:
> On 2015/02/02 12:46:37, tapted wrote:
> > These were using the SendKeyEvent(base::char16) overload, which only works
for
> > space and letters. For '\n' it does `ui::KeyboardCode code = '\n' + 'a',
which
> > isn't right. The method of clearing isn't interesting for these tests, so
I've
> > just done SetText("");
> 
> Can you send VKEY_DELETE instead?

Done.

[oshima, 2015-02-02 22:35:50]

lgtm

[tapted, 2015-02-03 04:46:49]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-02-03 04:47:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/879253002/300001

[commit-bot: I haz the power, 2015-02-03 04:51:38]

Committed patchset #6 (id:300001)

[commit-bot: I haz the power, 2015-02-03 04:52:33]

Patchset 6 (id:??) landed as
https://crrev.com/4a95d096d7832d7f10c174311723d529cfc35928
Cr-Commit-Position: refs/heads/master@{#314268}