[Mac] TextSuggestionsTouchBarController Ignore Replacement Selection

When a user selects a suggestion, the editing word range is selected
for replacement with the suggestion's text. Currently, that selection
causes a call to WebContentsObserver::DidChangeTextSelection() which
updates the TextSuggestionsTouchBarController's text selection to the
editing word range. This change in text selection modifies the
appearance of the touch bar which is undesired.

The change in text selection for replacing the editing word should be
ignored by the TextSuggestionsTouchBarController. A variable,
|shouldIgnoreReplacementSelection_|, is set to YES when the
TextSuggestionsTouchBarController modifies the current text selection
to replace the editing word. When the that text selection update
reaches TextSuggestionsTouchBarController, the selection will be
ignored and |shouldIgnoreReplacementSelection_| will be reset to NO.
This fixes the behavior where the touch bar appearance changes before
showing suggestions post-replacement.

A browser test was added,
TextSuggestionsTouchBarControllerTest.IgnoreReplacementSelection, to
test that a text selection is only ignored when necessary and that
other selection updates are not ignored.

Bug: 717553
Change-Id: Ib442d98b96e499acd73856399bcd89adf0f9c1d9
Reviewed-on: https://chromium-review.googlesource.com/1164167Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Sarah Chan <spqchan@chromium.org>
Commit-Queue: Tessa Nijssen <tnijssen@google.com>
Cr-Commit-Position: refs/heads/master@{#581660}

chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.h

@@ -62,15 +62,17 @@ class Range;
 
 @interface TextSuggestionsTouchBarController (ExposedForTesting)
 
+- (void)setWebContents:(content::WebContents*)webContents;
+- (content::WebContents*)webContents;
 - (void)setText:(NSString*)text;
 - (NSString*)text;
+- (void)setSelectionRange:(const gfx::Range&)range;
+- (gfx::Range)selectionRange;
 - (void)setSuggestions:(NSArray*)suggestions;
 - (NSArray*)suggestions;
 - (WebTextfieldTouchBarController*)controller;
-- (void)setWebContents:(content::WebContents*)webContents;
-- (content::WebContents*)webContents;
-- (void)setSelectionRange:(const gfx::Range&)range;
-- (gfx::Range)selectionRange;
+- (void)setShouldIgnoreReplacementSelection:(BOOL)shouldIgnore;
+- (void)setEditingWordRange:(const gfx::Range&)range;
 
 @end
 

chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller.mm

@@ -72,6 +72,13 @@ class WebContentsTextObserver : public content::WebContentsObserver {
   // is no word currently being edited and the suggestion will be placed at the
   // cursor.
   NSRange editingWordRange_;
+
+  // When YES, -updateTextSelection:range: should ignore a text selection that
+  // is equal to the editing word range. Set to YES when
+  // -replaceEditingWordWithSuggestion: modifies the current text selection.
+  // Reset to NO when the text selection for replacing the editing word reaches
+  // -updateTextSelection:range:.
+  BOOL shouldIgnoreReplacementSelection_;
 }
 @end
 
@@ -89,6 +96,7 @@ class WebContentsTextObserver : public content::WebContentsObserver {
     controller_ = controller;
     observer_.reset(
         new text_observer::WebContentsTextObserver(webContents_, self));
+    shouldIgnoreReplacementSelection_ = NO;
   }
 
   return self;
@@ -147,6 +155,12 @@ class WebContentsTextObserver : public content::WebContentsObserver {
 - (void)updateTextSelection:(const base::string16&)text
                       range:(const gfx::Range&)range {
   if (@available(macOS 10.12.2, *)) {
+    if (shouldIgnoreReplacementSelection_ &&
+        range == gfx::Range(editingWordRange_)) {
+      shouldIgnoreReplacementSelection_ = NO;
+      return;
+    }
+
     if (![self isTextfieldFocused]) {
       [controller_ invalidateTouchBar];
       return;
@@ -218,6 +232,7 @@ class WebContentsTextObserver : public content::WebContentsObserver {
   // If the editing word is not selected in its entirety, modify the selection
   // to cover the current editing word.
   if (!NSEqualRanges(editingWordRange_, selectionRange_)) {
+    shouldIgnoreReplacementSelection_ = YES;
     int start_adjust = editingWordRange_.location - selectionRange_.location;
     int end_adjust = (editingWordRange_.location + editingWordRange_.length) -
                      (selectionRange_.location + selectionRange_.length);
@@ -227,26 +242,6 @@ class WebContentsTextObserver : public content::WebContentsObserver {
   webContents_->Replace(base::SysNSStringToUTF16(text));
 }
 
-- (void)setText:(NSString*)text {
-  text_.reset([text copy]);
-}
-
-- (NSString*)text {
-  return text_;
-}
-
-- (void)setSuggestions:(NSArray*)suggestions {
-  suggestions_.reset([suggestions copy]);
-}
-
-- (NSArray*)suggestions {
-  return suggestions_;
-}
-
-- (WebTextfieldTouchBarController*)controller {
-  return controller_;
-}
-
 - (void)setWebContents:(content::WebContents*)webContents {
   webContents_ = webContents;
   observer_->UpdateWebContents(webContents);
@@ -270,6 +265,14 @@ class WebContentsTextObserver : public content::WebContentsObserver {
   return webContents_;
 }
 
+- (void)setText:(NSString*)text {
+  text_.reset([text copy]);
+}
+
+- (NSString*)text {
+  return text_;
+}
+
 - (void)setSelectionRange:(const gfx::Range&)range {
   selectionRange_ = range.ToNSRange();
 }
@@ -278,4 +281,24 @@ class WebContentsTextObserver : public content::WebContentsObserver {
   return gfx::Range(selectionRange_);
 }
 
+- (void)setSuggestions:(NSArray*)suggestions {
+  suggestions_.reset([suggestions copy]);
+}
+
+- (NSArray*)suggestions {
+  return suggestions_;
+}
+
+- (WebTextfieldTouchBarController*)controller {
+  return controller_;
+}
+
+- (void)setShouldIgnoreReplacementSelection:(BOOL)shouldIgnore {
+  shouldIgnoreReplacementSelection_ = shouldIgnore;
+}
+
+- (void)setEditingWordRange:(const gfx::Range&)range {
+  editingWordRange_ = range.ToNSRange();
+}
+
 @end

chrome/browser/ui/cocoa/touchbar/text_suggestions_touch_bar_controller_browsertest.mm

@@ -185,11 +185,12 @@ IN_PROC_BROWSER_TEST_F(TextSuggestionsTouchBarControllerTest, SetWebContents) {
   NSString* const kText = @"text";
   const gfx::Range kRange = gfx::Range(1, 1);
 
+  FocusTextfield();
+
   // A null-pointer should not break the controller.
   [touch_bar_controller_ setWebContents:nullptr];
   EXPECT_FALSE([touch_bar_controller_ webContents]);
 
-  FocusTextfield();
   [touch_bar_controller_ setText:kText];
   [touch_bar_controller_ setSelectionRange:kRange];
 
@@ -206,4 +207,55 @@ IN_PROC_BROWSER_TEST_F(TextSuggestionsTouchBarControllerTest, SetWebContents) {
   }
 }
 
-}  // namespace
+// Tests that the selection created when replacing the editing word with a
+// suggestion is ignored.
+IN_PROC_BROWSER_TEST_F(TextSuggestionsTouchBarControllerTest,
+                       IgnoreReplacementSelection) {
+  NSString* const kText = @"text";
+  const base::string16 kEmptyText = base::string16();
+  const gfx::Range kRange = gfx::Range(0, 4);
+  const gfx::Range kEmptyRange = gfx::Range();
+
+  FocusTextfield();
+  [touch_bar_controller_ setText:kText];
+  [touch_bar_controller_ setSelectionRange:kRange];
+
+  // If ignoreReplacementSelection is YES and new selection range is equal to
+  // editing word range, ignore text selection update.
+  [touch_bar_controller_ setShouldIgnoreReplacementSelection:YES];
+  [touch_bar_controller_ setEditingWordRange:kEmptyRange];
+  [touch_bar_controller_ updateTextSelection:kEmptyText range:kEmptyRange];
+  EXPECT_STREQ(kText.UTF8String, [touch_bar_controller_ text].UTF8String);
+  EXPECT_EQ(kRange, [touch_bar_controller_ selectionRange]);
+
+  // If ignoreReplacementSelection is YES but new selection range is not equal
+  // to editing word range, do not ignore text selection update.
+  [touch_bar_controller_ setShouldIgnoreReplacementSelection:YES];
+  [touch_bar_controller_ setEditingWordRange:kRange];
+  [touch_bar_controller_ updateTextSelection:kEmptyText range:kEmptyRange];
+  if (@available(macOS 10.12.2, *)) {
+    EXPECT_STREQ("", [touch_bar_controller_ text].UTF8String);
+    EXPECT_EQ(gfx::Range(), [touch_bar_controller_ selectionRange]);
+  } else {
+    EXPECT_STREQ(kText.UTF8String, [touch_bar_controller_ text].UTF8String);
+    EXPECT_EQ(kRange, [touch_bar_controller_ selectionRange]);
+  }
+
+  [touch_bar_controller_ setText:kText];
+  [touch_bar_controller_ setSelectionRange:kRange];
+
+  // If ignoreReplacementSelection is NO and new selection range is equal to
+  // editing word range, do not ignore text selection update.
+  [touch_bar_controller_ setShouldIgnoreReplacementSelection:NO];
+  [touch_bar_controller_ setEditingWordRange:kEmptyRange];
+  [touch_bar_controller_ updateTextSelection:kEmptyText range:kEmptyRange];
+  if (@available(macOS 10.12.2, *)) {
+    EXPECT_STREQ("", [touch_bar_controller_ text].UTF8String);
+    EXPECT_EQ(gfx::Range(), [touch_bar_controller_ selectionRange]);
+  } else {
+    EXPECT_STREQ(kText.UTF8String, [touch_bar_controller_ text].UTF8String);
+    EXPECT_EQ(kRange, [touch_bar_controller_ selectionRange]);
+  }
+}
+
+}  // namespace
\ No newline at end of file