Update TextSelection for non-user initiated events

chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc

 // Copyright 2016 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 <vector>
 
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_client.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #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"
 #include "ui/base/ime/text_input_mode.h"
 #include "ui/base/ime/text_input_type.h"
 #include "url/gurl.h"
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 #include "ui/base/ime/linux/text_edit_command_auralinux.h"
 #include "ui/base/ime/linux/text_edit_key_bindings_delegate_auralinux.h"
@@ skipping 98 matching lines @@
         tester()->GetTextInputType(&type) ? type : ui::TEXT_INPUT_TYPE_NONE;
     if (expected_type_ == type)
       OnSuccess();
   }
 
   const ui::TextInputType expected_type_;
 
   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:
   explicit ViewTextInputTypeObserver(content::WebContents* web_contents,
                                      content::RenderWidgetHostView* rwhv,
                                      ui::TextInputType expected_type)
       : TextInputManagerObserverBase(web_contents),
         web_contents_(web_contents),
         view_(rwhv),
         expected_type_(expected_type) {
@@ skipping 94 matching lines @@
   const content::RenderWidgetHostView* const expected_view_;
   const size_t expected_length_;
 
   DISALLOW_COPY_AND_ASSIGN(ViewTextSelectionObserver);
 };
 
 // This class observes all the text selection updates within a WebContents.
 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);
 };
 
 // This class monitors all the changes in TextInputState and keeps a record of
 // the active views. There is no waiting and the recording process is
 // continuous.
 class RecordActiveViewsObserver {
  public:
   explicit RecordActiveViewsObserver(content::WebContents* web_contents)
@@ skipping 667 matching lines @@
   };
 
   for (auto* frame : frames) {
     focus_frame_and_input(frame);
     EXPECT_TRUE(active_contents()->IsFocusedElementEditable());
     active_contents()->ClearFocusedElement();
     EXPECT_FALSE(active_contents()->IsFocusedElementEditable());
   }
 }
 
+// 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).
 #if defined(USE_AURA)
 // -----------------------------------------------------------------------------
 // Input Method Observer Tests
 //
 // The following tests will make use of the InputMethodObserver to verify that
 // OOPIF pages interact properly with the InputMethod through the tab's view.
 
@@ skipping 374 matching lines @@
 
   // Closing this WebContents while we still hold on to our TestBrowserClient.
   EXPECT_TRUE(browser()->tab_strip_model()->CloseWebContentsAt(
       1, TabStripModel::CLOSE_USER_GESTURE));
 
   // For the cleanup of the original WebContents in tab index 0.
   content::SetBrowserClientForTesting(old_browser_client);
 }
 #endif  //  defined(MAC_OSX)
 #endif  // !defined(OS_ANDROID)