Ash: Dispatch accelerators correctly in Tablet mode.

This is basically a "revert" of r155769, which landed 6 years ago.

Problem: The hardware play/pause key currently stops working in the CrOS
Audio Player in tablet mode. The event is not dispatched to the web
contents. Note an external keyboard or USB headset can send this key
event in tablet mode, even though events from a convertible's regular
keyboard are suppressed.

r155769 introduced logic that causes all accelerators to be processed as
though they have the same priority as, e.g., Alt+Tab when the AppList is
visible. Now, in tablet mode, the AppList _always_ declares itself
visible. So currently no key combinations bound to Ash accelerators are
dispatched to webcontents when in tablet mode. This is bad.

r155769 was to fix https://crbug.com/142067 - "Ctrl+Space unable to
switch keyboard layouts in the AppList". This occurred because an
AppList button had focus, not the text field. To retain current
behaviour, instead update the two Button subclasses to return false for
SkipDefaultKeyEventProcessing(), thereby prioritizing accelerators over
their 'space' activation. There are other buttons in the app list, but
they do not currently forward space to the search box (See
https://crbug.com/901245). These button types disappear or change focus
to the search box when searching anyway - only SearchResultBaseView
seems to really benefit from being able to handle Ctrl+Space.

Bug: 899094
Test: AudioPlayerBrowserTest.NativeMediaKey
Change-Id: I1cb585d88d57a775fdfa15ecfdce197ae52dfcdd
Reviewed-on: https://chromium-review.googlesource.com/c/1312439
Commit-Queue: Trent Apted <[email protected]>
Reviewed-by: Xiyuan Xia <[email protected]>
Reviewed-by: Luciano Pacheco <[email protected]>
Cr-Commit-Position: refs/heads/master@{#605219}

ash/accelerators/pre_target_accelerator_handler.cc

@@ -5,7 +5,6 @@
 #include "ash/accelerators/pre_target_accelerator_handler.h"
 
 #include "ash/accelerators/accelerator_controller.h"
-#include "ash/app_list/app_list_controller_impl.h"
 #include "ash/public/cpp/ash_features.h"
 #include "ash/shell.h"
 #include "ash/wm/window_state.h"
@@ -136,10 +135,7 @@ bool PreTargetAcceleratorHandler::ShouldProcessAcceleratorNow(
 
   // Handle preferred accelerators (such as ALT-TAB) before sending
   // to the target.
-  if (accelerator_controller->IsPreferred(accelerator))
-    return true;
-
-  return Shell::Get()->app_list_controller()->GetTargetVisibility();
+  return accelerator_controller->IsPreferred(accelerator);
 }
 
 }  // namespace ash

ash/app_list/views/app_list_item_view.cc

@@ -648,6 +648,12 @@ bool AppListItemView::OnMouseDragged(const ui::MouseEvent& event) {
   return true;
 }
 
+bool AppListItemView::SkipDefaultKeyEventProcessing(const ui::KeyEvent& event) {
+  // Ensure accelerators take priority in the app list. This ensures, e.g., that
+  // Ctrl+Space will switch input methods rather than activate the button.
+  return false;
+}
+
 void AppListItemView::OnFocus() {
   apps_grid_view_->SetSelectedView(this);
 }

ash/app_list/views/app_list_item_view.h

@@ -198,6 +198,7 @@ class APP_LIST_EXPORT AppListItemView
   bool OnMousePressed(const ui::MouseEvent& event) override;
   void OnMouseReleased(const ui::MouseEvent& event) override;
   bool OnMouseDragged(const ui::MouseEvent& event) override;
+  bool SkipDefaultKeyEventProcessing(const ui::KeyEvent& event) override;
   void OnFocus() override;
   void OnBlur() override;
 

ash/app_list/views/search_result_base_view.cc

@@ -10,6 +10,13 @@ SearchResultBaseView::SearchResultBaseView() : Button(this) {}
 
 SearchResultBaseView::~SearchResultBaseView() = default;
 
+bool SearchResultBaseView::SkipDefaultKeyEventProcessing(
+    const ui::KeyEvent& event) {
+  // Ensure accelerators take priority in the app list. This ensures, e.g., that
+  // Ctrl+Space will switch input methods rather than activate the button.
+  return false;
+}
+
 void SearchResultBaseView::SetBackgroundHighlighted(bool enabled) {
   background_highlighted_ = enabled;
   SchedulePaint();

ash/app_list/views/search_result_base_view.h

@@ -22,6 +22,9 @@ class APP_LIST_EXPORT SearchResultBaseView : public views::Button,
 
   bool background_highlighted() const { return background_highlighted_; }
 
+  // views::Button:
+  bool SkipDefaultKeyEventProcessing(const ui::KeyEvent& event) override;
+
  protected:
   ~SearchResultBaseView() override;
 

ash/shelf/back_button_unittest.cc

@@ -7,6 +7,8 @@
 #include <memory>
 
 #include "ash/accelerators/accelerator_controller.h"
+#include "ash/app_list/test/app_list_test_helper.h"
+#include "ash/app_list/views/app_list_view.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_view.h"
 #include "ash/shelf/shelf_view_test_api.h"
@@ -84,7 +86,15 @@ TEST_F(BackButtonTest, BackKeySequenceGenerated) {
 
   AcceleratorController* controller = Shell::Get()->accelerator_controller();
 
-  // Register an accelerator that looks for back presses.
+  // Register an accelerator that looks for back presses. Note there is already
+  // an accelerator on AppListView, which will handle the accelerator since it
+  // is targeted before AcceleratorController (switching to tablet mode with no
+  // other windows activates the app list). First remove that accelerator. In
+  // release, there's only the AppList's accelerator, so it's always hit when
+  // the app list is active. (ash/accelerators.cc has VKEY_BROWSER_BACK, but it
+  // also needs Ctrl pressed).
+  GetAppListTestHelper()->GetAppListView()->ResetAccelerators();
+
   ui::Accelerator accelerator_back_press(ui::VKEY_BROWSER_BACK, ui::EF_NONE);
   accelerator_back_press.set_key_state(ui::Accelerator::KeyState::PRESSED);
   ui::TestAcceleratorTarget target_back_press;

chrome/browser/chromeos/file_manager/audio_player_browsertest.cc

@@ -79,4 +79,9 @@ IN_PROC_BROWSER_TEST_F(AudioPlayerBrowserTest, ChangeTracksPlayListIcon) {
   StartTest();
 }
 
+IN_PROC_BROWSER_TEST_F(AudioPlayerBrowserTest, NativeMediaKey) {
+  set_test_case_name("mediaKeyNative");
+  StartTest();
+}
+
 }  // namespace file_manager

chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc

@@ -36,6 +36,7 @@
 #include "chrome/browser/notifications/notification_display_service_tester.h"
 #include "chrome/browser/sync_file_system/mock_remote_file_sync_service.h"
 #include "chrome/browser/sync_file_system/sync_file_system_service_factory.h"
+#include "chrome/browser/ui/ash/tablet_mode_client_test_util.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/api/file_manager_private.h"
@@ -55,6 +56,8 @@
 #include "content/public/common/service_manager_connection.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/api/test/test_api.h"
+#include "extensions/browser/app_window/app_window.h"
+#include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_function_registry.h"
 #include "extensions/browser/notification_types.h"
@@ -65,6 +68,10 @@
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "storage/browser/fileapi/external_mount_points.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_tree_host.h"
+#include "ui/events/event.h"
+#include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/message_center/public/cpp/notification.h"
 
 namespace file_manager {
@@ -1463,6 +1470,28 @@ void FileManagerBrowserTestBase::OnCommand(const std::string& name,
     return;
   }
 
+  if (name == "dispatchNativeMediaKey") {
+    ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_MEDIA_PLAY_PAUSE, 0);
+
+    // Try to dispatch the event close-to-native without pulling in too many
+    // dependencies (i.e. X11/Ozone/Wayland/Mus). aura::WindowTreeHost is pretty
+    // high up in the dispatch stack, but we might need event_injector.mojom
+    // for a more realistic Mus dispatch.
+    const auto& app_windows =
+        extensions::AppWindowRegistry::Get(profile())->app_windows();
+    ASSERT_FALSE(app_windows.empty());
+    app_windows.front()->GetNativeWindow()->GetHost()->DispatchKeyEventPostIME(
+        &key_event, base::BindOnce([](bool) {}));
+    *output = "mediaKeyDispatched";
+    return;
+  }
+
+  if (name == "enableTabletMode") {
+    ::test::SetAndWaitForTabletMode(true);
+    *output = "tabletModeEnabled";
+    return;
+  }
+
   FAIL() << "Unknown test message: " << name;
 }
 

ui/file_manager/integration_tests/audio_player/click_control_buttons.js

@@ -104,6 +104,56 @@ testcase.togglePlayState = function() {
       });
 };
 
+
+/**
+ * Confirms that native media keys are dispatched correctly.
+ * @return {Promise} Promise to be fulfilled on success.
+ */
+testcase.mediaKeyNative = function() {
+  const openAudio = launch('local', 'downloads', [ENTRIES.beautiful]);
+  let appId;
+  function ensurePlaying() {
+    return remoteCallAudioPlayer.waitForElement(appId, 'audio-player[playing]')
+        .then((element) => {
+          chrome.test.assertTrue(!!element, 'Not Playing.');
+        });
+  }
+  function ensurePaused() {
+    return remoteCallAudioPlayer
+        .waitForElement(appId, 'audio-player:not([playing])')
+        .then((element) => {
+          chrome.test.assertTrue(!!element, 'Not Paused.');
+        });
+  }
+  function sendMediaKey() {
+    return sendTestMessage({name: 'dispatchNativeMediaKey'}).then((result) => {
+      chrome.test.assertEq(
+          result, 'mediaKeyDispatched', 'Key dispatch failure');
+    });
+  }
+  function pauseAndUnpause() {
+    // Audio player should be playing when this is called,
+    return Promise.resolve()
+        .then(ensurePlaying)
+        .then(sendMediaKey)
+        .then(ensurePaused)
+        .then(sendMediaKey)
+        .then(ensurePlaying);
+  }
+  function enableTabletMode() {
+    return sendTestMessage({name: 'enableTabletMode'}).then((result) => {
+      chrome.test.assertEq(result, 'tabletModeEnabled');
+    });
+  }
+  return openAudio
+      .then((args) => {
+        appId = args[0];
+      })
+      .then(pauseAndUnpause)
+      .then(enableTabletMode)
+      .then(pauseAndUnpause);
+};
+
 /**
  * Confirms that the AudioPlayer default volume is 50, and that clicking the
  * volume button mutes / unmutes the volume.