Mac: Implement firstRectForCharacterRange:actualRange in BridgedContentView.

ui/views/cocoa/bridged_content_view.mm

Index: ui/views/cocoa/bridged_content_view.mm
diff --git a/ui/views/cocoa/bridged_content_view.mm b/ui/views/cocoa/bridged_content_view.mm
index 2167561368c38f1b8a637d99c1b1daa022faae15..1ed14cb455ca17f1e7c9934c9e33653f47922ac3 100644
--- a/ui/views/cocoa/bridged_content_view.mm
+++ b/ui/views/cocoa/bridged_content_view.mm
@@ -16,6 +16,7 @@
 #import "ui/events/keycodes/keyboard_code_conversion_mac.h"
 #include "ui/gfx/canvas_paint_mac.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/mac/coordinate_conversion.h"

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

nit: import

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

Done.

 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/controls/menu/menu_config.h"
 #include "ui/views/controls/menu/menu_controller.h"
@@ -56,6 +57,98 @@ bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
   return false;
 }
 
+// Returns the boundary rectangle for composition characters in the
+// |requested_range|. Sets |actual_range| corresponding to the returned
+// rectangle. For cases, where there is no composition text or the
+// |requested_range| lies outside the composition range, a zero width rectangle
+// corresponding to the caret bounds is returned. Logic used is similar to
+// RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(...).
+gfx::Rect GetFirstRectForRangeHelper(const ui::TextInputClient* client,
+                                     const gfx::Range& requested_range,
+                                     gfx::Range* actual_range) {
+  // NSRange doesn't support reversed ranges.
+  DCHECK(!requested_range.is_reversed());
+  DCHECK(actual_range);
+
+  // Set up default return values, to be returned in case of unusual cases.
+  gfx::Rect default_rect;
+  *actual_range = gfx::Range::InvalidRange();
+  if (!client)
+    return default_rect;
+
+  // If possible, modify default return values to correspond to caret position.
+  gfx::Range selection_range;
+  if (client->GetSelectionRange(&selection_range)) {
+    default_rect = client->GetCaretBounds();
+    default_rect.set_width(0);
+    // Caret bounds correspond to end index of selection_range.
+    *actual_range = gfx::Range(selection_range.end());
+  }
+
+  if (!client->HasCompositionText())
+    return default_rect;
+
+  gfx::Range composition_range;
+  if (!client->GetCompositionTextRange(&composition_range))
+    return default_rect;
+
+  DCHECK(!composition_range.is_reversed());
+
+  if (!composition_range.Contains(requested_range))
+    return default_rect;
+
+  // Range relative to composition_range.start().
+  const gfx::Range relative_range(
+      requested_range.start() - composition_range.start(),
+      requested_range.end() - composition_range.start());
+
+  // Pick the first character's bounds as the initial rectangle, then grow it to
+  // the full |requested_range| if possible.
+  gfx::Rect union_rect;
+  const size_t composition_count = composition_range.length();
+
+  // In the case where relative_range is [composition_count, composition_count],
+  // use the bounds for the last compositioned character.
+  const bool request_is_composition_end =
+      relative_range.start() == composition_count;
+  const size_t first_index = request_is_composition_end
+                                 ? composition_count - 1
+                                 : relative_range.start();
+  if (!client->GetCompositionCharacterBounds(first_index, &union_rect))
+    return default_rect;
+
+  // If relative_range is empty, return a zero width rectangle corresponding to
+  // the relative_range.
+  if (relative_range.is_empty()) {

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

Can any reviewer confirm whether the handling of this case is correct? Apple
documentation says to return a zero width rectangle coinciding with the caret
position for this case -
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTextI...:
. However, in RenderWidgetHostViewMac::GetCachedFirstRectForCharacterRange(...),
the case is handled as below. In practice, I can't get this code path to execute
since Mac's native IMEs only query with an empty range when there is no
composition text(which seems reasonable), and hence we return the caret bounds
for it(line 89). Also, in my tests Google's Japanese IME for mac never queries
with an empty range.

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

I don't know the answer from a NSTextInputClient standpoint, but there's
typically a strong aversion to committing (non-test) code that's effectively
dead in a Chrome release (i.e. impossible to execute, or only executed in
tests).

In this case, you can spelunk through the code review where it originally went
in:

(assuming I followed it correctly), the logic came from here:

https://chromiumcodereview.appspot.com/10656019/diff2/17010:21006/content/bro...

The reviewer gave the comment, "I'm still wondering if it's possible that an IME
wants to know the end position of a composition range, so it calls this method
with rang.start() == rang.end() == composition_bounds_.size() ?"

It won't be the case that IME on Mac will always be Apple's implementation
(Chrome has support for plugins/extensions as well), so I think it's important
to keep the logic. It would be great if you could single out the test case that
gives coverage of this block though -- annotate the EXPECT_FOO call with a
comment along with a quote from the apple documentation that says our
implementation might be non-standard. (This way, if the test starts failing for
someone who later tries to align it to the standard, they can make sense of why
it is the way it is).

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

Done.

+    if (request_is_composition_end) {
+      // In case of an empty requested range at end of composition, return the
+      // rectangle to the right of the last compositioned character.
+      union_rect.set_origin(union_rect.top_right());
+    }
+    union_rect.set_width(0);
+    *actual_range = requested_range;
+    return union_rect;
+  }
+
+  // Toolkit-views textfields are always single-line, so no need to check for
+  // line breaks.
+
+  // Return the union rectangle of the character bounds within the
+  // relative_range.
+  for (size_t i = relative_range.start() + 1; i < relative_range.end(); i++) {
+    gfx::Rect current_rect;
+    if (client->GetCompositionCharacterBounds(i, &current_rect)) {
+      union_rect.Union(current_rect);
+    } else {
+      *actual_range =
+          gfx::Range(requested_range.start(), i + composition_range.start());
+      return union_rect;
+    }
+  }
+  *actual_range = requested_range;
+  return union_rect;
+}
+
 }  // namespace
 
 @interface BridgedContentView ()
@@ -591,9 +684,13 @@ bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
 }
 
 - (NSRect)firstRectForCharacterRange:(NSRange)range
-                         actualRange:(NSRangePointer)actualRange {
-  NOTIMPLEMENTED();
-  return NSZeroRect;
+                         actualRange:(NSRangePointer)actualNSRange {
+  gfx::Range actualRange;
+  gfx::Rect rect = GetFirstRectForRangeHelper(textInputClient_,
+                                              gfx::Range(range), &actualRange);
+  if (actualNSRange)
+    *actualNSRange = actualRange.ToNSRange();
+  return gfx::ScreenRectToNSRect(rect);
 }
 
 - (BOOL)hasMarkedText {