Reland: Update TextSelection for non-user initiated events

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 901eb2261a0b4512b4db7645600e3a2c2d0b0864..740ca73cfcfce4f54db8d8bb6392b3194ba7a46f 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1419,7 +1419,9 @@ void RenderFrameImpl::PepperSelectionChanged(
     PepperPluginInstanceImpl* instance) {
   if (instance != focused_pepper_plugin_)
     return;
-  SyncSelectionIfRequired();
+  // Keep the original behavior for pepper plugins and handle all selection
+  // change as user initiated.

[Changwan Ryu, 2017/07/14 18:44:03]

I'm not too familiar with the pepper and desktop use cases in general, but can't
this be caused both by IME change and by pepper's own commandline? Is this an
intermediate state or a final implementation?

[Peter Varga, 2017/07/17 09:36:06]

I'm also not familiar with pepper, that's why I keep the original behavior here.

+  SyncSelectionIfRequired(true);
 }
 
 RenderWidgetFullscreenPepper* RenderFrameImpl::CreatePepperFullscreenContainer(
@@ -2005,7 +2007,9 @@ void RenderFrameImpl::OnReplace(const base::string16& text) {
     frame_->SelectWordAroundCaret();
 
   frame_->ReplaceSelection(WebString::FromUTF16(text));
-  SyncSelectionIfRequired();
+  // Handle this selection change as user initiated since typically triggered
+  // from context menu.
+  SyncSelectionIfRequired(true);
 }
 
 void RenderFrameImpl::OnReplaceMisspelling(const base::string16& text) {
@@ -2707,9 +2711,11 @@ void RenderFrameImpl::DetachGuest(int element_instance_id) {
 
 void RenderFrameImpl::SetSelectedText(const base::string16& selection_text,
                                       size_t offset,
-                                      const gfx::Range& range) {
+                                      const gfx::Range& range,
+                                      bool user_initiated) {
   Send(new FrameHostMsg_SelectionChanged(routing_id_, selection_text,
-                                         static_cast<uint32_t>(offset), range));
+                                         static_cast<uint32_t>(offset), range,
+                                         user_initiated));
 }
 
 void RenderFrameImpl::EnsureMojoBuiltinsAreAvailable(
@@ -4087,9 +4093,20 @@ void RenderFrameImpl::AbortClientNavigation() {
 }
 
 void RenderFrameImpl::DidChangeSelection(bool is_empty_selection) {
-  if (!GetRenderWidget()->input_handler().handling_input_event() &&
-      !handling_select_range_)
-    return;
+  bool user_initiated =
+      GetRenderWidget()->input_handler().handling_input_event() ||
+      handling_select_range_;
+
+  if (!user_initiated) {
+    // Do not update text input state unnecessarily when text selection remains
+    // empty.
+    if (is_empty_selection && selection_text_.empty())

[Changwan Ryu, 2017/07/14 18:44:03]

What happens if move the cursor from one position to another from JS?
Could you add a test case to ensure that we still update text input state and
sync selection in that case?

[Peter Varga, 2017/07/17 09:36:06]

I assume you mean to change cursor position by empty text selection, eg.
input.setSelectionRange(pos, pos). Makes sense, I will check this.

+      return;
+
+    // Ignore selection change of text replacement triggered by IME composition.
+    if (GetRenderWidget()->input_handler().ime_composition_replacement())

[Changwan Ryu, 2017/07/14 18:44:03]

Hmm... This approach is quite brittle. SetComposingText() is not the only place
where intermediate selection changes can be made - see
InputMethodController.cpp. You will need to apply it to every IME operation.

When I come to think about it, it's quite unfortunate that Android's and
desktop's selection update paths are completely different.

On Android, ImeEventGuard + UpdateTextInputState() should already prevent
propagation of all the intermediate selection updates.

Is there any way we can remove SyncSelectionIfRequired() and merge it into
UpdateTextInputState path? Then we wouldn't need this sort of divergence at all.

Failing that, is it possible to call SyncSelectionIfRequired() inside
UpdateTextInputState()?

[EhsanK, 2017/07/14 18:55:21]

I think this is something worth pursing. The potential problems I see are:

1- SyncSelectionIfRequired is a frame concept while TextInputState is a
WebFrameWidget thing. So if a frame is not focused, it does not contribute to
TextInputState but it can still update its selection. This is something to
consider.

2- Minor issue but selection uses string16 whereas text input state uses
std::string. I am not sure if there is any complications to migrate one to
another.

3- Are TextInputState's selection values still relevant when we have highlighted
text in an non-editable region?

[Peter Varga, 2017/07/17 09:36:06]

I try to handle this in render_widget.cc. Only unnecessary intermediate
selection that I found so far is the replacement of a composing text. Other
cases are already handled as an user input event by
input_handler_->set_handling_input_event(true).
I agree with this. I tried to point to this in a comment when I did the first
reland patch with the android skip.
 
It might be possible but as far as I see UpdateTextInputState is not called
before SyncSelectionIfRequired in other cases (pepper, OnReplace). I assume that
in other cases UpdateTextInputState is called in another code path. Implementing
this would raise new risks that needs more investigation and testing. Currently
I don't have too much time for this. I consider the current solution less likely
to introduce regressions. Could it be possible to do this "refactor" in a
subsequent patch by someone else?

+      return;
+  }
 
   if (is_empty_selection)
     selection_text_.clear();
@@ -4100,7 +4117,7 @@ void RenderFrameImpl::DidChangeSelection(bool is_empty_selection) {
   // to notify the selection was changed.  Focus change should be notified
   // before selection change.
   GetRenderWidget()->UpdateTextInputState();
-  SyncSelectionIfRequired();
+  SyncSelectionIfRequired(user_initiated);
 }
 
 bool RenderFrameImpl::HandleCurrentKeyboardEvent() {
@@ -6210,10 +6227,10 @@ void RenderFrameImpl::UpdateEncoding(WebFrame* frame,
     Send(new FrameHostMsg_UpdateEncoding(routing_id_, encoding_name));
 }
 
-void RenderFrameImpl::SyncSelectionIfRequired() {
+void RenderFrameImpl::SyncSelectionIfRequired(bool user_initiated) {
   base::string16 text;
-  size_t offset;
-  gfx::Range range;
+  size_t offset = 0;
+  gfx::Range range = gfx::Range::InvalidRange();
 #if BUILDFLAG(ENABLE_PLUGINS)
   if (focused_pepper_plugin_) {
     focused_pepper_plugin_->GetSurroundingText(&text, &range);
@@ -6224,32 +6241,37 @@ void RenderFrameImpl::SyncSelectionIfRequired() {
   {
     WebRange selection =
         GetRenderWidget()->GetWebWidget()->CaretOrSelectionRange();
-    if (selection.IsNull())
-      return;
 
-    range = gfx::Range(selection.StartOffset(), selection.EndOffset());
-
-    if (frame_->GetInputMethodController()->TextInputType() !=
-        blink::kWebTextInputTypeNone) {
-      // If current focused element is editable, we will send 100 more chars
-      // before and after selection. It is for input method surrounding text
-      // feature.
-      if (selection.StartOffset() > kExtraCharsBeforeAndAfterSelection)
-        offset = selection.StartOffset() - kExtraCharsBeforeAndAfterSelection;
-      else
-        offset = 0;
-      size_t length =
-          selection.EndOffset() - offset + kExtraCharsBeforeAndAfterSelection;
-      text = frame_->RangeAsText(WebRange(offset, length)).Utf16();
-    } else {
-      offset = selection.StartOffset();
-      text = frame_->SelectionAsText().Utf16();
-      // http://crbug.com/101435
-      // In some case, frame->selectionAsText() returned text's length is not
-      // equal to the length returned from
-      // GetWebWidget()->caretOrSelectionRange().
-      // So we have to set the range according to text.length().
-      range.set_end(range.start() + text.length());
+    // When clearing text selection from JavaScript the selection range
+    // might be null but the selected text still have to be updated.
+    // Do not cancel sync selection if the clear was not user initiated.
+    if (!selection.IsNull()) {
+      range = gfx::Range(selection.StartOffset(), selection.EndOffset());
+
+      if (frame_->GetInputMethodController()->TextInputType() !=
+          blink::kWebTextInputTypeNone) {
+        // If current focused element is editable, we will send 100 more chars
+        // before and after selection. It is for input method surrounding text
+        // feature.
+        if (selection.StartOffset() > kExtraCharsBeforeAndAfterSelection)
+          offset = selection.StartOffset() - kExtraCharsBeforeAndAfterSelection;
+        else
+          offset = 0;
+        size_t length =
+            selection.EndOffset() - offset + kExtraCharsBeforeAndAfterSelection;
+        text = frame_->RangeAsText(WebRange(offset, length)).Utf16();
+      } else {
+        offset = selection.StartOffset();
+        text = frame_->SelectionAsText().Utf16();
+        // http://crbug.com/101435
+        // In some case, frame->selectionAsText() returned text's length is not
+        // equal to the length returned from
+        // GetWebWidget()->caretOrSelectionRange().
+        // So we have to set the range according to text.length().
+        range.set_end(range.start() + text.length());
+      }
+    } else if (user_initiated) {
+      return;
     }
   }
 
@@ -6265,7 +6287,7 @@ void RenderFrameImpl::SyncSelectionIfRequired() {
     selection_text_ = text;
     selection_text_offset_ = offset;
     selection_range_ = range;
-    SetSelectedText(text, offset, range);
+    SetSelectedText(text, offset, range, user_initiated);
   }
   GetRenderWidget()->UpdateSelectionBounds();
 }