views::Textfield: Implement transpose editing command.

views::Textfield: Implement transpose editing command.

This CL implements the transpose editing command for views::Textfield. It is
only enabled on MacViews.

TEST=Enable MacViews. Give focus to a MacViews textfield. Verify pressing Ctrl+T
transposes the text around the insertion point.

BUG=586985
Committed: https://crrev.com/30f577b8cd883fbdf8c278a5534ff578e0ab8f1a
Cr-Commit-Position: refs/heads/master@{#403086}

[karandeepb, 2016-06-28 01:37:21]

Patchset #1 (id:1) has been deleted

[karandeepb, 2016-06-28 01:37:30]

Patchset #1 (id:20001) has been deleted

[karandeepb, 2016-06-28 01:40:55]

Description was changed from

==========
Transpose.
==========

to

==========
views::Textfield: Implement transpose editing command.

This CL implements the transpose editing command for views::Textfield. It is 
only enabled on MacViews.

TEST=Enable MacViews. Give focus to a MacViews textfield. Verify pressing Ctrl+T
transposes the text around the insertion point.

BUG=586985
==========

[karandeepb, 2016-06-28 03:26:26]

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

[karandeepb, 2016-06-28 03:26:27]

PTAL Trent. Thanks.

https://codereview.chromium.org/1999773002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1999773002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:985: domCode:ui::DomCode::US_T
I am not sure if giving it the Ctrl+T keycode is appropriate here. I believe the
intention behind also providing Window/Linux specific key-codes in addition to
setting the command, is to take care of commands like PageUp, which are
currently handled using OnKeyPressed. But Ctrl+T on Windows/Linux doesn't mean
transpose. Maybe we should give a dummy keycode like ui::VKEY_UNKOWN?

[tapted, 2016-06-28 04:52:32]

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model.cc (right):

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:539: // If we are at the end of
the line, last two characters should be transposed.
nit: "If we are at .. line, last" -> "At .. line, the last"

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:546: // This happens when we are
at the beginning of the line or the line has less
nit: remove "when we are", add `when` in "..line or when the line.."

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:554: text.substr(cur - prev) +
text.substr(0, cur - prev);
Should we add some emoji/surrogate pairs to
BridgedNativeWidgetTest::TestEditingCommands ?

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:558: }
do you need to do anything special to get `undo` to work? Maybe that's another
good thing to include in BridgedNativeWidgetTest::TestEditingCommands -- i.e.
after PerformCommand, PerformCommand(@selector(undo:)) and see if they text and
selection matches.  (I suspect the selection currently will not..)

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

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1924: // Non-Mac platforms
don't have a key binding for Transpose.
Can we put this in textfield_model_unittest instead? (and run it on all
platforms?)

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1933: const base::string16
surrogate_pairs({0xD83D, 0xDE07, 'a', 0xD83D, 0xDE0E});
This is probably invoking std::string's initializer list constructor - I ..
think that's OK now. But it's a fairly recent thing. 

An alternative might be to use base::WideToUTF16 and a full string rather than
an intiializer list

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1940: } cases[][4] = {
I don't think it's correct to constrain this to 4 test cases per test string --
feels a bit shoe-horned to fit this array declaration. E.g. you test non-zero
selection ranges for rtl/non-rtl but not for emoji. But since we have
initializer lists we can probably do something like

struct TestCase {.. };

std::vector<TestCase> ltr_tests = {
  {gfx::Range(0), ltr, gfx::Range(0)},
  ..,
}

...
std::vector<std::vector<TestCase>> all_tests = {ltr_tests, rtl_tests,
emoji_tests};

etc.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1948: {gfx::Range(0), rtl,
gfx::Range(0)},
is this what git-cl-format did? it might be a clang-format bug. Giving the test
cases a temporary is probably nicer anyway -- "intrinsic documentation" rather
than having to add the "Cases for foo" comments

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1970:
base::UTF16ToUTF8(test_strings[i]).c_str()));
can you use

SCOPED_TRACE(testing::Message() << "Testing"... << i << ", " << j << " ..)

printf feels a bit clumsy here (Also it might be tempting fate to output emoji
in the log output..)

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1971: auto test_case =
cases[i][j];
nit: const auto&, (or const TextCase&)

[karandeepb, 2016-06-29 05:44:38]

PTAL Trent.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model.cc (right):

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:539: // If we are at the end of
the line, last two characters should be transposed.
On 2016/06/28 04:52:32, tapted wrote:
> nit: "If we are at .. line, last" -> "At .. line, the last"

Done.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:546: // This happens when we are
at the beginning of the line or the line has less
On 2016/06/28 04:52:32, tapted wrote:
> nit: remove "when we are", add `when` in "..line or when the line.."

Done.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:554: text.substr(cur - prev) +
text.substr(0, cur - prev);
On 2016/06/28 04:52:32, tapted wrote:
> Should we add some emoji/surrogate pairs to
> BridgedNativeWidgetTest::TestEditingCommands ?

Tried this but there are a few minor differences between how Views and
NSTextView handle emojis. For example for a string like 😊😎 NSTextView doesn't
transpose if we are at the end of the string. (probably an NSTextView bug). 
If at the string beginning and we do Alt+right (moveWordRight:),
Views::Textfield moves past both emojis while NSTextView moves past only the
first emoji.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:558: }
On 2016/06/28 04:52:32, tapted wrote:
> do you need to do anything special to get `undo` to work? Maybe that's another
> good thing to include in BridgedNativeWidgetTest::TestEditingCommands -- i.e.
> after PerformCommand, PerformCommand(@selector(undo:)) and see if they text
and
> selection matches.  (I suspect the selection currently will not..)

Undo behavior is not completely correct currently, not just for MacViews but
also on other platforms. I had filed a bug for this - http://crbug.com/615345
and plan to take it on later.

Regarding adding undo verification to
BridgedNativeWidgetTest::TestEditingCommands, it seems that action messages
performed using doCommandBySelector are not added to the undo stack for the
NSTextView. Will have to investigate how to get undo to work for these
programmatic NSTextView modifications, but that can probably wait till the undo
fix.

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

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1924: // Non-Mac platforms
don't have a key binding for Transpose.
On 2016/06/28 04:52:32, tapted wrote:
> Can we put this in textfield_model_unittest instead? (and run it on all
> platforms?)

Done.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1933: const base::string16
surrogate_pairs({0xD83D, 0xDE07, 'a', 0xD83D, 0xDE0E});
On 2016/06/28 04:52:32, tapted wrote:
> This is probably invoking std::string's initializer list constructor - I ..
> think that's OK now. But it's a fairly recent thing. 
> 
> An alternative might be to use base::WideToUTF16 and a full string rather than
> an intiializer list

Had tried that. It won't work since wstring is platform dependent. For example
on Mac, wchar_t is 32 bits, on Windows 16 bits. Hence the wstring representation
will be different on different platforms.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1940: } cases[][4] = {
On 2016/06/28 04:52:32, tapted wrote:
> I don't think it's correct to constrain this to 4 test cases per test string
--
> feels a bit shoe-horned to fit this array declaration. E.g. you test non-zero
> selection ranges for rtl/non-rtl but not for emoji. But since we have
> initializer lists we can probably do something like
> 
> struct TestCase {.. };
> 
> std::vector<TestCase> ltr_tests = {
>   {gfx::Range(0), ltr, gfx::Range(0)},
>   ..,
> }
> 
> ...
> std::vector<std::vector<TestCase>> all_tests = {ltr_tests, rtl_tests,
> emoji_tests};
> 
> etc.

Done.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1948: {gfx::Range(0), rtl,
gfx::Range(0)},
On 2016/06/28 04:52:32, tapted wrote:
> is this what git-cl-format did? it might be a clang-format bug. Giving the
test
> cases a temporary is probably nicer anyway -- "intrinsic documentation" rather
> than having to add the "Cases for foo" comments

Done. Yeah git cl format did this.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1970:
base::UTF16ToUTF8(test_strings[i]).c_str()));
On 2016/06/28 04:52:32, tapted wrote:
> can you use
> 
> SCOPED_TRACE(testing::Message() << "Testing"... << i << ", " << j << " ..)
> 
> printf feels a bit clumsy here (Also it might be tempting fate to output emoji
> in the log output..)

Done.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1971: auto test_case =
cases[i][j];
On 2016/06/28 04:52:32, tapted wrote:
> nit: const auto&, (or const TextCase&)

Done.

[tapted, 2016-06-29 07:06:15]

lgtm

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield_model.cc (right):

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:554: text.substr(cur - prev) +
text.substr(0, cur - prev);
On 2016/06/29 05:44:37, karandeepb wrote:
> On 2016/06/28 04:52:32, tapted wrote:
> > Should we add some emoji/surrogate pairs to
> > BridgedNativeWidgetTest::TestEditingCommands ?
> 
> Tried this but there are a few minor differences between how Views and
> NSTextView handle emojis. For example for a string like 😊😎 NSTextView doesn't
> transpose if we are at the end of the string. (probably an NSTextView bug). 
> If at the string beginning and we do Alt+right (moveWordRight:),
> Views::Textfield moves past both emojis while NSTextView moves past only the
> first emoji.

Acknowledged.

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_model.cc:558: }
On 2016/06/29 05:44:37, karandeepb wrote:
> On 2016/06/28 04:52:32, tapted wrote:
> > do you need to do anything special to get `undo` to work? Maybe that's
another
> > good thing to include in BridgedNativeWidgetTest::TestEditingCommands --
i.e.
> > after PerformCommand, PerformCommand(@selector(undo:)) and see if they text
> and
> > selection matches.  (I suspect the selection currently will not..)
> 
> Undo behavior is not completely correct currently, not just for MacViews but
> also on other platforms. I had filed a bug for this - http://crbug.com/615345
> and plan to take it on later.
> 
> Regarding adding undo verification to
> BridgedNativeWidgetTest::TestEditingCommands, it seems that action messages
> performed using doCommandBySelector are not added to the undo stack for the
> NSTextView. Will have to investigate how to get undo to work for these
> programmatic NSTextView modifications, but that can probably wait till the
undo
> fix.

Acknowledged.

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

https://codereview.chromium.org/1999773002/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield_unittest.cc:1933: const base::string16
surrogate_pairs({0xD83D, 0xDE07, 'a', 0xD83D, 0xDE0E});
On 2016/06/29 05:44:38, karandeepb wrote:
> On 2016/06/28 04:52:32, tapted wrote:
> > This is probably invoking std::string's initializer list constructor - I ..
> > think that's OK now. But it's a fairly recent thing. 
> > 
> > An alternative might be to use base::WideToUTF16 and a full string rather
than
> > an intiializer list
> 
> Had tried that. It won't work since wstring is platform dependent. For example
> on Mac, wchar_t is 32 bits, on Windows 16 bits. Hence the wstring
representation
> will be different on different platforms.

yah.. I think there's some macro that can help, but it looks good with
initializer lists - might as well use these language features now that we
finally can.

https://codereview.chromium.org/1999773002/diff/80001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/1999773002/diff/80001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:368: [[NSTextView alloc]
initWithFrame:NSMakeRect(0, 0, 100, 100)]);
probs need to rebase this bit

[karandeepb, 2016-06-29 07:33:45]

https://codereview.chromium.org/1999773002/diff/80001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/1999773002/diff/80001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:368: [[NSTextView alloc]
initWithFrame:NSMakeRect(0, 0, 100, 100)]);
On 2016/06/29 07:06:14, tapted wrote:
> probs need to rebase this bit

Done.

[karandeepb, 2016-06-29 07:36:12]

karandeepb@chromium.org changed reviewers:
+ msw@chromium.org, sky@chromium.org

[karandeepb, 2016-06-29 07:36:12]

+msw for ui/views/controls/textfield review.
+sky for ui/base review.

@msw: Is the transpose command needed on any Non-Mac platform?

[sky, 2016-06-29 15:21:42]

LGTM

[msw, 2016-06-29 17:16:45]

nice model test; lgtm with a minor nit.

https://codereview.chromium.org/1999773002/diff/100001/ui/views/controls/text...
File ui/views/controls/textfield/textfield_model_unittest.cc (right):

https://codereview.chromium.org/1999773002/diff/100001/ui/views/controls/text...
ui/views/controls/textfield/textfield_model_unittest.cc:1621:
std::vector<TestCase> emoji_tests = {
nit: naming consistency between surrogate_pairs and emoji.

[karandeepb, 2016-06-30 03:03:39]

https://codereview.chromium.org/1999773002/diff/100001/ui/views/controls/text...
File ui/views/controls/textfield/textfield_model_unittest.cc (right):

https://codereview.chromium.org/1999773002/diff/100001/ui/views/controls/text...
ui/views/controls/textfield/textfield_model_unittest.cc:1621:
std::vector<TestCase> emoji_tests = {
On 2016/06/29 17:16:45, msw wrote:
> nit: naming consistency between surrogate_pairs and emoji.

Done.

[karandeepb, 2016-06-30 03:03:49]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2016-06-30 03:03:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sky@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1999773002/#ps120001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-06-30 03:04:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 04:04:48]

Description was changed from

==========
views::Textfield: Implement transpose editing command.

This CL implements the transpose editing command for views::Textfield. It is 
only enabled on MacViews.

TEST=Enable MacViews. Give focus to a MacViews textfield. Verify pressing Ctrl+T
transposes the text around the insertion point.

BUG=586985
==========

to

==========
views::Textfield: Implement transpose editing command.

This CL implements the transpose editing command for views::Textfield. It is 
only enabled on MacViews.

TEST=Enable MacViews. Give focus to a MacViews textfield. Verify pressing Ctrl+T
transposes the text around the insertion point.

BUG=586985
==========

[commit-bot: I haz the power, 2016-06-30 04:04:49]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-06-30 04:04:53]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 04:06:52]

Description was changed from

==========
views::Textfield: Implement transpose editing command.

This CL implements the transpose editing command for views::Textfield. It is 
only enabled on MacViews.

TEST=Enable MacViews. Give focus to a MacViews textfield. Verify pressing Ctrl+T
transposes the text around the insertion point.

BUG=586985
==========

to

==========
views::Textfield: Implement transpose editing command.

This CL implements the transpose editing command for views::Textfield. It is
only enabled on MacViews.

TEST=Enable MacViews. Give focus to a MacViews textfield. Verify pressing Ctrl+T
transposes the text around the insertion point.

BUG=586985

Committed: https://crrev.com/30f577b8cd883fbdf8c278a5534ff578e0ab8f1a
Cr-Commit-Position: refs/heads/master@{#403086}
==========

[commit-bot: I haz the power, 2016-06-30 04:06:54]

Patchset 5 (id:??) landed as
https://crrev.com/30f577b8cd883fbdf8c278a5534ff578e0ab8f1a
Cr-Commit-Position: refs/heads/master@{#403086}