Make Views Textfield key handling execute commands.

Make Views Textfield key handling execute commands.

Add commands similar to WebKit text editing commands.
Translate key events to commands for textfield execution.
Consolidate key handling and command execution code.
Fix unit tests.

Modify Windows behavior for CTRL+SHIFT+[BACK|DEL].
(now behaves the same as CTRL+[BACK|DEL], not no-op)

BUG=319437
TEST=No behavior changes except for CTRL+SHIFT+[BACK|DEL] on Windows.
R=oshima@chromium.org, erg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260310

[Elliot Glaysher, 2014-03-28 22:47:49]

https://codereview.chromium.org/211593006/diff/40001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/211593006/diff/40001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:976: case
IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
So, if we're going to have versions for the events that are
_AND_MODIFY_SELECTION, I'll need to modify my approach to not just setting a
bool for the "modify selection" part.

(This list will also need unification with the uncomitted
ui/wm/public/text_edit_command_x11.cc.)

Also, at what level is it safe to use these IDS_* commands? Shouldn't these all
be IDC? (I thought IDS was for "S"tring while IDC was for "C"ommand.) Would I be
able to use the IDC commands up one level at ui/wm/?

[msw, 2014-03-28 22:55:29]

https://codereview.chromium.org/211593006/diff/40001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/211593006/diff/40001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:976: case
IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
On 2014/03/28 22:47:49, Elliot Glaysher wrote:
> So, if we're going to have versions for the events that are
> _AND_MODIFY_SELECTION, I'll need to modify my approach to not just setting a
> bool for the "modify selection" part.

The separate _AND_MODIFY_SELECTION commands come from following the conventions
of third_party/WebKit/Source/core/editing/EditorCommand.cpp.

> (This list will also need unification with the uncomitted
> ui/wm/public/text_edit_command_x11.cc.)

That consolidation can come later, views and web content textfield event/command
handling won't overlap at all, at least for now.

> Also, at what level is it safe to use these IDS_* commands? Shouldn't these
all
> be IDC? (I thought IDS was for "S"tring while IDC was for "C"ommand.) Would I
be
> able to use the IDC commands up one level at ui/wm/?

IDC_* is defined at the chrome level, whereas these are needed at the ui level.
Using IDS_* instead has been the convention for IDS_APP_CUT/COPY/PASTE, etc. and
seems appropriate as a medium-term solution; these same IDS_* should be
available to ui/wm/.

[oshima, 2014-03-28 23:14:45]

On 2014/03/28 22:55:29, msw wrote:
>
https://codereview.chromium.org/211593006/diff/40001/ui/views/controls/textfi...
> File ui/views/controls/textfield/textfield.cc (right):
> 
>
https://codereview.chromium.org/211593006/diff/40001/ui/views/controls/textfi...
> ui/views/controls/textfield/textfield.cc:976: case
> IDS_MOVE_LEFT_AND_MODIFY_SELECTION:
> On 2014/03/28 22:47:49, Elliot Glaysher wrote:
> > So, if we're going to have versions for the events that are
> > _AND_MODIFY_SELECTION, I'll need to modify my approach to not just setting a
> > bool for the "modify selection" part.
> 
> The separate _AND_MODIFY_SELECTION commands come from following the
conventions
> of third_party/WebKit/Source/core/editing/EditorCommand.cpp.
> 
> > (This list will also need unification with the uncomitted
> > ui/wm/public/text_edit_command_x11.cc.)
> 
> That consolidation can come later, views and web content textfield
event/command
> handling won't overlap at all, at least for now.
> 
> > Also, at what level is it safe to use these IDS_* commands? Shouldn't these
> all
> > be IDC? (I thought IDS was for "S"tring while IDC was for "C"ommand.) Would
I
> be
> > able to use the IDC commands up one level at ui/wm/?
> 
> IDC_* is defined at the chrome level, whereas these are needed at the ui
level.
> Using IDS_* instead has been the convention for IDS_APP_CUT/COPY/PASTE, etc.
and
> seems appropriate as a medium-term solution; these same IDS_* should be
> available to ui/wm/.

Yeah, this has been a known issue for long time. I think we need to fix soon
(but not now)
as this becomes essential part of chrome/views/textfield.

[oshima, 2014-03-28 23:23:26]

https://codereview.chromium.org/211593006/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/211593006/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:60: return 0;
can you define a cont value for 0 (like NO_COMMAND, IGNORED)

https://codereview.chromium.org/211593006/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:502: return true;
Is this correct? This method used to return false if text/curosr hasn't changed.

[msw, 2014-03-28 23:36:11]

Please take another look; thanks.

https://codereview.chromium.org/211593006/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/211593006/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:60: return 0;
On 2014/03/28 23:23:27, oshima wrote:
> can you define a cont value for 0 (like NO_COMMAND, IGNORED)
> 

Done.

https://codereview.chromium.org/211593006/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:502: return true;
On 2014/03/28 23:23:27, oshima wrote:
> Is this correct? This method used to return false if text/curosr hasn't
changed.

I think this is actually a better approach; if a command was valid and executed,
the key event should be considered handled. For example, if the backspace key
doesn't do anything because there's no preceding text, the event should not
generally bubble up to the next potential handler.

[oshima, 2014-03-28 23:53:25]

https://codereview.chromium.org/211593006/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/211593006/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:502: if (command != kNoCommand) {
discussed offline and lgtm if this is changed to use IsCommandIdEnabled

[msw, 2014-03-28 23:56:27]

Done; landing manually; prior try jobs look good.

https://codereview.chromium.org/211593006/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/211593006/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:502: if (command != kNoCommand) {
On 2014/03/28 23:53:26, oshima wrote:
> discussed offline and lgtm if this is changed to use IsCommandIdEnabled

Done.

[msw, 2014-03-29 00:09:04]

Committed patchset #5 manually as r260310 (presubmit successful).