Reland: Update TextSelection for non-user initiated events

content/renderer/render_frame_impl.cc

 // Copyright 2013 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.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <algorithm>
 #include <map>
 #include <string>
 #include <utility>
@@ skipping 1401 matching lines @@
 #if defined(OS_MACOSX) || defined(USE_AURA)
   GetRenderWidget()->UpdateCompositionInfo(
       false /* not an immediate request */);
 #endif
 }
 
 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(
     PepperPluginInstanceImpl* plugin) {
   GURL active_url;
   if (render_view()->webview())
     active_url = render_view()->GetURLForGraphicsContext3D();
 
   // Synchronous IPC to obtain a routing id for the fullscreen widget.
   int32_t fullscreen_widget_routing_id = MSG_ROUTING_NONE;
@@ skipping 565 matching lines @@
   base::AutoReset<bool> handling_select_range(&handling_select_range_, true);
   frame_->MoveRangeSelectionExtent(
       render_view_->ConvertWindowPointToViewport(point));
 }
 
 void RenderFrameImpl::OnReplace(const base::string16& text) {
   if (!frame_->HasSelection())
     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) {
   if (!frame_->HasSelection())
     return;
 
   frame_->ReplaceMisspelledRange(WebString::FromUTF16(text));
 }
 
 void RenderFrameImpl::OnCopyImageAt(int x, int y) {
@@ skipping 681 matching lines @@
 void RenderFrameImpl::AttachGuest(int element_instance_id) {
   BrowserPluginManager::Get()->Attach(element_instance_id);
 }
 
 void RenderFrameImpl::DetachGuest(int element_instance_id) {
   BrowserPluginManager::Get()->Detach(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(
     v8::Isolate* isolate,
     v8::Local<v8::Context> context) {
   gin::ModuleRegistry* registry = gin::ModuleRegistry::From(context);
   if (registry->available_modules().count(mojo::edk::js::Core::kModuleName))
     return;
 
   v8::HandleScope handle_scope(isolate);
@@ skipping 1357 matching lines @@
     return false;
   }
   return true;
 }
 
 void RenderFrameImpl::AbortClientNavigation() {
   Send(new FrameHostMsg_AbortNavigation(routing_id_));
 }
 
 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();
 
   // UpdateTextInputState should be called before SyncSelectionIfRequired.
   // UpdateTextInputState may send TextInputStateChanged to notify the focus
   // was changed, and SyncSelectionIfRequired may send SelectionChanged
   // to notify the selection was changed.  Focus change should be notified
   // before selection change.
   GetRenderWidget()->UpdateTextInputState();
-  SyncSelectionIfRequired();
+  SyncSelectionIfRequired(user_initiated);
 }
 
 bool RenderFrameImpl::HandleCurrentKeyboardEvent() {
   bool did_execute_command = false;
   for (auto command : GetRenderWidget()->edit_commands()) {
     // In gtk and cocoa, it's possible to bind multiple edit commands to one
     // key (but it's the exception). Once one edit command is not executed, it
     // seems safest to not execute the rest.
     if (!frame_->ExecuteCommand(blink::WebString::FromUTF8(command.name),
                                 blink::WebString::FromUTF8(command.value)))
@@ skipping 2089 matching lines @@
   }
 }
 
 void RenderFrameImpl::UpdateEncoding(WebFrame* frame,
                                      const std::string& encoding_name) {
   // Only update main frame's encoding_name.
   if (!frame->Parent())
     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);
     offset = 0;  // Pepper API does not support offset reporting.
     // TODO(kinaba): cut as needed.
   } else
 #endif
   {
     WebRange selection =
         GetRenderWidget()->GetWebWidget()->CaretOrSelectionRange();
-    if (selection.IsNull())
+
+    // 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;
-
-    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());
     }
   }
 
   // TODO(dglazkov): Investigate if and why this would be happening,
   // and resolve this. We shouldn't be carrying selection text here.
   // http://crbug.com/632920.
   // Sometimes we get repeated didChangeSelection calls from webkit when
   // the selection hasn't actually changed. We don't want to report these
   // because it will cause us to continually claim the X clipboard.
   if (selection_text_offset_ != offset ||
       selection_range_ != range ||
       selection_text_ != text) {
     selection_text_ = text;
     selection_text_offset_ = offset;
     selection_range_ = range;
-    SetSelectedText(text, offset, range);
+    SetSelectedText(text, offset, range, user_initiated);
   }
   GetRenderWidget()->UpdateSelectionBounds();
 }
 
 void RenderFrameImpl::InitializeUserMediaClient() {
   RenderThreadImpl* render_thread = RenderThreadImpl::current();
   if (!render_thread)  // Will be NULL during unit tests.
     return;
 
 #if BUILDFLAG(ENABLE_WEBRTC)
@@ skipping 614 matching lines @@
       replaces_current_history_item(info.replaces_current_history_item),
       history_navigation_in_new_child_frame(
           info.is_history_navigation_in_new_child_frame),
       client_redirect(info.is_client_redirect),
       triggering_event_info(info.triggering_event_info),
       cache_disabled(info.is_cache_disabled),
       form(info.form),
       source_location(info.source_location) {}
 
 }  // namespace content