SPM: Fix ExtensionViewHost key event not working
The problem is similar to WebDialogView in https://crbug.com/919183
that ExtensionViewHost does return a proper "handled/procssed" flag
for the unhandled key events. The CL makes it to return the flag
from ExtensionView instead of always returning true.
Bug: 920713
Change-Id: I9ee61c0ba5b390e43a20fce38d7393e26fe3d847
Reviewed-on: https://chromium-review.googlesource.com/c/1407196
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622513}diff --git a/chrome/browser/extensions/extension_view_host.cc b/chrome/browser/extensions/extension_view_host.cc
index 56573ba..31c170e 100644
--- a/chrome/browser/extensions/extension_view_host.cc
+++ b/chrome/browser/extensions/extension_view_host.cc
@@ -107,10 +107,10 @@
}
}
-void ExtensionViewHost::UnhandledKeyboardEvent(
+bool ExtensionViewHost::UnhandledKeyboardEvent(
WebContents* source,
const content::NativeWebKeyboardEvent& event) {
- view_->HandleKeyboardEvent(source, event);
+ return view_->HandleKeyboardEvent(source, event);
}
// ExtensionHost overrides:
@@ -203,8 +203,7 @@
return true;
}
}
- UnhandledKeyboardEvent(source, event);
- return true;
+ return UnhandledKeyboardEvent(source, event);
}
bool ExtensionViewHost::PreHandleGestureEvent(
diff --git a/chrome/browser/extensions/extension_view_host.h b/chrome/browser/extensions/extension_view_host.h
index fb034c1..d86583f 100644
--- a/chrome/browser/extensions/extension_view_host.h
+++ b/chrome/browser/extensions/extension_view_host.h
@@ -54,8 +54,8 @@
// Handles keyboard events that were not handled by HandleKeyboardEvent().
// Platform specific implementation may override this method to handle the
- // event in platform specific way.
- virtual void UnhandledKeyboardEvent(
+ // event in platform specific way. Returns whether the events are handled.
+ virtual bool UnhandledKeyboardEvent(
content::WebContents* source,
const content::NativeWebKeyboardEvent& event);
diff --git a/chrome/browser/ui/views/content_test_utils.cc b/chrome/browser/ui/views/content_test_utils.cc
new file mode 100644
index 0000000..ac5d7b9
--- /dev/null
+++ b/chrome/browser/ui/views/content_test_utils.cc
@@ -0,0 +1,61 @@
+// Copyright 2019 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 "chrome/browser/ui/views/content_test_utils.h"
+
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "content/public/test/browser_test_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/ui_base_features.h"
+#include "ui/events/test/event_generator.h"
+
+#if defined(USE_AURA)
+#include "ui/aura/window.h"
+#endif // defined(USE_AURA)
+
+namespace {
+
+// Helper to use inside a loop instead of using RunLoop::RunUntilIdle() to avoid
+// the loop being a busy loop that prevents renderer from doing its job. Use
+// only when there is no better way to synchronize.
+void GiveItSomeTime(base::TimeDelta delta) {
+ base::RunLoop run_loop;
+ base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+ FROM_HERE, run_loop.QuitClosure(), delta);
+ run_loop.Run();
+}
+
+} // namespace
+
+void TestTextInputViaKeyEvent(content::WebContents* contents) {
+ // Replace the dialog content with a single text input element and focus it.
+ ASSERT_TRUE(content::WaitForLoadStop(contents));
+ ASSERT_TRUE(content::ExecuteScript(contents, R"(
+ document.body.innerHTML = '<input type="text" id="text-id">';
+ document.getElementById('text-id').focus();
+ )"));
+
+ // Generate a key event for 'a'.
+ gfx::NativeWindow event_window = contents->GetTopLevelNativeWindow();
+#if defined(USE_AURA)
+ event_window = event_window->GetRootWindow();
+#endif
+ if (features::IsUsingWindowService())
+ event_window = nullptr;
+
+ ui::test::EventGenerator generator(event_window);
+ generator.PressKey(ui::VKEY_A, ui::EF_NONE);
+
+ // Verify text input is updated.
+ std::string result;
+ while (result != "a") {
+ GiveItSomeTime(base::TimeDelta::FromMilliseconds(100));
+
+ ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents, R"(
+ window.domAutomationController.send(
+ document.getElementById('text-id').value);
+ )", &result));
+ }
+}
diff --git a/chrome/browser/ui/views/content_test_utils.h b/chrome/browser/ui/views/content_test_utils.h
new file mode 100644
index 0000000..b3b3070
--- /dev/null
+++ b/chrome/browser/ui/views/content_test_utils.h
@@ -0,0 +1,18 @@
+// Copyright 2019 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_CONTENT_TEST_UTILS_H_
+#define CHROME_BROWSER_UI_VIEWS_CONTENT_TEST_UTILS_H_
+
+#include "base/time/time.h"
+
+namespace content {
+class WebContents;
+}
+
+// Tests that text input in |contents| can be updated via key events. Note this
+// function is destructive and replaces the page in |contents| with a input.
+void TestTextInputViaKeyEvent(content::WebContents* contents);
+
+#endif // CHROME_BROWSER_UI_VIEWS_CONTENT_TEST_UTILS_H_
diff --git a/chrome/browser/ui/views/extensions/extension_dialog_browsertest.cc b/chrome/browser/ui/views/extensions/extension_dialog_browsertest.cc
new file mode 100644
index 0000000..9e5ea4c
--- /dev/null
+++ b/chrome/browser/ui/views/extensions/extension_dialog_browsertest.cc
@@ -0,0 +1,38 @@
+// Copyright 2019 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 "chrome/browser/ui/views/extensions/extension_dialog.h"
+
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_view_host.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/views/content_test_utils.h"
+#include "chrome/browser/ui/views/extensions/extension_dialog.h"
+#include "extensions/test/extension_test_message_listener.h"
+
+namespace {
+
+using ExtensionDialogTest = extensions::ExtensionBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(ExtensionDialogTest, TextInputViaKeyEvent) {
+ ExtensionTestMessageListener init_listener("ready", /*will_reply=*/false);
+
+ scoped_refptr<const extensions::Extension> extension =
+ LoadExtension(test_data_dir_.AppendASCII("uitest/tab_traversal"));
+ ASSERT_TRUE(extension.get());
+
+ constexpr int kDialogWidth = 400;
+ constexpr int kDialogHeight = 300;
+ auto* dialog = ExtensionDialog::Show(
+ extension->url().Resolve("main.html"),
+ browser()->window()->GetNativeWindow(), browser()->profile(),
+ /*web_contents=*/nullptr, /*is_modal=*/true, kDialogWidth, kDialogHeight,
+ kDialogWidth, kDialogHeight, base::string16(), /*observer=*/nullptr);
+ ASSERT_TRUE(dialog);
+ ASSERT_TRUE(init_listener.WaitUntilSatisfied());
+
+ TestTextInputViaKeyEvent(dialog->host()->host_contents());
+}
+
+} // namespace
diff --git a/chrome/browser/ui/views/web_dialog_view_browsertest.cc b/chrome/browser/ui/views/web_dialog_view_browsertest.cc
index 10f4834..7fab17d 100644
--- a/chrome/browser/ui/views/web_dialog_view_browsertest.cc
+++ b/chrome/browser/ui/views/web_dialog_view_browsertest.cc
@@ -14,6 +14,7 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/content_test_utils.h"
#include "chrome/browser/ui/webui/chrome_web_contents_handler.h"
#include "chrome/common/url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
@@ -21,18 +22,11 @@
#include "content/public/browser/browser_context.h"
#include "content/public/browser/render_widget_host_view.h"
#include "content/public/browser/web_contents.h"
-#include "content/public/test/browser_test_utils.h"
#include "content/public/test/test_utils.h"
-#include "ui/base/ui_base_features.h"
-#include "ui/events/test/event_generator.h"
#include "ui/views/controls/webview/web_dialog_view.h"
#include "ui/views/widget/widget.h"
#include "ui/web_dialogs/test/test_web_dialog_delegate.h"
-#if defined(USE_AURA)
-#include "ui/aura/window.h"
-#endif // defined(USE_AURA)
-
namespace {
// Initial size of WebDialog for SizeWindow test case. Note the height must be
@@ -248,39 +242,5 @@
// Test that key event is translated to a text input properly.
IN_PROC_BROWSER_TEST_F(WebDialogBrowserTest, TextInputViaKeyEvent) {
- // Replace the dialog content with a single text input element and focus it.
- content::WebContents* contents = view_->web_contents();
- ASSERT_TRUE(content::WaitForLoadStop(contents));
- ASSERT_TRUE(content::ExecuteScript(contents, R"(
- document.body.innerHTML = '<input type="text" id="text-id">';
- document.getElementById('text-id').focus();
- )"));
-
- // Generate a key event for 'a'.
- gfx::NativeWindow event_window = view_->GetWidget()->GetNativeWindow();
-#if defined(USE_AURA)
- event_window = event_window->GetRootWindow();
-#endif
- if (features::IsUsingWindowService())
- event_window = nullptr;
-
- ui::test::EventGenerator generator(event_window);
- generator.PressKey(ui::VKEY_A, ui::EF_NONE);
-
- // Verify text input is updated.
- std::string result;
- while (result != "a") {
- // Use PostDelayedTask instead of RunLoop::RunUntilIdle() to avoid being
- // a busy loop that prevents renderer doing its job in time.
- base::RunLoop run_loop;
- base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
- FROM_HERE, run_loop.QuitClosure(),
- base::TimeDelta::FromMilliseconds(100));
- run_loop.Run();
-
- ASSERT_TRUE(content::ExecuteScriptAndExtractString(contents, R"(
- window.domAutomationController.send(
- document.getElementById('text-id').value);
- )", &result));
- }
+ TestTextInputViaKeyEvent(view_->web_contents());
}
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index 572551d..75329b82 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -949,6 +949,8 @@
"../browser/ui/views/chrome_cleaner_dialog_browsertest_win.cc",
"../browser/ui/views/chrome_cleaner_reboot_dialog_browsertest_win.cc",
"../browser/ui/views/content_setting_bubble_contents_browsertest.cc",
+ "../browser/ui/views/content_test_utils.cc",
+ "../browser/ui/views/content_test_utils.h",
"../browser/ui/views/device_chooser_browsertest.cc",
"../browser/ui/views/hats/hats_browsertest.cc",
"../browser/ui/views/try_chrome_dialog_win/try_chrome_dialog_browsertest.cc",
@@ -1470,6 +1472,7 @@
"../browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model_browsertest_win.cc",
"../browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_test_utils.cc",
"../browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_test_utils.h",
+ "../browser/ui/views/extensions/extension_dialog_browsertest.cc",
"../browser/ui/webui/media_router/media_router_dialog_controller_webui_impl_browsertest.cc",
]
