[Mac] Preserve original selection when suggesting completions with diacritics

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

 // Copyright (c) 2011 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 "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"
 #include "chrome/app/chrome_command_ids.h"  // IDC_*
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/ui/browser_list.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.h"
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
@@ skipping 25 matching lines @@
 BOOL ThePasteboardIsTooDamnBig() {
   NSPasteboard* pb = [NSPasteboard generalPasteboard];
   NSString* type =
       [pb availableTypeFromArray:[NSArray arrayWithObject:NSStringPboardType]];
   if (!type)
     return NO;
 
   return [[pb stringForType:type] length] > kMaxPasteLength;
 }
 
+NSRange VisualSelectionRangeFromRange(NSRange range, NSString* string) {
+  if (range.location >= string.length) {
+    return range;
+  }
+  base::string16 text = base::SysNSStringToUTF16(string);
+  // TODO(lgrey): Should the break iterator be cached?

[erikchen, 2016/10/12 22:37:48]

sounds like premature optimization to me.

[lgrey, 2016/10/13 15:42:52]

Removed comment

+  base::i18n::BreakIterator grapheme_iterator(
+      text, base::i18n::BreakIterator::BREAK_CHARACTER);
+  if (!grapheme_iterator.Init()) {
+    return range;
+  }
+  while (range.location < text.length() &&

[erikchen, 2016/10/12 22:37:48]

interesting. This works because NSString's internal representation uses UTF-16
code units.
https://developer.apple.com/reference/foundation/nsstring

I wasn't aware of that. Do you think it's worth adding a comment here?

[lgrey, 2016/10/13 15:42:52]

Added a comment. I could also change it to |string.length|, but that still
requires the explanation, since the break iterator is operating on a string16.
What do you think?

[erikchen, 2016/10/13 17:08:28]

Comments seems good enough.

+         !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
 
 - (BOOL)shouldDrawInsertionPoint {
   return [super shouldDrawInsertionPoint] &&
          ![[[self delegate] cell] hideFocusState];
 }
 
@@ skipping 329 matching lines @@
 // (URLDropTarget protocol)
 - (void)draggingExited:(id<NSDraggingInfo>)sender {
   return [dropHandler_ draggingExited:sender];
 }
 
 // (URLDropTarget protocol)
 - (BOOL)performDragOperation:(id<NSDraggingInfo>)sender {
   return [dropHandler_ performDragOperation:sender];
 }
 
+- (void)_userReplaceRange:(NSRange)range withString:(NSString*)string {
+  if (NSEqualRanges(visualSelectionRange_, range) &&
+      !NSEqualRanges(visualSelectionRange_, actualSelectionRange_)) {
+    range = actualSelectionRange_;
+  }
+  [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 {
   AutocompleteTextFieldObserver* observer = [self observer];
   if (observer)
     observer->OnInsertText();
 
   // Repeatedly remove control characters.  The loop will only ever
   // execute at all when the user enters control characters (using
   // Ctrl-Alt- or Ctrl-Q).  Making this generally efficient would
@@ skipping 11 matching lines @@
   } else {
     NSRange range = [aString rangeOfCharacterFromSet:forbiddenCharacters_];
     while (range.location != NSNotFound) {
       aString =
           [aString stringByReplacingCharactersInRange:range withString:@""];
       range = [aString rangeOfCharacterFromSet:forbiddenCharacters_];
     }
     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(visualSelectionRange_, actualSelectionRange_)) {
+    [super replaceCharactersInRange:actualSelectionRange_ 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
                               granularity:(NSSelectionGranularity)granularity {
   AutocompleteTextFieldObserver* observer = [self observer];
   NSRange modifiedRange = [super selectionRangeForProposedRange:proposedSelRange
                                                     granularity:granularity];
   if (observer)
     return observer->SelectionRangeForProposedRange(modifiedRange);
   return modifiedRange;
 }
 
 - (void)setSelectedRange:(NSRange)charRange
                 affinity:(NSSelectionAffinity)affinity
           stillSelecting:(BOOL)flag {
-  [super setSelectedRange:charRange affinity:affinity stillSelecting:flag];
+  actualSelectionRange_ = charRange;
+  visualSelectionRange_ =
+      VisualSelectionRangeFromRange(charRange, [self string]);
+  [super setSelectedRange:visualSelectionRange_
+                 affinity:affinity
+           stillSelecting:flag];
 
   // We're only interested in selection changes directly caused by keyboard
   // input from the user.
   if (interpretingKeyEvents_)
     textChangedByKeyEvents_ = YES;
 }
 
+- (NSRange)selectedRange {
+  return actualSelectionRange_;

[erikchen, 2016/10/12 22:37:48]

Is there any way we can avoid overriding this method? 

NSTextView has a lot of internal caching, and it's not clear to me that if we
override this method that all the caches also get updated appropriately. 

[lgrey, 2016/10/13 15:42:51]

We could expose a new method like |actualRange| or similar, but then we'd need
to do a cast here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/omnibox/omnibox_...
and here
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/location_bar/aut...

alternatively, the implementation could be something like:

NSRange visualRange = [super selectedRange];
if (!NSEqualRanges(visualRange, visualSelectionRange_)) {
  visualSelectionRange_ = actualSelectionRange_ = visualRange;
}
return actualSelectionRange_

Would that address the caching issue or is it at another level?

[erikchen, 2016/10/13 17:08:28]

If the only reason this code is necessary is to propagate information to other
Chrome classes, then we should definitely make a new method and do casts as
necessary.
No. My concern is that the internal implementation of NSTextView [which is very
complex and uses caching] could easily have code that calls [self selectedRange]
and assumes that this is the same value that got cached in the earlier call to
setSelectedRange:... Overriding getters to return a different value than the one
passed to the setter is really scary. I would be willing to go great lengths to
avoid that in general.

[lgrey, 2016/10/13 21:03:56]

Exposed actualSelectedRange_ as a property and used that instead. 

+}
+
 - (void)interpretKeyEvents:(NSArray *)eventArray {
   DCHECK(!interpretingKeyEvents_);
   interpretingKeyEvents_ = YES;
   textChangedByKeyEvents_ = NO;
   AutocompleteTextFieldObserver* observer = [self observer];
 
   if (observer)
     observer->OnBeforeChange();
 
   [super interpretKeyEvents:eventArray];
@@ skipping 132 matching lines @@
 // ThemedWindowDrawing implementation.
 
 - (void)windowDidChangeTheme {
   [self updateColorsToMatchTheme];
 }
 
 - (void)windowDidChangeActive {
 }
 
 @end