MacViews: Fix behavior of move and select commands when selection direction changes.

ui/views/cocoa/bridged_native_widget_unittest.mm

Index: ui/views/cocoa/bridged_native_widget_unittest.mm
diff --git a/ui/views/cocoa/bridged_native_widget_unittest.mm b/ui/views/cocoa/bridged_native_widget_unittest.mm
index 0db6aa98b74bd90f03160c320084de0467766783..443518cf392c30a079e5132b1be6dde91a3fd77d 100644
--- a/ui/views/cocoa/bridged_native_widget_unittest.mm
+++ b/ui/views/cocoa/bridged_native_widget_unittest.mm
@@ -61,6 +61,11 @@
 
 namespace {
 
+enum class TestCase {
+  ALL,       // Test all strings.
+  LTR_ONLY,  // Only test Left To Right strings.
+};
+
 // Implemented NSResponder action messages for use in tests.
 NSArray* const kMoveActions = @[
   @"moveForward:",
@@ -332,11 +337,12 @@ void InstallTextField(const base::string16& text,
 
   // Test editing commands in |selectors| against the expectations set by
   // |dummy_text_view_|. This is done by selecting every substring within a set
-  // of test strings (both RTL and non-RTL) and performing every selector on
-  // both the NSTextView and the BridgedContentView hosting a focused
-  // views::TextField to ensure the resulting text and selection ranges match.
-  // |selectors| is an NSArray of NSStrings.
-  void TestEditingCommands(NSArray* selectors);
+  // of test strings (both RTL and non-RTL by default) and performing every
+  // selector on both the NSTextView and the BridgedContentView hosting a
+  // focused views::TextField to ensure the resulting text and selection ranges
+  // match. |selectors| is an NSArray of NSStrings. |cases| determines whether
+  // RTL strings are to be tested.
+  void TestEditingCommands(NSArray* selectors, TestCase cases = TestCase::ALL);
 
   std::unique_ptr<views::View> view_;
 
@@ -545,11 +551,13 @@ void InstallTextField(const base::string16& text,
                     GetActualSelectionRange());
 }
 
-void BridgedNativeWidgetTest::TestEditingCommands(NSArray* selectors) {
-  const base::string16 test_strings[] = {
-      base::WideToUTF16(L"ab c"),
-      base::WideToUTF16(L"\x0634\x0632 \x064A")  // RTL string.
-  };
+void BridgedNativeWidgetTest::TestEditingCommands(NSArray* selectors,
+                                                  TestCase cases) {
+  std::vector<base::string16> test_strings;
+  test_strings.push_back(base::WideToUTF16(L"ab c"));
+  if (cases == TestCase::ALL)

[msw, 2016/08/16 18:37:58]

nit: curly braces

[karandeepb, 2016/08/17 04:37:23]

Done.

+    test_strings.push_back(
+        base::WideToUTF16(L"\x0634\x0632 \x064A"));  // RTL string.
 
   for (const base::string16& test_string : test_strings) {
     for (NSString* selector_string in selectors) {
@@ -1108,12 +1116,12 @@ void PerformInit() {
   TestEditingCommands(kMoveActions);
 }
 
-// Todo(karandeepb): Enable this test once the behavior of all move and select
-// commands are fixed.
 // Test move and select commands against expectations set by |dummy_text_view_|.
-TEST_F(BridgedNativeWidgetTest,
-       TextInput_MoveAndSelectEditingCommands_DISABLED) {
-  TestEditingCommands(kSelectActions);
+TEST_F(BridgedNativeWidgetTest, TextInput_MoveAndSelectEditingCommands) {

[msw, 2016/08/16 18:37:58]

Thanks for enabling this test for LTR!

+  // The behavior of NSTextView for RTL strings is buggy for some move and
+  // select commands. Hence don't test against an RTL string. See
+  // rdar://27863290.

[msw, 2016/08/16 18:37:58]

q: I'm not familiar with rdar, but I found via codesearch that this might
correspond to http://www.openradar.me/27863290, but that doesn't seem to work.
Can you verify that this is correct and tell me how to use this? Maybe [also]
use the long-form address? (ok as-is if it's meant to open a special mac app)

[karandeepb, 2016/08/17 04:37:22]

OpenRadar is an external system where you can log copies of radars that you file
against Apple so that other people can see them. You should be able to view the
actual bug report at http://bugreport.apple.com. See the internal Google page
related to Apple radars for how to login. (Don't think I should be pasting an
internal URL here). As for why we use url's of the form rdar://{IssueID}, I am
also not sure. This seems to be the practice in the existing code base and in
the internal Google page about radars.

+  TestEditingCommands(kSelectActions, TestCase::LTR_ONLY);
 }
 
 // Test delete commands against expectations set by |dummy_text_view_|.