Flip the action of side volume button if necessary.

Bug: 937907
Change-Id: Ib49153233e54cc6be09fff64a73fcd2e38bbbc93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1566615Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Min Chen <minch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652558}

ash/accelerators/accelerator_controller.cc

@@ -20,6 +20,7 @@
 #include "ash/debug.h"
 #include "ash/display/display_configuration_controller.h"
 #include "ash/display/display_move_window_util.h"
+#include "ash/display/screen_orientation_controller.h"
 #include "ash/focus_cycler.h"
 #include "ash/ime/ime_controller.h"
 #include "ash/ime/ime_switch_type.h"
@@ -60,6 +61,7 @@
 #include "ash/wm/mru_window_tracker.h"
 #include "ash/wm/overview/overview_controller.h"
 #include "ash/wm/screen_pinning_controller.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/wm/window_cycle_controller.h"
 #include "ash/wm/window_positioning_utils.h"
 #include "ash/wm/window_state.h"
@@ -87,6 +89,8 @@
 #include "ui/display/display.h"
 #include "ui/display/manager/managed_display_info.h"
 #include "ui/display/screen.h"
+#include "ui/events/devices/input_device.h"
+#include "ui/events/devices/input_device_manager.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/keyboard/keyboard_controller.h"
 #include "ui/message_center/message_center.h"
@@ -964,10 +968,8 @@ void HandleToggleSpokenFeedback() {
                                        A11Y_NOTIFICATION_SHOW);
 }
 
-void HandleVolumeDown(mojom::VolumeController* volume_controller,
-                      const ui::Accelerator& accelerator) {
-  if (accelerator.key_code() == ui::VKEY_VOLUME_DOWN)
-    base::RecordAction(UserMetricsAction("Accel_VolumeDown_F9"));
+void HandleVolumeDown(mojom::VolumeController* volume_controller) {
+  base::RecordAction(UserMetricsAction("Accel_VolumeDown_F9"));
 
   if (volume_controller)
     volume_controller->VolumeDown();
@@ -982,10 +984,8 @@ void HandleVolumeMute(mojom::VolumeController* volume_controller,
     volume_controller->VolumeMute();
 }
 
-void HandleVolumeUp(mojom::VolumeController* volume_controller,
-                    const ui::Accelerator& accelerator) {
-  if (accelerator.key_code() == ui::VKEY_VOLUME_UP)
-    base::RecordAction(UserMetricsAction("Accel_VolumeUp_F10"));
+void HandleVolumeUp(mojom::VolumeController* volume_controller) {
+  base::RecordAction(UserMetricsAction("Accel_VolumeUp_F10"));
 
   if (volume_controller)
     volume_controller->VolumeUp();
@@ -1426,12 +1426,10 @@ void AcceleratorController::PerformAction(AcceleratorAction action,
   if (restriction != RESTRICTION_NONE)
     return;
 
-  // TODO(minch): For VOLUME_DOWN and VOLUME_UP. Check whether the action is
-  // from side volume button based on accelerator.source_device_id() and
-  // ui::InputDeviceManager::GetInstance()->GetUncategorizedDevices(). Do the
-  // calculation whether we need to flip its action on
-  // SideVolumeButtonLocation and current screen orientation.
-  // http://crbug.com/937907.
+  if ((action == VOLUME_DOWN || action == VOLUME_UP) &&
+      ShouldSwapSideVolumeButtons(accelerator.source_device_id())) {
+    action = action == VOLUME_DOWN ? VOLUME_UP : VOLUME_DOWN;
+  }
 
   // If your accelerator invokes more than one line of code, please either
   // implement it in your module's controller code or pull it into a HandleFoo()
@@ -1727,13 +1725,13 @@ void AcceleratorController::PerformAction(AcceleratorAction action,
       accelerators::UnpinWindow();
       break;
     case VOLUME_DOWN:
-      HandleVolumeDown(volume_controller_.get(), accelerator);
+      HandleVolumeDown(volume_controller_.get());
       break;
     case VOLUME_MUTE:
       HandleVolumeMute(volume_controller_.get(), accelerator);
       break;
     case VOLUME_UP:
-      HandleVolumeUp(volume_controller_.get(), accelerator);
+      HandleVolumeUp(volume_controller_.get());
       break;
     case WINDOW_CYCLE_SNAP_LEFT:
     case WINDOW_CYCLE_SNAP_RIGHT:
@@ -1873,4 +1871,64 @@ void AcceleratorController::ParseSideVolumeButtonLocationInfo() {
                           &side_volume_button_location_.side);
 }
 
+bool AcceleratorController::IsSideVolumeButton(int source_device_id) const {
+  if (source_device_id == ui::ED_UNKNOWN_DEVICE)
+    return false;
+
+  for (const ui::InputDevice& uncategorized_device :
+       ui::InputDeviceManager::GetInstance()->GetUncategorizedDevices()) {
+    if (uncategorized_device.id == source_device_id &&
+        uncategorized_device.type ==
+            ui::InputDeviceType::INPUT_DEVICE_INTERNAL) {
+      return true;
+    }
+  }
+  return false;
+}
+
+bool AcceleratorController::IsValidSideVolumeButtonLocation() const {
+  const std::string region = side_volume_button_location_.region;
+  const std::string side = side_volume_button_location_.side;
+  if (region != kVolumeButtonRegionKeyboard &&
+      region != kVolumeButtonRegionScreen) {
+    return false;
+  }
+  if (side != kVolumeButtonSideLeft && side != kVolumeButtonSideRight &&
+      side != kVolumeButtonSideTop && side != kVolumeButtonSideBottom) {
+    return false;
+  }
+  return true;
+}
+
+bool AcceleratorController::ShouldSwapSideVolumeButtons(
+    int source_device_id) const {
+  if (!Shell::Get()
+           ->tablet_mode_controller()
+           ->IsTabletModeWindowManagerEnabled() ||
+      !IsSideVolumeButton(source_device_id)) {
+    return false;
+  }
+
+  if (!IsValidSideVolumeButtonLocation())
+    return false;
+
+  OrientationLockType screen_orientation =
+      Shell::Get()->screen_orientation_controller()->GetCurrentOrientation();
+  const std::string side = side_volume_button_location_.side;
+  const bool is_landscape_secondary_or_portrait_primary =
+      screen_orientation == OrientationLockType::kLandscapeSecondary ||
+      screen_orientation == OrientationLockType::kPortraitPrimary;
+
+  if (side_volume_button_location_.region == kVolumeButtonRegionKeyboard) {
+    if (side == kVolumeButtonSideLeft || side == kVolumeButtonSideRight)
+      return IsPrimaryOrientation(screen_orientation);
+    return is_landscape_secondary_or_portrait_primary;
+  }
+
+  DCHECK_EQ(kVolumeButtonRegionScreen, side_volume_button_location_.region);
+  if (side == kVolumeButtonSideLeft || side == kVolumeButtonSideRight)
+    return !IsPrimaryOrientation(screen_orientation);
+  return is_landscape_secondary_or_portrait_primary;
+}
+
 }  // namespace ash

ash/accelerators/accelerator_controller.h

@@ -189,6 +189,11 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget,
   void set_side_volume_button_file_path_for_testing(base::FilePath path) {
     side_volume_button_location_file_path_ = path;
   }
+  void set_side_volume_button_location_for_testing(const std::string& region,
+                                                   const std::string& side) {
+    side_volume_button_location_.region = region;
+    side_volume_button_location_.side = side;
+  }
   SideVolumeButtonLocation side_volume_button_location_for_testing() {
     return side_volume_button_location_;
   }
@@ -239,6 +244,17 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget,
       AcceleratorAction action,
       const ui::Accelerator& accelerator) const;
 
+  // Returns true if |source_device_id| corresponds to the side volume buttons.
+  bool IsSideVolumeButton(int source_device_id) const;
+
+  // Returns true if |side_volume_button_location_| is in agreed format and
+  // values.
+  bool IsValidSideVolumeButtonLocation() const;
+
+  // Returns true if the side volume buttons should be swapped. See
+  // SideVolumeButonLocation for the details.
+  bool ShouldSwapSideVolumeButtons(int source_device_id) const;
+
   std::unique_ptr<ui::AcceleratorManager> accelerator_manager_;
 
   // A tracker for the current and previous accelerators.
@@ -296,7 +312,7 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget,
   // set to different paths in test.
   base::FilePath side_volume_button_location_file_path_;
 
-  // Stores the location info of side volume button.
+  // Stores the location info of side volume buttons.
   SideVolumeButtonLocation side_volume_button_location_;
 
   DISALLOW_COPY_AND_ASSIGN(AcceleratorController);

ash/accelerators/accelerator_controller_unittest.cc

@@ -13,6 +13,8 @@
 #include "ash/accessibility/test_accessibility_controller_client.h"
 #include "ash/app_list/app_list_metrics.h"
 #include "ash/app_list/test/app_list_test_helper.h"
+#include "ash/display/screen_orientation_controller.h"
+#include "ash/display/screen_orientation_controller_test_api.h"
 #include "ash/ime/ime_controller.h"
 #include "ash/ime/test_ime_controller_client.h"
 #include "ash/magnifier/docked_magnifier_controller.h"
@@ -32,6 +34,7 @@
 #include "ash/test_media_client.h"
 #include "ash/test_screenshot_delegate.h"
 #include "ash/wm/lock_state_controller.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/wm/test_session_state_animator.h"
 #include "ash/wm/window_positioning_utils.h"
 #include "ash/wm/window_state.h"
@@ -48,6 +51,7 @@
 #include "media/base/media_switches.h"
 #include "services/media_session/public/cpp/test/test_media_controller.h"
 #include "services/media_session/public/mojom/media_session.mojom.h"
+#include "services/ws/public/cpp/input_devices/input_device_client_test_api.h"
 #include "services/ws/public/mojom/window_tree_constants.mojom.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/test/test_window_delegate.h"
@@ -59,6 +63,7 @@
 #include "ui/base/ime/chromeos/ime_keyboard.h"
 #include "ui/display/manager/display_manager.h"
 #include "ui/display/screen.h"
+#include "ui/display/test/display_manager_test_api.h"
 #include "ui/events/event.h"
 #include "ui/events/event_sink.h"
 #include "ui/events/keycodes/dom/dom_code.h"
@@ -1088,6 +1093,149 @@ TEST_F(AcceleratorControllerTest, SideVolumeButtonLocation) {
   base::DeleteFile(file_path, false);
 }
 
+class SideVolumeButtonAcceleratorTest
+    : public AcceleratorControllerTest,
+      public testing::WithParamInterface<std::pair<std::string, std::string>> {
+ public:
+  // Input device id of the side volume button.
+  static constexpr int kSideVolumeButtonId = 7;
+
+  SideVolumeButtonAcceleratorTest()
+      : region_(GetParam().first), side_(GetParam().second) {}
+  ~SideVolumeButtonAcceleratorTest() override = default;
+
+  void SetUp() override {
+    AcceleratorControllerTest::SetUp();
+    Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(true);
+    AcceleratorController* controller = GetController();
+    DCHECK(controller);
+    controller->set_side_volume_button_location_for_testing(region_, side_);
+    ws::InputDeviceClientTestApi().SetUncategorizedDevices({ui::InputDevice(
+        kSideVolumeButtonId, ui::InputDeviceType::INPUT_DEVICE_INTERNAL,
+        "cros_ec_buttons")});
+  }
+
+  bool IsLeftOrRightSide() const {
+    return side_ == AcceleratorController::kVolumeButtonSideLeft ||
+           side_ == AcceleratorController::kVolumeButtonSideRight;
+  }
+
+  bool IsOnKeyboard() const {
+    return region_ == AcceleratorController::kVolumeButtonRegionKeyboard;
+  }
+
+ private:
+  std::string region_, side_;
+
+  DISALLOW_COPY_AND_ASSIGN(SideVolumeButtonAcceleratorTest);
+};
+
+// Tests the the action of side volume button will get flipped in corresponding
+// screen orientation.
+TEST_P(SideVolumeButtonAcceleratorTest, FlipSideVolumeButtonAction) {
+  display::test::ScopedSetInternalDisplayId set_internal(
+      display_manager(), GetPrimaryDisplay().id());
+
+  ScreenOrientationControllerTestApi test_api(
+      Shell::Get()->screen_orientation_controller());
+  // Set the screen orientation to LANDSCAPE_PRIMARY.
+  test_api.SetDisplayRotation(display::Display::ROTATE_0,
+                              display::Display::RotationSource::ACTIVE);
+  EXPECT_EQ(test_api.GetCurrentOrientation(),
+            OrientationLockType::kLandscapePrimary);
+
+  base::UserActionTester user_action_tester;
+  const ui::Accelerator volume_down(ui::VKEY_VOLUME_DOWN, ui::EF_NONE);
+  ASSERT_EQ(ui::ED_UNKNOWN_DEVICE, volume_down.source_device_id());
+  ProcessInController(volume_down);
+  // Tests that the VOLUME_DOWN accelerator always goes to decrease the volume
+  // if it is not from the side volume button.
+  EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeDown_F9"));
+  user_action_tester.ResetCounts();
+
+  ui::KeyEvent event(ui::ET_KEY_PRESSED, ui::VKEY_VOLUME_DOWN,
+                     ui::DomCode::VOLUME_DOWN, /*flags=*/0, /*dom_key=*/2099727,
+                     base::TimeTicks::Now());
+  event.set_source_device_id(kSideVolumeButtonId);
+  const ui::Accelerator volume_down_from_side_volume_button(event);
+  ProcessInController(volume_down_from_side_volume_button);
+  // Tests that the action of side volume button will get flipped in landscape
+  // primary if the the button is at the left or right of keyboard.
+  if (IsOnKeyboard() && IsLeftOrRightSide())
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeUp_F10"));
+  else
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeDown_F9"));
+  user_action_tester.ResetCounts();
+
+  // Rotate the screen by 270 degree.
+  test_api.SetDisplayRotation(display::Display::ROTATE_270,
+                              display::Display::RotationSource::ACTIVE);
+  EXPECT_EQ(test_api.GetCurrentOrientation(),
+            OrientationLockType::kPortraitPrimary);
+  ProcessInController(volume_down_from_side_volume_button);
+  // Tests that the action of side volume button will not be flipped in portrait
+  // primary if the button is at the left or right of screen. Otherwise, the
+  // action will be flipped.
+  if (!IsOnKeyboard() && IsLeftOrRightSide())
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeDown_F9"));
+  else
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeUp_F10"));
+  user_action_tester.ResetCounts();
+
+  // Rotate the screen by 180 degree.
+  test_api.SetDisplayRotation(display::Display::ROTATE_180,
+                              display::Display::RotationSource::ACTIVE);
+  EXPECT_EQ(test_api.GetCurrentOrientation(),
+            OrientationLockType::kLandscapeSecondary);
+  ProcessInController(volume_down_from_side_volume_button);
+  // Tests that the action of side volume button will not be flipped in
+  // landscape secondary if the button is at the left or right of keyboard.
+  // Otherwise, the action will be flipped.
+  if (IsOnKeyboard() && IsLeftOrRightSide())
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeDown_F9"));
+  else
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeUp_F10"));
+  user_action_tester.ResetCounts();
+
+  // Rotate the screen by 90 degree.
+  test_api.SetDisplayRotation(display::Display::ROTATE_90,
+                              display::Display::RotationSource::ACTIVE);
+  EXPECT_EQ(test_api.GetCurrentOrientation(),
+            OrientationLockType::kPortraitSecondary);
+  ProcessInController(volume_down_from_side_volume_button);
+  // Tests that the action of side volume button will be flipped in portrait
+  // secondary if the buttonis at the left or right of screen.
+  if (!IsOnKeyboard() && IsLeftOrRightSide())
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeUp_F10"));
+  else
+    EXPECT_EQ(1, user_action_tester.GetActionCount("Accel_VolumeDown_F9"));
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    AshSideVolumeButton,
+    SideVolumeButtonAcceleratorTest,
+    testing::ValuesIn({std::make_pair<std::string, std::string>(
+                           AcceleratorController::kVolumeButtonRegionKeyboard,
+                           AcceleratorController::kVolumeButtonSideLeft),
+                       std::make_pair<std::string, std::string>(
+                           AcceleratorController::kVolumeButtonRegionKeyboard,
+                           AcceleratorController::kVolumeButtonSideRight),
+                       std::make_pair<std::string, std::string>(
+                           AcceleratorController::kVolumeButtonRegionKeyboard,
+                           AcceleratorController::kVolumeButtonSideBottom),
+                       std::make_pair<std::string, std::string>(
+                           AcceleratorController::kVolumeButtonRegionScreen,
+                           AcceleratorController::kVolumeButtonSideLeft),
+                       std::make_pair<std::string, std::string>(
+                           AcceleratorController::kVolumeButtonRegionScreen,
+                           AcceleratorController::kVolumeButtonSideRight),
+                       std::make_pair<std::string, std::string>(
+                           AcceleratorController::kVolumeButtonRegionScreen,
+                           AcceleratorController::kVolumeButtonSideTop),
+                       std::make_pair<std::string, std::string>(
+                           AcceleratorController::kVolumeButtonRegionScreen,
+                           AcceleratorController::kVolumeButtonSideBottom)}));
+
 namespace {
 
 // Tests the TOGGLE_CAPS_LOCK accelerator.

services/ws/public/cpp/input_devices/input_device_client_test_api.cc

@@ -107,6 +107,14 @@ void InputDeviceClientTestApi::SetTouchpadDevices(
   }
 }
 
+void InputDeviceClientTestApi::SetUncategorizedDevices(
+    const std::vector<ui::InputDevice>& devices) {
+  if (ui::DeviceDataManager::instance_)
+    ui::DeviceDataManager::instance_->OnUncategorizedDevicesUpdated(devices);
+  else
+    GetInputDeviceClient()->OnUncategorizedDeviceConfigurationChanged(devices);
+}
+
 InputDeviceClient* InputDeviceClientTestApi::GetInputDeviceClient() {
   if (ui::DeviceDataManager::instance_ ||
       !ui::InputDeviceManager::HasInstance())

services/ws/public/cpp/input_devices/input_device_client_test_api.h

@@ -42,6 +42,7 @@ class InputDeviceClientTestApi {
   void SetKeyboardDevices(const std::vector<ui::InputDevice>& devices);
   void SetMouseDevices(const std::vector<ui::InputDevice>& devices);
   void SetTouchpadDevices(const std::vector<ui::InputDevice>& devices);
+  void SetUncategorizedDevices(const std::vector<ui::InputDevice>& devices);
 
   // |are_touchscreen_target_displays_valid| is only applicable to
   // InputDeviceClient. See

ui/base/accelerators/accelerator.h

@@ -125,7 +125,7 @@ class UI_BASE_EXPORT Accelerator {
   bool interrupted_by_mouse_event_;
 
   // The |source_device_id_| of the KeyEvent.
-  int source_device_id_ = -1;
+  int source_device_id_ = ui::ED_UNKNOWN_DEVICE;
 };
 
 // An interface that classes that want to register for keyboard accelerators