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..7c07fb14dfedfdff9d8ee2157624e2ac6bb79f91 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"
 #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,72 @@ bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
   return false;
 }
 
+gfx::Rect GetFirstRectForRangeHelper(const ui::TextInputClient* client,

[tapted, 2015/12/22 02:33:57]

This method is big enough that it should have a comment

[karandeepb, 2015/12/29 07:21:15]

Done.

+                                     const gfx::Range& requested_range,
+                                     gfx::Range* actual_range) {
+  if (!client)
+    return gfx::Rect();
+
+  // NSRange doesn't support reversed ranges.
+  DCHECK(!requested_range.is_reversed());
+
+  // Set up default return values, to be returned in case of unusual cases.
+  gfx::Rect caret_bounds = client->GetCaretBounds();
+  caret_bounds.set_width(0);
+  *actual_range = gfx::Range(requested_range.start());
+
+  if (!client->HasCompositionText())
+    return caret_bounds;
+
+  gfx::Range composition_range;
+  if (!client->GetCompositionTextRange(&composition_range))

[tapted, 2015/12/22 02:33:58]

Can the composition_range be reversed? (will that break stuff?) DCHECK that it's
not?

[karandeepb, 2015/12/29 07:21:15]

Its not the case for a textfield atleast -
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/....
Added a DCHECK.

+    return caret_bounds;
+
+  if (!composition_range.Contains(requested_range))
+    return caret_bounds;
+
+  // Range relative to composition_range.start().
+  gfx::Range relative_range(requested_range.start() - composition_range.start(),
+                            requested_range.end() - composition_range.start());
+  gfx::Rect union_rect;

[tapted, 2015/12/22 02:33:58]

Maybe a comment about what's going on with |union_rect|. Like, // Pick a single
character's bounds as the initial rect, then grow it to the full
|requested_range| if possible.

[karandeepb, 2015/12/29 07:21:15]

Done.

+  size_t composition_count =

[tapted, 2015/12/22 02:33:58]

composition_count -> composition_range.length() (also can it be zero? can that
break stuff?)

[karandeepb, 2015/12/29 07:21:15]

Done. It can't be zero since we have already checked for the case where there is
no composition text in - 
if (!client->HasCompositionText()) ...

+      composition_range.end() - composition_range.start();
+  size_t first_index = relative_range.start() == composition_count

[tapted, 2015/12/22 02:33:57]

Needs a comment here - it's not immediately obvious what's happening. i.e. //
Pick the initial character based on ...

[karandeepb, 2015/12/29 07:21:15]

Done.

+                           ? composition_count - 1
+                           : relative_range.start();
+  if (!client->GetCompositionCharacterBounds(first_index, &union_rect))
+    return caret_bounds;
+
+  // If |requested_range| is empty, return a zero width rectangle corresponding

[tapted, 2015/12/22 02:33:58]

comment needs updating

[karandeepb, 2015/12/29 07:21:15]

Updated. I had kept this because relative_range is just for arithmetic
convenience. Actual bounds returned still correspond to requested_range.

+  // to the |requested_range|.
+  if (relative_range.is_empty()) {
+    if (relative_range.start() == composition_count) {

[tapted, 2015/12/22 02:33:58]

Maybe this can be saved in a bool up above to help readability. like

const request_is_composition_end = relative_range.start() == composition_count;

(not sure if that's a good name, since I'm not familiar with this IME stuff)

[karandeepb, 2015/12/29 07:21:15]

Done.

+      // Return a rectangle corresponding to the top right corner of the last

[tapted, 2015/12/22 02:33:58]

Rather than a comment just saying what the code does, it can be good to say why
it's necessary, and why what it's doing works (or, sometimes, why doing the
alternative is bad -- i.e. if a special case is needed and it's not clear why,
the special case needs a justification).

E.g. // To handle a requested range that's <something> move the rectangle to the
right side to ensure the candidate window appears <somehow>.

[karandeepb, 2015/12/29 07:21:15]

This is a case which I can't reproduce in practice, hence can't really explain
how this affects the candidate window. Returning the rectangle to the right of
the last compositioned character feels natural here.

+      // compositioned character.
+      union_rect.set_origin(union_rect.top_right());

[tapted, 2015/12/22 02:33:57]

Is top_right always correct? (e.g. is there any such thing as RTL in IME, which
might require a different position?)

[karandeepb, 2015/12/29 07:21:15]

I couldn't find any RTL IME. I had also talked to Matt regarding this, and he
was also not aware of any RTL IME. The only case which might be worth looking at
is using IMEs in an RTL context(when the system language is RTL). Apple's
documentation for firstRectForCharacterRange:actualRange: says that the returned
rectangle can have a negative size if text flows to left. However, I couldn't
observe any visible difference on setting the returned rectangle's size to be
negative.

+    }
+    union_rect.set_width(0);
+    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
+  // |requested_range|.
+  gfx::Rect current_rect;
+  for (size_t i = relative_range.start() + 1; i < relative_range.end(); i++) {
+    if (client->GetCompositionCharacterBounds(i, &current_rect)) {
+      union_rect.Union(current_rect);
+    } else {
+      actual_range->set_end(i + composition_range.start());
+      return union_rect;
+    }
+  }
+  actual_range->set_end(requested_range.end());
+  return union_rect;
+}
+
 }  // namespace
 
 @interface BridgedContentView ()
@@ -591,9 +658,15 @@ bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
 }
 
 - (NSRect)firstRectForCharacterRange:(NSRange)range
-                         actualRange:(NSRangePointer)actualRange {
-  NOTIMPLEMENTED();
-  return NSZeroRect;
+                         actualRange:(NSRangePointer)actualNSRange {
+  gfx::Range requestedRange(range);

[tapted, 2015/12/22 02:33:57]

nit: temporary not needed

[karandeepb, 2015/12/29 07:21:15]

Done.

+  gfx::Range actualRange;
+  gfx::Rect rect = GetFirstRectForRangeHelper(textInputClient_, requestedRange,
+                                              &actualRange);
+  *actualNSRange = actualRange.ToNSRange();
+
+  // Convert rect to AppKit screen coordinates.

[tapted, 2015/12/22 02:33:58]

nit: comment not needed (it's on the function prototype :)

[karandeepb, 2015/12/29 07:21:15]

Done.

+  return gfx::ScreenRectToNSRect(rect);
 }
 
 - (BOOL)hasMarkedText {