See also:

http://code.google.com/p/chromium/issues/detail?id=134093
https://chromiumcodereview.appspot.com/10993067/

This is a better fix than 10993067. This CL undoes that change and moves the fix to the AcceleratorController. This is a more general fix which employs a table of actions that are allowed when in a modal dialog. All other actions are suppressed.

BUG=153077
TEST=System Tray -> Ehternet -> Join Other; while the modal dialog is up, try various ash shortcuts. Observe that the ones are working (like volume up) make sense, but things like new window or moving around windows are inactive.

Review URL: https://chromiumcodereview.appspot.com/10977088

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@162240 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 222 insertions, 8 deletions

diff --git a/ash/accelerators/accelerator_controller.cc b/ash/accelerators/accelerator_controller.cc
index 4817c59..f4e8f26 100644
--- a/ash/accelerators/accelerator_controller.cc
+++ b/ash/accelerators/accelerator_controller.cc
@@ -359,12 +359,12 @@ void AcceleratorController::Init() {
     actions_allowed_at_lock_screen_.insert(
         kActionsAllowedAtLoginOrLockScreen[i]);
   }
-  for (size_t i = 0; i < kActionsAllowedAtLockScreenLength; ++i) {
+  for (size_t i = 0; i < kActionsAllowedAtLockScreenLength; ++i)
     actions_allowed_at_lock_screen_.insert(kActionsAllowedAtLockScreen[i]);
-  }
-  for (size_t i = 0; i < kReservedActionsLength; ++i) {
+  for (size_t i = 0; i < kActionsAllowedAtModalWindowLength; ++i)
+    actions_allowed_at_modal_window_.insert(kActionsAllowedAtModalWindow[i]);
+  for (size_t i = 0; i < kReservedActionsLength; ++i)
     reserved_actions_.insert(kReservedActions[i]);
-  }
 
   RegisterAccelerators(kAcceleratorData, kAcceleratorDataLength);
 
@@ -421,18 +421,25 @@ bool AcceleratorController::PerformAction(int action,
 #if defined(OS_CHROMEOS)
   at_login_screen = shell->delegate() && !shell->delegate()->IsSessionStarted();
 #endif
-  bool at_lock_screen = shell->IsScreenLocked();
-
   if (at_login_screen &&
       actions_allowed_at_login_screen_.find(action) ==
       actions_allowed_at_login_screen_.end()) {
     return false;
   }
-  if (at_lock_screen &&
+  if (shell->IsScreenLocked() &&
       actions_allowed_at_lock_screen_.find(action) ==
       actions_allowed_at_lock_screen_.end()) {
     return false;
   }
+  if (shell->IsModalWindowOpen() &&
+      actions_allowed_at_modal_window_.find(action) ==
+      actions_allowed_at_modal_window_.end()) {
+    // Note: we return true. This indicates the shortcut is handled
+    // and will not be passed to the modal window. This is important
+    // for things like Alt+Tab that would cause an undesired effect
+    // in the modal window by cycling through its window elements.
+    return true;
+  }
   const ui::KeyboardCode key_code = accelerator.key_code();
 
   const ui::AcceleratorManagerContext& context =
diff --git a/ash/accelerators/accelerator_controller.h b/ash/accelerators/accelerator_controller.h
index 0e2ddf4..672e895 100644
--- a/ash/accelerators/accelerator_controller.h
+++ b/ash/accelerators/accelerator_controller.h
@@ -115,6 +115,8 @@ class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget {
   std::set<int> actions_allowed_at_login_screen_;
   // Actions allowed when the screen is locked.
   std::set<int> actions_allowed_at_lock_screen_;
+  // Actions allowed when a modal window is up.
+  std::set<int> actions_allowed_at_modal_window_;
   // Reserved actions. See accelerator_table.h for details.
   std::set<int> reserved_actions_;
 
diff --git a/ash/accelerators/accelerator_controller_unittest.cc b/ash/accelerators/accelerator_controller_unittest.cc
index 5482d25..5f11bfb 100644
--- a/ash/accelerators/accelerator_controller_unittest.cc
+++ b/ash/accelerators/accelerator_controller_unittest.cc
@@ -1035,5 +1035,147 @@ TEST_F(AcceleratorControllerTest, ReservedAccelerators) {
       ui::Accelerator(ui::VKEY_A, ui::EF_NONE)));
 }
 
+TEST_F(AcceleratorControllerTest, DisallowedAtModalWindow) {
+  std::set<AcceleratorAction> allActions;
+  for (size_t i = 0 ; i < kAcceleratorDataLength; ++i)
+    allActions.insert(kAcceleratorData[i].action);
+  std::set<AcceleratorAction> actionsAllowedAtModalWindow;
+  for (size_t k = 0 ; k < kActionsAllowedAtModalWindowLength; ++k)
+    actionsAllowedAtModalWindow.insert(kActionsAllowedAtModalWindow[k]);
+  for (std::set<AcceleratorAction>::const_iterator it =
+           actionsAllowedAtModalWindow.begin();
+       it != actionsAllowedAtModalWindow.end(); ++it) {
+    EXPECT_FALSE(allActions.find(*it) == allActions.end())
+        << " action from kActionsAllowedAtModalWindow"
+        << " not found in kAcceleratorData. action: " << *it;
+  }
+  scoped_ptr<aura::Window> window(
+      aura::test::CreateTestWindowWithBounds(gfx::Rect(5, 5, 20, 20), NULL));
+  const ui::Accelerator dummy;
+  wm::ActivateWindow(window.get());
+  Shell::GetInstance()->SimulateModalWindowOpenForTesting(true);
+  for (std::set<AcceleratorAction>::const_iterator it = allActions.begin();
+       it != allActions.end(); ++it) {
+    if (actionsAllowedAtModalWindow.find(*it) ==
+        actionsAllowedAtModalWindow.end()) {
+      EXPECT_TRUE(GetController()->PerformAction(*it, dummy))
+          << " for action (disallowed at modal window): " << *it;
+    }
+  }
+#if defined(OS_CHROMEOS)
+  //  Testing of top row (F5-F10) accelerators that should still work
+  //  when a modal window is open
+  //
+  // Screenshot
+  {
+    EXPECT_TRUE(GetController()->Process(
+        ui::Accelerator(ui::VKEY_F5, ui::EF_CONTROL_DOWN)));
+    EXPECT_TRUE(GetController()->Process(
+        ui::Accelerator(ui::VKEY_PRINT, ui::EF_NONE)));
+    EXPECT_TRUE(GetController()->Process(
+        ui::Accelerator(ui::VKEY_F5, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN)));
+    DummyScreenshotDelegate* delegate = new DummyScreenshotDelegate;
+    GetController()->SetScreenshotDelegate(
+        scoped_ptr<ScreenshotDelegate>(delegate).Pass());
+    EXPECT_EQ(0, delegate->handle_take_screenshot_count());
+    EXPECT_TRUE(GetController()->Process(
+        ui::Accelerator(ui::VKEY_F5, ui::EF_CONTROL_DOWN)));
+    EXPECT_EQ(1, delegate->handle_take_screenshot_count());
+    EXPECT_TRUE(GetController()->Process(
+        ui::Accelerator(ui::VKEY_PRINT, ui::EF_NONE)));
+    EXPECT_EQ(2, delegate->handle_take_screenshot_count());
+    EXPECT_TRUE(GetController()->Process(
+        ui::Accelerator(ui::VKEY_F5, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN)));
+    EXPECT_EQ(2, delegate->handle_take_screenshot_count());
+  }
+  // Brightness
+  const ui::Accelerator f6(ui::VKEY_F6, ui::EF_NONE);
+  const ui::Accelerator f7(ui::VKEY_F7, ui::EF_NONE);
+  {
+    EXPECT_FALSE(GetController()->Process(f6));
+    EXPECT_FALSE(GetController()->Process(f7));
+    DummyBrightnessControlDelegate* delegate =
+        new DummyBrightnessControlDelegate(true);
+    GetController()->SetBrightnessControlDelegate(
+        scoped_ptr<BrightnessControlDelegate>(delegate).Pass());
+    EXPECT_FALSE(GetController()->Process(f6));
+    EXPECT_FALSE(GetController()->Process(f7));
+  }
+  EnableInternalDisplay();
+  {
+    EXPECT_FALSE(GetController()->Process(f6));
+    EXPECT_FALSE(GetController()->Process(f7));
+    DummyBrightnessControlDelegate* delegate =
+        new DummyBrightnessControlDelegate(false);
+    GetController()->SetBrightnessControlDelegate(
+        scoped_ptr<BrightnessControlDelegate>(delegate).Pass());
+    EXPECT_EQ(0, delegate->handle_brightness_down_count());
+    EXPECT_FALSE(GetController()->Process(f6));
+    EXPECT_EQ(1, delegate->handle_brightness_down_count());
+    EXPECT_EQ(f6, delegate->last_accelerator());
+    EXPECT_EQ(0, delegate->handle_brightness_up_count());
+    EXPECT_FALSE(GetController()->Process(f7));
+    EXPECT_EQ(1, delegate->handle_brightness_up_count());
+    EXPECT_EQ(f7, delegate->last_accelerator());
+  }
+  {
+    DummyBrightnessControlDelegate* delegate =
+        new DummyBrightnessControlDelegate(true);
+    GetController()->SetBrightnessControlDelegate(
+        scoped_ptr<BrightnessControlDelegate>(delegate).Pass());
+    EXPECT_EQ(0, delegate->handle_brightness_down_count());
+    EXPECT_TRUE(GetController()->Process(f6));
+    EXPECT_EQ(1, delegate->handle_brightness_down_count());
+    EXPECT_EQ(f6, delegate->last_accelerator());
+    EXPECT_EQ(0, delegate->handle_brightness_up_count());
+    EXPECT_TRUE(GetController()->Process(f7));
+    EXPECT_EQ(1, delegate->handle_brightness_up_count());
+    EXPECT_EQ(f7, delegate->last_accelerator());
+  }
+  // Volume
+  const ui::Accelerator f8(ui::VKEY_F8, ui::EF_NONE);
+  const ui::Accelerator f9(ui::VKEY_F9, ui::EF_NONE);
+  const ui::Accelerator f10(ui::VKEY_F10, ui::EF_NONE);
+  {
+    EXPECT_TRUE(GetController()->Process(f8));
+    EXPECT_TRUE(GetController()->Process(f9));
+    EXPECT_TRUE(GetController()->Process(f10));
+    DummyVolumeControlDelegate* delegate =
+        new DummyVolumeControlDelegate(false);
+    ash::Shell::GetInstance()->tray_delegate()->SetVolumeControlDelegate(
+        scoped_ptr<VolumeControlDelegate>(delegate).Pass());
+    EXPECT_EQ(0, delegate->handle_volume_mute_count());
+    EXPECT_FALSE(GetController()->Process(f8));
+    EXPECT_EQ(1, delegate->handle_volume_mute_count());
+    EXPECT_EQ(f8, delegate->last_accelerator());
+    EXPECT_EQ(0, delegate->handle_volume_down_count());
+    EXPECT_FALSE(GetController()->Process(f9));
+    EXPECT_EQ(1, delegate->handle_volume_down_count());
+    EXPECT_EQ(f9, delegate->last_accelerator());
+    EXPECT_EQ(0, delegate->handle_volume_up_count());
+    EXPECT_FALSE(GetController()->Process(f10));
+    EXPECT_EQ(1, delegate->handle_volume_up_count());
+    EXPECT_EQ(f10, delegate->last_accelerator());
+  }
+  {
+    DummyVolumeControlDelegate* delegate = new DummyVolumeControlDelegate(true);
+    ash::Shell::GetInstance()->tray_delegate()->SetVolumeControlDelegate(
+        scoped_ptr<VolumeControlDelegate>(delegate).Pass());
+    EXPECT_EQ(0, delegate->handle_volume_mute_count());
+    EXPECT_TRUE(GetController()->Process(f8));
+    EXPECT_EQ(1, delegate->handle_volume_mute_count());
+    EXPECT_EQ(f8, delegate->last_accelerator());
+    EXPECT_EQ(0, delegate->handle_volume_down_count());
+    EXPECT_TRUE(GetController()->Process(f9));
+    EXPECT_EQ(1, delegate->handle_volume_down_count());
+    EXPECT_EQ(f9, delegate->last_accelerator());
+    EXPECT_EQ(0, delegate->handle_volume_up_count());
+    EXPECT_TRUE(GetController()->Process(f10));
+    EXPECT_EQ(1, delegate->handle_volume_up_count());
+    EXPECT_EQ(f10, delegate->last_accelerator());
+  }
+#endif
+}
+
 }  // namespace test
 }  // namespace ash
diff --git a/ash/accelerators/accelerator_table.cc b/ash/accelerators/accelerator_table.cc
index 35a60ae..bd50541 100644
--- a/ash/accelerators/accelerator_table.cc
+++ b/ash/accelerators/accelerator_table.cc
@@ -253,4 +253,41 @@ const AcceleratorAction kActionsAllowedAtLockScreen[] = {
 const size_t kActionsAllowedAtLockScreenLength =
     arraysize(kActionsAllowedAtLockScreen);
 
+const AcceleratorAction kActionsAllowedAtModalWindow[] = {
+  BRIGHTNESS_DOWN,
+  BRIGHTNESS_UP,
+  DISABLE_CAPS_LOCK,
+  EXIT,
+  KEYBOARD_BRIGHTNESS_DOWN,
+  KEYBOARD_BRIGHTNESS_UP,
+  MAGNIFY_SCREEN_ZOOM_IN,
+  MAGNIFY_SCREEN_ZOOM_OUT,
+  MEDIA_NEXT_TRACK,
+  MEDIA_PLAY_PAUSE,
+  MEDIA_PREV_TRACK,
+  NEXT_IME,
+  OPEN_FEEDBACK_PAGE,
+  POWER_PRESSED,
+  POWER_RELEASED,
+  PREVIOUS_IME,
+  SHOW_KEYBOARD_OVERLAY,
+  SWAP_PRIMARY_DISPLAY,
+  SWITCH_IME,
+  TAKE_SCREENSHOT,
+  TAKE_PARTIAL_SCREENSHOT,
+  TOGGLE_CAPS_LOCK,
+  TOGGLE_SPOKEN_FEEDBACK,
+  TOGGLE_WIFI,
+  VOLUME_DOWN,
+  VOLUME_MUTE,
+  VOLUME_UP,
+#if defined(OS_CHROMEOS)
+  CYCLE_DISPLAY_MODE,
+  LOCK_SCREEN,
+#endif
+};
+
+const size_t kActionsAllowedAtModalWindowLength =
+    arraysize(kActionsAllowedAtModalWindow);
+
 }  // namespace ash
diff --git a/ash/accelerators/accelerator_table.h b/ash/accelerators/accelerator_table.h
index 14b4327..54c5d43 100644
--- a/ash/accelerators/accelerator_table.h
+++ b/ash/accelerators/accelerator_table.h
@@ -136,6 +136,12 @@ ASH_EXPORT extern const AcceleratorAction kActionsAllowedAtLockScreen[];
 // The number of elements in kActionsAllowedAtLockScreen.
 ASH_EXPORT extern const size_t kActionsAllowedAtLockScreenLength;
 
+// Actions allowed while a modal window is up.
+ASH_EXPORT extern const AcceleratorAction kActionsAllowedAtModalWindow[];
+
+// The number of elements in kActionsAllowedAtModalWindow.
+ASH_EXPORT extern const size_t kActionsAllowedAtModalWindowLength;
+
 }  // namespace ash
 
 #endif  // ASH_ACCELERATORS_ACCELERATOR_TABLE_H_
diff --git a/ash/accelerators/accelerator_table_unittest.cc b/ash/accelerators/accelerator_table_unittest.cc
index 8ec0645..6467a9b 100644
--- a/ash/accelerators/accelerator_table_unittest.cc
+++ b/ash/accelerators/accelerator_table_unittest.cc
@@ -58,4 +58,13 @@ TEST(AcceleratorTableTest, CheckDuplicatedActionsAllowedAtLoginOrLockScreen) {
   }
 }
 
+TEST(AcceleratorTableTest, CheckDuplicatedActionsAllowedAtModalWindow) {
+  std::set<AcceleratorAction> actions;
+  for (size_t i = 0; i < kActionsAllowedAtModalWindowLength; ++i) {
+    EXPECT_TRUE(actions.insert(kActionsAllowedAtModalWindow[i]).second)
+        << "Duplicated action: " << kActionsAllowedAtModalWindow[i]
+        << " at index: " << i;
+  }
+}
+
 }  // namespace ash
diff --git a/ash/shell.cc b/ash/shell.cc
index 3584a2f..83afc25 100644
--- a/ash/shell.cc
+++ b/ash/shell.cc
@@ -186,7 +186,8 @@ Shell::Shell(ShellDelegate* delegate)
       output_configurator_animation_(
           new internal::OutputConfiguratorAnimation()),
 #endif  // defined(OS_CHROMEOS)
-      browser_context_(NULL) {
+      browser_context_(NULL),
+      simulate_modal_window_open_for_testing_(false) {
   gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, screen_.get());
   gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_ALTERNATE, screen_.get());
   ui_controls::InstallUIControlsAura(internal::CreateUIControls());
@@ -538,6 +539,8 @@ bool Shell::IsScreenLocked() const {
 }
 
 bool Shell::IsModalWindowOpen() const {
+  if (simulate_modal_window_open_for_testing_)
+    return true;
   // TODO(oshima): Walk though all root windows.
   const aura::Window* modal_container = GetContainer(
       GetPrimaryRootWindow(),
diff --git a/ash/shell.h b/ash/shell.h
index 2f08927..dbcc2fd 100644
--- a/ash/shell.h
+++ b/ash/shell.h
@@ -214,6 +214,11 @@ class ASH_EXPORT Shell : internal::SystemModalContainerEventFilterDelegate{
   // Returns true if a modal dialog window is currently open.
   bool IsModalWindowOpen() const;
 
+  // For testing only: set simulation that a modal window is open
+  void SimulateModalWindowOpenForTesting(bool modal_window_open) {
+    simulate_modal_window_open_for_testing_ = modal_window_open;
+  }
+
   // Creates a default views::NonClientFrameView for use by windows in the
   // Ash environment.
   views::NonClientFrameView* CreateDefaultNonClientFrameView(
@@ -494,6 +499,9 @@ class ASH_EXPORT Shell : internal::SystemModalContainerEventFilterDelegate{
   // Used by ash/shell.
   content::BrowserContext* browser_context_;
 
+  // For testing only: simulate that a modal window is open
+  bool simulate_modal_window_open_for_testing_;
+
   DISALLOW_COPY_AND_ASSIGN(Shell);
 };