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

ui/views/cocoa/bridged_native_widget_unittest.mm

 // Copyright 2014 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.
 
 #import "ui/views/cocoa/bridged_native_widget.h"
 
 #import <Cocoa/Cocoa.h>
 
 #include <memory>
 
@@ skipping 43 matching lines @@
 #define EXPECT_EQ_RANGE_3(expected_literal, expected_cocoa, actual_views) \
   EXPECT_EQ_RANGE(expected_literal, actual_views);                        \
   EXPECT_EQ_RANGE(expected_cocoa, actual_views);
 
 #define EXPECT_EQ_3(expected_literal, expected_cocoa, actual_views) \
   EXPECT_EQ(expected_literal, actual_views);                        \
   EXPECT_EQ(expected_cocoa, actual_views);
 
 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:",
   @"moveRight:",
   @"moveBackward:",
   @"moveLeft:",
   @"moveUp:",
   @"moveDown:",
   @"moveWordForward:",
   @"moveWordBackward:",
@@ skipping 251 matching lines @@
   // Test delete to beginning of line or paragraph based on |sel|. |sel| can be
   // either deleteToBeginningOfLine: or deleteToBeginningOfParagraph:.
   void TestDeleteBeginning(SEL sel);
 
   // Test delete to end of line or paragraph based on |sel|. |sel| can be
   // either deleteToEndOfLine: or deleteToEndOfParagraph:.
   void TestDeleteEnd(SEL sel);
 
   // 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_;
 
   // Weak. Owned by bridge().
   BridgedContentView* ns_view_;
 
   // An NSTextView which helps set the expectations for our tests.
   base::scoped_nsobject<NSTextView> dummy_text_view_;
 
   base::MessageLoopForUI message_loop_;
@@ skipping 188 matching lines @@
   EXPECT_EQ_RANGE_3(NSMakeRange(0, 0), GetExpectedSelectionRange(),
                     GetActualSelectionRange());
 
   // Verify yanking inserts the deleted text.
   PerformCommand(@selector(yank:));
   EXPECT_NSEQ_3(@"foo bar", GetExpectedText(), GetActualText());
   EXPECT_EQ_RANGE_3(NSMakeRange(4, 0), GetExpectedSelectionRange(),
                     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) {
       SEL sel = NSSelectorFromString(selector_string);
       const int len = test_string.length();
       for (int i = 0; i <= len; i++) {
         for (int j = 0; j <= len; j++) {
           SCOPED_TRACE(base::StringPrintf(
               "Testing range [%d-%d] for case %s and selector %s\n", i, j,
               base::UTF16ToUTF8(test_string).c_str(),
@@ skipping 538 matching lines @@
 
 TEST_F(BridgedNativeWidgetTest, TextInput_DeleteToEndOfParagraph) {
   TestDeleteEnd(@selector(deleteToEndOfParagraph:));
 }
 
 // Test move commands against expectations set by |dummy_text_view_|.
 TEST_F(BridgedNativeWidgetTest, TextInput_MoveEditingCommands) {
   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_|.
 TEST_F(BridgedNativeWidgetTest, TextInput_DeleteCommands) {
   TestEditingCommands(kDeleteActions);
 }
 
 // Test that we don't crash during an action message even if the TextInputClient
 // is nil. Regression test for crbug.com/615745.
 TEST_F(BridgedNativeWidgetTest, NilTextInputClient) {
@@ skipping 170 matching lines @@
   [center postNotificationName:NSWindowDidExitFullScreenNotification
                         object:window];
   EXPECT_EQ(1, [window ignoredToggleFullScreenCount]);  // No change.
   EXPECT_FALSE(bridge()->target_fullscreen_state());
 
   widget_->CloseNow();
 }
 
 }  // namespace test
 }  // namespace views