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

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 =

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_;
 

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

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

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_

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

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(),

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);
 };
 

chrome/browser/chromeos/options/wifi_config_view.cc

@@ -25,7 +25,6 @@
 #include "ui/views/controls/combobox/combobox.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/textfield/textfield.h"
-#include "ui/views/focus/focus_manager.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
@@ -378,7 +377,6 @@ WifiConfigView::WifiConfigView(NetworkConfigView* parent, bool show_8021x)
 }
 
 WifiConfigView::~WifiConfigView() {
-  views::FocusManager::set_shortcut_handling_suspended(false);
   if (cert_library_)
     cert_library_->RemoveObserver(this);
 }
@@ -882,7 +880,6 @@ void WifiConfigView::Init(WifiNetwork* wifi, bool show_8021x) {
 
   views::GridLayout* layout = views::GridLayout::CreatePanel(this);
   SetLayoutManager(layout);
-  views::FocusManager::set_shortcut_handling_suspended(true);
 
   const int column_view_set_id = 0;
   views::ColumnSet* column_set = layout->AddColumnSet(column_view_set_id);