Update TextSelection for non-user initiated events

chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc

Index: chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
diff --git a/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc b/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
index a06446b7ac41606c3d191efc851e72b01f240e52..4fe3679e54d69394a6ed35ec515832f8225dfefe 100644
--- a/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
+++ b/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
@@ -24,6 +24,7 @@
 #include "content/public/test/text_input_test_utils.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "ui/base/clipboard/clipboard.h"
 #include "ui/base/ime/composition_underline.h"
 #include "ui/base/ime/text_edit_commands.h"
 #include "ui/base/ime/text_input_client.h"
@@ -142,24 +143,6 @@ class TextInputManagerTypeObserver : public TextInputManagerObserverBase {
   DISALLOW_COPY_AND_ASSIGN(TextInputManagerTypeObserver);
 };
 
-// This class observes TextInputManager for the first change in TextInputState.
-class TextInputManagerChangeObserver : public TextInputManagerObserverBase {
- public:
-  explicit TextInputManagerChangeObserver(content::WebContents* web_contents)
-      : TextInputManagerObserverBase(web_contents) {
-    tester()->SetUpdateTextInputStateCalledCallback(base::Bind(
-        &TextInputManagerChangeObserver::VerifyChange, base::Unretained(this)));
-  }
-
- private:
-  void VerifyChange() {
-    if (tester()->IsTextInputStateChanged())
-      OnSuccess();
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(TextInputManagerChangeObserver);
-};
-
 // This class observes |TextInputState.type| for a specific RWHV.
 class ViewTextInputTypeObserver : public TextInputManagerObserverBase {
  public:
@@ -274,25 +257,44 @@ class ViewTextSelectionObserver : public TextInputManagerObserverBase {
 class TextSelectionObserver : public TextInputManagerObserverBase {
  public:
   explicit TextSelectionObserver(content::WebContents* web_contents)
-      : TextInputManagerObserverBase(web_contents) {
+      : TextInputManagerObserverBase(web_contents), callback_called_(false) {
     tester()->SetOnTextSelectionChangedCallback(base::Bind(
         &TextSelectionObserver::VerifyChange, base::Unretained(this)));
   }
 
-  void WaitForSelectedText(const std::string& text) {
-    selected_text_ = text;
-    Wait();
+  void WaitForSelectedText(const std::string& text,
+                           bool user_initiated = true) {
+    expected_text_ = text;
+    expected_user_initiated_ = user_initiated;
+    if (callback_called_ && last_selected_text_ == expected_text_ &&
+        last_user_initiated_ == expected_user_initiated_) {
+      OnSuccess();
+    } else {
+      Wait();
+    }
   }
 
  private:
   void VerifyChange() {
-    if (base::UTF16ToUTF8(tester()->GetUpdatedView()->GetSelectedText()) ==
-        selected_text_) {
+    callback_called_ = true;
+
+    last_selected_text_ =
+        base::UTF16ToUTF8(tester()->GetUpdatedView()->GetSelectedText());
+    EXPECT_TRUE(tester()->GetTextSelectionUserInitiatedForView(
+        tester()->GetUpdatedView(), &last_user_initiated_));
+
+    if (last_selected_text_ == expected_text_ &&
+        last_user_initiated_ == expected_user_initiated_) {
       OnSuccess();
     }
   }
 
-  std::string selected_text_;
+  bool callback_called_;
+
+  std::string last_selected_text_;
+  std::string expected_text_;
+  bool last_user_initiated_;

[Lei Zhang, 2017/05/17 19:23:05]

Should these bools be initialized in the ctor?

[Peter Varga, 2017/05/18 08:27:20]

They are initialized to false this way and I don't see any reason to initialize
them with a particular value in the constructor.

|expected_user_initiated_| is initialized in the WaitForSelectedText function
and I don't see any other use case of this property.

|last_user_initiated_| is initialized when the TextSelectionChangedCallback is
called. If the callback is not called this property is not used thus it doesn't
matter whether it is true or false. This is guaranteed by the |callback_called_|
property what is initialized in the constructor.

[Lei Zhang, 2017/05/18 18:13:46]

Can VerifyChange() get called before the first WaitForSelectedText() call? If
yes, and |last_selected_text_| ends up as an empty string, then the code will
try to read an uninitialized |expected_user_initiated_|. Can this scenario
occur?
Maybe combine |callback_called_| and |last_user_initiated_| a
base::Optional<bool> since it's really a tri-state? [not called, called/false,
called/true]

[Peter Varga, 2017/05/19 08:50:01]

hmm, yes it can.
 
I wasn't familiar with this base::Optional solution but sounds better than use
the |callback_called_|. However, I would rather make the |last_selected_text_|
property optional due to simpler value assignment in VerifyChange(). Thank you
for pointing this out.

+  bool expected_user_initiated_;
 
   DISALLOW_COPY_AND_ASSIGN(TextSelectionObserver);
 };
@@ -980,6 +982,124 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
   }
 }
 
+// The following tests verify that the TextInputManager notifies about a
+// about text selection change event, the corresponding |user_initiated|
+// property of TextSelection is valid, and the selection clipboard is updated
+// according to the source of the event.
+// See: https://crbug.com/671986
+
+// Test text selection change event for non-user initiated cases
+// (eg. JavaScript). Non-user initiated events should not update the selection
+// clipboard.
+IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
+                       NonUserInitiatedTextSelection) {
+#if defined(USE_AURA) && defined(OS_LINUX) && !defined(OS_CHROMEOS)
+  ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread();
+  ASSERT_TRUE(!!clipboard);

[Lei Zhang, 2017/05/17 19:23:05]

I think ASSERT_TRUE() works with the "!!"

[Peter Varga, 2017/05/18 08:27:20]

I don't really understand what is wrong with this line, but yes it works with
the "!!" :)

If you mean it works without the "!!" then I don't see any reason to remove it.
I copied this from another test and based on the doc in gtest.h, ASSERT_TRUE
expects boolean expression.

[Lei Zhang, 2017/05/18 18:13:46]

I just wanted to point out that you can omit the "!!" and accomplish the same
thing. Just because some other piece of code does something does not always mean
it's always the best idea. Let's take another situation where a statement
expects a boolean expression:

void* ptr = MayReturnNullptrFunc();
if (...) {
  DoSomething();
}

Do you write if (ptr), or if (!!ptr) ?

[Peter Varga, 2017/05/19 08:50:01]

I would rather write if (ptr) :) Since I haven't found any suggestion about
coding conventions in this case I tried to work from other tests, hoping that
the same solution doesn't break any style/code convention. Removing "!!" doesn't
seem to me risky either, so let it be removed.

+  clipboard->Clear(ui::CLIPBOARD_TYPE_SELECTION);
+#endif
+
+  CreateIframePage("a(b, c)");
+  std::vector<std::string> values{"node_a", "node_b", "node_c"};
+  std::vector<content::RenderFrameHost*> frames{GetFrame(IndexVector{}),
+                                                GetFrame(IndexVector{0}),
+                                                GetFrame(IndexVector{1})};
+
+  for (size_t i = 0; i < frames.size(); ++i)
+    AddInputFieldToFrame(frames[i], "text", values[i], true);
+
+  // Test text selection from JavaScript across frames (non-user initiated).
+  for (size_t i = 0; i < frames.size(); ++i) {
+    // Trigger text selection.
+    TextSelectionObserver trigger_observer(active_contents());
+    EXPECT_TRUE(
+        ExecuteScript(frames[i], "document.querySelector('input').select();"));
+    trigger_observer.WaitForSelectedText(values[i], false);
+
+#if defined(USE_AURA) && defined(OS_LINUX) && !defined(OS_CHROMEOS)
+    // Non-user initiated text selection should not update the selection
+    // clipboard. See: https://crbug.com/12392
+    base::string16 result_text;
+    clipboard->ReadText(ui::CLIPBOARD_TYPE_SELECTION, &result_text);
+    EXPECT_TRUE(result_text.empty());
+#endif
+
+    // Clear text selection.
+    TextSelectionObserver clear_observer(active_contents());
+    EXPECT_TRUE(ExecuteScript(frames[i], "document.getSelection().empty();"));
+    clear_observer.WaitForSelectedText("", false);
+  }
+}
+
+// Test text selection change event for user initiated cases (eg. key press)
+// User initiated events should update the selection clipboard where it is
+// supported.
+IN_PROC_BROWSER_TEST_F(SitePerProcessTextInputManagerTest,
+                       UserInitiatedTextSelection) {
+#if defined(USE_AURA) && defined(OS_LINUX) && !defined(OS_CHROMEOS)
+  ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread();
+  ASSERT_TRUE(!!clipboard);
+  clipboard->Clear(ui::CLIPBOARD_TYPE_SELECTION);
+#endif
+
+  CreateIframePage("a(b, c)");
+  std::vector<std::string> values{"node_a", "node_b", "node_c"};
+  std::vector<content::RenderFrameHost*> frames{GetFrame(IndexVector{}),
+                                                GetFrame(IndexVector{0}),
+                                                GetFrame(IndexVector{1})};
+
+  for (size_t i = 0; i < frames.size(); ++i)
+    AddInputFieldToFrame(frames[i], "text", values[i], true);
+
+  // Test text selection by user input across frames (user initiated).
+  for (size_t i = 0; i < frames.size(); ++i) {
+    // Focus on input element.
+    std::string result;
+    std::string script =
+        "function getInputField() {"
+        "  return document.querySelector('input');"
+        "}"
+        "function onInputFocus(e) {"
+        "  domAutomationController.setAutomationId(0);"
+        "  domAutomationController.send(getInputField().value);"
+        "}"
+        "getInputField().addEventListener('focus', onInputFocus);";
+    EXPECT_TRUE(ExecuteScript(frames[i], script));
+    EXPECT_TRUE(ExecuteScriptAndExtractString(
+        frames[i], "window.focus(); document.querySelector('input').focus();",
+        &result));
+    EXPECT_EQ(values[i], result);
+    EXPECT_EQ(frames[i], active_contents()->GetFocusedFrame());
+
+    // Press ctrl+a to select text in the input field.
+    TextSelectionObserver trigger_observer(active_contents());
+#if !defined(OS_MACOSX)
+    SimulateKeyPress(active_contents(), ui::DomKey::FromCharacter('A'),
+                     ui::DomCode::US_A, ui::VKEY_A, true, false, false, false);
+#else
+    // On macOS the select all shortcut (Cmd+A) is handled by the browser via
+    // keyboard accelerator. Thus we can't simulate key press on WebContents.
+    // As a workaround, call the SelectAll directly as the shortcut would do.
+    active_contents()->SelectAll();
+#endif
+    trigger_observer.WaitForSelectedText(values[i], true);
+
+#if defined(USE_AURA) && defined(OS_LINUX) && !defined(OS_CHROMEOS)
+    // User initiated text selection should update the selection clipboard.
+    base::string16 result_text;
+    clipboard->ReadText(ui::CLIPBOARD_TYPE_SELECTION, &result_text);
+    EXPECT_EQ(base::ASCIIToUTF16(values[i]), result_text);
+#endif
+
+    // Press down key to clear text selection.
+    TextSelectionObserver clear_observer(active_contents());
+    SimulateKeyPress(active_contents(), ui::DomKey::ARROW_DOWN,
+                     ui::DomCode::ARROW_DOWN, ui::VKEY_DOWN, false, false,
+                     false, false);
+    clear_observer.WaitForSelectedText("", true);
+  }
+}
+
 // TODO(ekaramad): The following tests are specifically written for Aura and are
 // based on InputMethodObserver. Write similar tests for Mac/Android/Mus
 // (crbug.com/602723).