[Mac] Preserve original selection when suggesting completions with diacritics

chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm

Index: chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
index 83126cf2e5eee10622c834fac7b0c7ca7142aed2..8a668973a4502c6caf627bce965f1a478245b9c8 100644
--- a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
+++ b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
@@ -4,6 +4,7 @@
 
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.h"
 
+#include "base/i18n/break_iterator.h"
 #include "base/mac/sdk_forward_declarations.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
@@ -49,13 +50,58 @@ BOOL ThePasteboardIsTooDamnBig() {
   return [[pb stringForType:type] length] > kMaxPasteLength;
 }
 
+NSRange VisualSelectedRangeFromRange(NSRange range, NSString* string) {
+  if (range.location >= string.length)
+    return range;
+  base::string16 text = base::SysNSStringToUTF16(string);
+  base::i18n::BreakIterator grapheme_iterator(
+      text, base::i18n::BreakIterator::BREAK_CHARACTER);
+  if (!grapheme_iterator.Init())
+    return range;
+  // This works because NSString uses UTF-16 code units.
+  while (range.location < text.length() &&
+         !grapheme_iterator.IsGraphemeBoundary(
+             static_cast<size_t>(range.location))) {
+    range.location++;
+    if (range.length > 0)
+      range.length--;
+  }
+  return range;
+}
+
 }  // namespace
 
+// Method exposed for the purpose of overriding.
+// Used to restore model's selection range when the view doesn't
+// match the model due to combining characters.
+//
+// In some cases, (completing 'y' to 'ÿour', for example), the autocomplete
+// system requests a selection range that begins on a combining character.
+// setSelectedRange: and friends document that the range passed to them
+// "must begin and end on glyph boundaries and not split base glyphs and
+// their nonspacing marks". If passed such a range, the selection is
+// expanded to include the original user input, preventing the user
+// from being able to type other words beginning with that letter.
+//
+// To resolve this, the field editor modifies the selection to start
+// on the next glyph boundary, then keeps track of the original and
+// modified selections, substituting the original when the user takes
+// actions that operate on the selection. Since there are many methods
+// in NSResponder (for example deleteToBeginningOfLine:) that operate
+// on the selection, rather than shimming them all, we override this
+// private method that they're implemented in terms of.
+//
+@interface NSTextView (PrivateTextEditing)
+- (void)_userReplaceRange:(NSRange)range withString:(NSString*)string;
+@end
+
 @interface AutocompleteTextFieldEditor ()<NSDraggingSource>
 @end
 
 @implementation AutocompleteTextFieldEditor
 
+@synthesize actualSelectedRange = actualSelectedRange_;
+
 - (BOOL)shouldDrawInsertionPoint {
   return [super shouldDrawInsertionPoint] &&
          ![[[self delegate] cell] hideFocusState];
@@ -400,6 +446,14 @@ BOOL ThePasteboardIsTooDamnBig() {
   return [dropHandler_ performDragOperation:sender];
 }
 
+- (void)_userReplaceRange:(NSRange)range withString:(NSString*)string {
+  if (NSEqualRanges(visualSelectedRange_, range) &&
+      !NSEqualRanges(visualSelectedRange_, actualSelectedRange_)) {
+    range = actualSelectedRange_;
+  }
+  [super _userReplaceRange:range withString:string];
+}
+
 // Prevent control characters from being entered into the Omnibox.
 // This is invoked for keyboard entry, not for pasting.
 - (void)insertText:(id)aString {
@@ -431,11 +485,15 @@ BOOL ThePasteboardIsTooDamnBig() {
     DCHECK_EQ(range.length, 0U);
   }
 
-  // NOTE: If |aString| is empty, this intentionally replaces the
-  // selection with empty.  This seems consistent with the case where
-  // the input contained a mixture of characters and the string ended
-  // up not empty.
-  [super insertText:aString];
+  if (!NSEqualRanges(visualSelectedRange_, actualSelectedRange_)) {
+    [super replaceCharactersInRange:actualSelectedRange_ withString:aString];
+  } else {
+    // NOTE: If |aString| is empty, this intentionally replaces the
+    // selection with empty.  This seems consistent with the case where
+    // the input contained a mixture of characters and the string ended
+    // up not empty.
+    [super insertText:aString];
+  }
 }
 
 - (NSRange)selectionRangeForProposedRange:(NSRange)proposedSelRange
@@ -451,7 +509,11 @@ BOOL ThePasteboardIsTooDamnBig() {
 - (void)setSelectedRange:(NSRange)charRange
                 affinity:(NSSelectionAffinity)affinity
           stillSelecting:(BOOL)flag {
-  [super setSelectedRange:charRange affinity:affinity stillSelecting:flag];
+  actualSelectedRange_ = charRange;
+  visualSelectedRange_ = VisualSelectedRangeFromRange(charRange, [self string]);
+  [super setSelectedRange:visualSelectedRange_
+                 affinity:affinity
+           stillSelecting:flag];
 
   // We're only interested in selection changes directly caused by keyboard
   // input from the user.
@@ -459,6 +521,22 @@ BOOL ThePasteboardIsTooDamnBig() {
     textChangedByKeyEvents_ = YES;
 }
 
+// This is called by |moveLeftAndModifySelection:| and similar methods, despite
+// the fact that this field is opted out of multiple selection due to the
+// delegate not implementing
+// |textView:willChangeSelectionFromCharacterRanges:oldSelectedCharRanges:
+// toCharacterRanges:|
+//
+// Since this method isn't being invoked by the omnibox completion, we just
+// ensure that |visualSelectedRange_| and |actualSelectedRange_| and don't try
+// to adjust for grapheme boundaries.
+- (void)setSelectedRanges:(NSArray<NSValue *> *) ranges
+                 affinity:(NSSelectionAffinity)affinity
+           stillSelecting:(BOOL)flag {

[erikchen, 2016/10/14 18:41:35]

-setSelectedRange:... actually calls -setSelectedRanges:

disassembly pseudocode:
"""
void -[NSTextView setSelectedRange:affinity:stillSelecting:](void * self, void *
_cmd, struct _NSRange arg2, unsigned long long arg3, char arg4) {
    rdx = [NSSelectionArray arrayWithRange:arg2];
    [self setSelectedRanges:rdx affinity:arg4 stillSelecting:r9];
    return;
}
"""

Maybe we should just override the latter?

[lgrey, 2016/10/14 20:42:35]

How's this?

+  [super setSelectedRanges:ranges affinity:affinity stillSelecting:flag];
+  actualSelectedRange_ = visualSelectedRange_ = [self selectedRange];
+}
+
 - (void)interpretKeyEvents:(NSArray *)eventArray {
   DCHECK(!interpretingKeyEvents_);
   interpretingKeyEvents_ = YES;
@@ -564,7 +642,7 @@ BOOL ThePasteboardIsTooDamnBig() {
 
 - (BOOL)validateMenuItem:(NSMenuItem*)item {
   if ([item action] == @selector(copyToFindPboard:))
-    return [self selectedRange].length > 0;
+    return actualSelectedRange_.length > 0;
   if ([item action] == @selector(pasteAndGo:)) {
     // TODO(rohitrao): If the clipboard is empty, should we show a
     // greyed-out "Paste and Go" or nothing at all?
@@ -576,11 +654,10 @@ BOOL ThePasteboardIsTooDamnBig() {
 }
 
 - (void)copyToFindPboard:(id)sender {
-  NSRange selectedRange = [self selectedRange];
-  if (selectedRange.length == 0)
+  if (actualSelectedRange_.length == 0)
     return;
   NSAttributedString* selection =
-      [self attributedSubstringForProposedRange:selectedRange
+      [self attributedSubstringForProposedRange:actualSelectedRange_
                                     actualRange:NULL];
   if (!selection)
     return;