Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView.

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>
 
 #import "base/mac/foundation_util.h"
 #import "base/mac/mac_util.h"
 #import "base/mac/sdk_forward_declarations.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #import "testing/gtest_mac.h"
 #import "ui/base/cocoa/window_size_constants.h"
 #include "ui/base/ime/input_method.h"
 #import "ui/gfx/test/ui_cocoa_test_helper.h"
+#include "ui/gfx/mac/coordinate_conversion.h"

[tapted, 2015/12/30 06:48:53]

nit: import

[karandeepb, 2015/12/31 02:42:43]

Done.

 #import "ui/views/cocoa/bridged_content_view.h"
 #import "ui/views/cocoa/native_widget_mac_nswindow.h"
 #import "ui/views/cocoa/views_nswindow_delegate.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/native_widget_mac.h"
 #include "ui/views/widget/root_view.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
 using base::ASCIIToUTF16;
 using base::SysNSStringToUTF8;
 using base::SysNSStringToUTF16;
 using base::SysUTF8ToNSString;
 
 #define EXPECT_EQ_RANGE(a, b)        \
   EXPECT_EQ(a.location, b.location); \
   EXPECT_EQ(a.length, b.length);
 
 namespace {
 
 // Empty range shortcut for readibility.
 NSRange EmptyRange() {
   return NSMakeRange(NSNotFound, 0);
 }
 
+// Sets |composition_text| as the composition text with caret placed at
+// |caret_pos| and updates |caret_range|.
+void SetCompositionText(ui::TextInputClient* client,
+                        const base::string16& composition_text,
+                        const int caret_pos,
+                        NSRange* caret_range = nil) {

[tapted, 2015/12/30 06:48:53]

I don't think anything relies on the default argument. Remove?

[karandeepb, 2015/12/31 02:42:43]

Done.

+  ui::CompositionText composition;
+  composition.selection = gfx::Range(caret_pos);
+  composition.text = composition_text;
+  client->SetCompositionText(composition);
+  if (caret_range)
+    *caret_range = NSMakeRange(caret_pos, 0);
+}
+
+// Returns a zero width rectangle corresponding to current caret position.
+gfx::Rect GetCaretBounds(const ui::TextInputClient* client) {
+  gfx::Rect caret_bounds = client->GetCaretBounds();
+  caret_bounds.set_width(0);
+  return caret_bounds;
+}
+
+// Returns a zero width rectangle corresponding to caret bounds when it's placed
+// at |caret_pos| and updates |caret_range|.
+gfx::Rect GetCaretBoundsForPosition(ui::TextInputClient* client,
+                                    const base::string16& composition_text,
+                                    const int caret_pos,
+                                    NSRange* caret_range = nil) {

[tapted, 2015/12/30 06:48:52]

I think there's one call that wants the default - maybe just omit it. default
arguments haven't really taken off in Chrome yet, since they've only recently
been blessed by go/cppguide

[karandeepb, 2015/12/31 02:42:44]

Done.

+  SetCompositionText(client, composition_text, caret_pos, caret_range);
+  return GetCaretBounds(client);
+}
+
 }  // namespace
 
 // Class to override -[NSWindow toggleFullScreen:] to a no-op. This simulates
 // NSWindow's behavior when attempting to toggle fullscreen state again, when
 // the last attempt failed but Cocoa has not yet sent
 // windowDidFailToEnterFullScreen:.
 @interface BridgedNativeWidgetTestFullScreenWindow : NativeWidgetMacNSWindow {
  @private
   int ignoredToggleFullScreenCount_;
 }
@@ skipping 65 matching lines @@
     // before the tests covering the Init() flow are ready to do that.
     init_params_.type = Widget::InitParams::TYPE_WINDOW_FRAMELESS;
 
     // To control the lifetime without an actual window that must be closed,
     // tests in this file need to use WIDGET_OWNS_NATIVE_WIDGET.
     init_params_.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
 
     // Opacity defaults to "infer" which is usually updated by ViewsDelegate.
     init_params_.opacity = Widget::InitParams::OPAQUE_WINDOW;
 
+    init_params_.bounds = gfx::Rect(100, 100, 100, 100);
+
     native_widget_mac_->GetWidget()->Init(init_params_);
   }
 
  protected:
   scoped_ptr<Widget> widget_;
   MockNativeWidgetMac* native_widget_mac_;  // Weak. Owned by |widget_|.
 
   // Make the InitParams available to tests to cover initialization codepaths.
   Widget::InitParams init_params_;
 };
@@ skipping 26 matching lines @@
 
 BridgedNativeWidgetTest::BridgedNativeWidgetTest() {
 }
 
 BridgedNativeWidgetTest::~BridgedNativeWidgetTest() {
 }
 
 void BridgedNativeWidgetTest::InstallTextField(const std::string& text) {
   Textfield* textfield = new Textfield();
   textfield->SetText(ASCIIToUTF16(text));
+  textfield->SetBoundsRect(init_params_.bounds);
   view_->AddChildView(textfield);
 
   // Request focus so the InputMethod can dispatch events to the RootView, and
   // have them delivered to the textfield. Note that focusing a textfield
   // schedules a task to flash the cursor, so this requires |message_loop_|.
   textfield->RequestFocus();
 
   [ns_view_ setTextInputClient:textfield];
 }
 
@@ skipping 311 matching lines @@
   EXPECT_EQ("a", GetText());
   EXPECT_EQ_RANGE(NSMakeRange(1, 0), [ns_view_ selectedRange]);
 
   // Should succeed after moving left first.
   [ns_view_ doCommandBySelector:@selector(moveLeft:)];
   [ns_view_ doCommandBySelector:@selector(deleteForward:)];
   EXPECT_EQ("", GetText());
   EXPECT_EQ_RANGE(NSMakeRange(0, 0), [ns_view_ selectedRange]);
 }
 
+// Test firstRectForCharacterRange:actualRange for cases where query range is
+// empty or outside composition range.
+TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange_Caret) {
+  InstallTextField("");
+  ui::TextInputClient* client = [ns_view_ textInputClient];
+
+  // No composition. Ensure bounds and range corresponding to the current caret
+  // position are returned.
+  // Initially selection range will be [0, 0].
+  NSRange caret_range = NSMakeRange(0, 0);
+  NSRange query_range = NSMakeRange(1, 1);
+  NSRange actual_range;
+  NSRect rect = [ns_view_ firstRectForCharacterRange:query_range
+                                         actualRange:&actual_range];
+  EXPECT_EQ(GetCaretBounds(client), gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(caret_range, actual_range);
+
+  // Set composition with caret before second character ('e').
+  const base::string16 kTestString = base::ASCIIToUTF16("test_str");

[tapted, 2015/12/30 06:48:53]

So there's a weird convention (or, the literal interpretation of the style
guide) for naming constants that only primitive types are named as kFooBar (e.g.
char[] or int is fine, but std::string should be `const std::string
test_string;`)

[karandeepb, 2015/12/31 02:42:44]

Can't find this in the cpp style guide -
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#....
Also, similar naming has been followed at other places in the file, should I
refactor it?

[tapted, 2016/01/04 02:28:10]

It says "Variables declared constexpr or const, and whose value is fixed for the
duration of the program". Things with constructors only take on a value when
they're constructed and that value is gone when it's destroyed, so I've had
senior reviewers ping me on this in the past.

So it's something to be aware of, but it's still a "guide" :). You can leave
this as kTestString if you prefer - consistency is important too, and it can
sometimes trump other style concerns. I'll leave it up to you. (Anecdotally,
there's a `kTestString.size()` elsewhere in the file -- this looks really
jarring to me, so I'd support changing them all, but it should be in a separate
CL since there's a lot).

[karandeepb, 2016/01/04 04:33:55]

Done. Will refactor the other usages in a separate CL.

+  const size_t kTextLength = 8;
+  SetCompositionText(client, kTestString, 1, &caret_range);
+
+  // Test empty ranges within the composition range.

[tapted, 2015/12/30 06:48:53]

nit: follow comment with the expectations, E.g. 

"Test empty range after the second character: should return caret bounds
(since..?)"

[karandeepb, 2015/12/31 02:42:43]

Done.

+  query_range = NSMakeRange(1, 0);
+  rect = [ns_view_ firstRectForCharacterRange:query_range
+                                  actualRange:&actual_range];
+  EXPECT_EQ(GetCaretBoundsForPosition(client, kTestString, 1, &caret_range),
+            gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(query_range, actual_range);
+
+  query_range = NSMakeRange(kTextLength, 0);

[tapted, 2015/12/30 06:48:53]

likewise, "Test empty range at the end of the text: should return <..>"

[karandeepb, 2015/12/31 02:42:44]

Done.

+  rect = [ns_view_ firstRectForCharacterRange:query_range
+                                  actualRange:&actual_range];
+  EXPECT_NE(GetCaretBoundsForPosition(client, kTestString, 1, &caret_range),
+            gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ(
+      GetCaretBoundsForPosition(client, kTestString, kTextLength, &caret_range),
+      gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(query_range, actual_range);
+
+  // Query outside composition range. Ensure bounds and range corresponding to
+  // the current caret position are returned.
+  query_range = NSMakeRange(kTextLength + 1, 0);
+  rect = [ns_view_ firstRectForCharacterRange:query_range
+                                  actualRange:&actual_range];
+  EXPECT_EQ(GetCaretBounds(client), gfx::ScreenRectFromNSRect(rect));
+  EXPECT_EQ_RANGE(caret_range, actual_range);
+
+  // Make sure not crashing by passing NULL pointer instead of actualRange.

[tapted, 2015/12/30 06:48:52]

nit: NULL->null

[karandeepb, 2015/12/31 02:42:43]

Done.

+  rect = [ns_view_ firstRectForCharacterRange:query_range actualRange:NULL];

[tapted, 2015/12/30 06:48:53]

nit: NULL->nullptr

(again it's convention: `null` in comments, but always `nullptr` in code)

[karandeepb, 2015/12/31 02:42:43]

Done.

[tapted, 2016/01/04 02:28:10]

(ooh, I should say -- and you may have already figured this one out --  'nil' is
also acceptable in an objective C context if it's a pointer to a [subclass of]
NSObject*. Here nullptr is more correct since NSRange is a C-style struct and
not a subtype of NSObject.)

[karandeepb, 2016/01/04 04:33:55]

Didn't know all of it. Thanks!

+}
+
+// Test firstRectForCharacterRange:actualRange for all valid ranges of a test
+// string.
+TEST_F(BridgedNativeWidgetTest, TextInput_FirstRectForCharacterRange) {
+  InstallTextField("");
+  ui::TextInputClient* client = [ns_view_ textInputClient];
+  const base::string16 kTestString = base::ASCIIToUTF16("test_str");

[tapted, 2015/12/30 06:48:53]

Is there a minimum test string that can give code coverage without requiring the
loops below?

It's usually confusing to put EXPECT_* statements into loops, since you're not
sure what the input data is.

You can add SCOPED_TRACE(testing::Message() << "Range [begin, end) = [" << i <<
", " << j << ")"); to the inner loop, but it's clumsy. No loops would be best.

[karandeepb, 2015/12/31 02:42:43]

Removed the loops. Somehow, this approach did not strike me earlier!

+  const size_t kTextLength = 8;
+
+  std::vector<gfx::Rect> char_bounds(kTextLength);
+  std::vector<gfx::Rect> caret_bounds(kTextLength + 1);
+
+  // Generate caret_bounds between different characters.
+  for (size_t i = 0; i <= kTextLength; i++)
+    caret_bounds[i] = GetCaretBoundsForPosition(client, kTestString, i);
+
+  // Generate individual character bounds from caret positions.
+  for (size_t i = 0; i < kTextLength; i++) {
+    char_bounds[i].set_origin(caret_bounds[i].origin());
+    char_bounds[i].set_width(caret_bounds[i + 1].x() - caret_bounds[i].x());
+    char_bounds[i].set_height(caret_bounds[i].height());
+  }
+
+  // Verify bounds for all valid ranges.
+  NSRange query_range;
+  NSRange actual_range;
+  NSRect rect;
+  gfx::Rect expected_bounds;
+  for (size_t i = 0; i < kTextLength; i++) {
+    for (size_t j = i + 1; j <= kTextLength; j++) {
+      query_range = NSMakeRange(i, j - i);
+      rect = [ns_view_ firstRectForCharacterRange:query_range
+                                      actualRange:&actual_range];
+
+      expected_bounds = gfx::Rect();
+      for (size_t k = i; k < j; k++)
+        expected_bounds.Union(char_bounds[k]);
+
+      EXPECT_EQ(expected_bounds, gfx::ScreenRectFromNSRect(rect));
+      EXPECT_EQ_RANGE(query_range, actual_range);
+    }
+  }
+}
+
 typedef BridgedNativeWidgetTestBase BridgedNativeWidgetSimulateFullscreenTest;
 
 // Simulate the notifications that AppKit would send out if a fullscreen
 // operation begins, and then fails and must abort. This notification sequence
 // was determined by posting delayed tasks to toggle fullscreen state and then
 // mashing Ctrl+Left/Right to keep OSX in a transition between Spaces to cause
 // the fullscreen transition to fail.
 TEST_F(BridgedNativeWidgetSimulateFullscreenTest, FailToEnterAndExit) {
   if (base::mac::IsOSSnowLeopard())
     return;
@@ skipping 58 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