Add a workaround for crbug.com/139556.

* Switch to the next IME when Shift+Alt+Enter (or Space) are pressed and then Enter (or Space) is released. Since most of CJK IMEs use either Enter or Space as a "commit" key, this should work fine for such IME users.
* The workaround might not be good enough for Cangjie/Quick IME users since the IME also uses [0-9] keys for committing a string.

We might have to add additional workaround for switching from a keyboard layout to an IME, but I believe this CL would be a good enough as an initial trial.

BUG=139556
TEST=added some new ash_unittests. also manually checked that the key repeat suppression change by mtomasz is still working fine.


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

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

3 files changed, 73 insertions, 25 deletions

diff --git a/ash/accelerators/accelerator_controller.cc b/ash/accelerators/accelerator_controller.cc
index c37a883..5520e98 100644
--- a/ash/accelerators/accelerator_controller.cc
+++ b/ash/accelerators/accelerator_controller.cc
@@ -310,19 +310,15 @@ bool HandlePrintWindowHierarchy() {
 ////////////////////////////////////////////////////////////////////////////////
 // AcceleratorControllerContext, public:
 
-AcceleratorControllerContext::AcceleratorControllerContext()
-    : repeated_(false),
-      previous_event_type_(ui::ET_UNKNOWN) {
+AcceleratorControllerContext::AcceleratorControllerContext() {
+  current_accelerator_.set_type(ui::ET_UNKNOWN);
+  previous_accelerator_.set_type(ui::ET_UNKNOWN);
 }
 
 void AcceleratorControllerContext::UpdateContext(
     const ui::Accelerator& accelerator) {
-  const ui::Accelerator previous_accelerator = current_accelerator_;
+  previous_accelerator_ = current_accelerator_;
   current_accelerator_ = accelerator;
-
-  // Compute contextual information.
-  repeated_ = previous_accelerator == current_accelerator_;
-  previous_event_type_ = previous_accelerator.type();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -444,7 +440,8 @@ bool AcceleratorController::PerformAction(int action,
     return true;
   }
   // Type of the previous accelerator. Used by NEXT_IME and DISABLE_CAPS_LOCK.
-  const ui::EventType previous_event_type = context_.previous_event_type();
+  const ui::EventType previous_event_type =
+    context_.previous_accelerator().type();
 
   // You *MUST* return true when some action is performed. Otherwise, this
   // function might be called *twice*, via BrowserView::PreHandleKeyboardEvent
@@ -617,7 +614,15 @@ bool AcceleratorController::PerformAction(int action,
       // ET_KEY_RELEASED accelerator for Chrome OS (see ash/accelerators/
       // accelerator_controller.cc) when Shift+Alt+Tab is pressed and then Tab
       // is released.
-      if (previous_event_type == ui::ET_KEY_RELEASED) {
+      if (previous_event_type == ui::ET_KEY_RELEASED &&
+          // Workaround for crbug.com/139556. CJK IME users tend to press
+          // Enter (or Space) and Shift+Alt almost at the same time to commit
+          // an IME string and then switch from the IME to the English layout.
+          // This workaround allows the user to trigger NEXT_IME even if the
+          // user presses Shift+Alt before releasing Enter.
+          // TODO(nona|mazda): Fix crbug.com/139556 in a cleaner way.
+          context_.previous_accelerator().key_code() != ui::VKEY_RETURN &&
+          context_.previous_accelerator().key_code() != ui::VKEY_SPACE) {
         // We totally ignore this accelerator.
         // TODO(mazda): Fix crbug.com/158217
         return false;
diff --git a/ash/accelerators/accelerator_controller.h b/ash/accelerators/accelerator_controller.h
index dd099ee..ad55e15 100644
--- a/ash/accelerators/accelerator_controller.h
+++ b/ash/accelerators/accelerator_controller.h
@@ -39,18 +39,18 @@ class ASH_EXPORT AcceleratorControllerContext {
   // event type of the previous accelerator.
   void UpdateContext(const ui::Accelerator& accelerator);
 
-  ui::EventType previous_event_type() const { return previous_event_type_; }
-  bool repeated() const { return repeated_; }
+  const ui::Accelerator& previous_accelerator() const {
+    return previous_accelerator_;
+  }
+  bool repeated() const {
+    return current_accelerator_ == previous_accelerator_ &&
+        current_accelerator_.type() != ui::ET_UNKNOWN;
+  }
 
  private:
   ui::Accelerator current_accelerator_;
-
-  // If the current accelerator was repeated.
-  bool repeated_;
-
-  // Event type of the previous accelerator. Used for NEXT_IME and
-  // DISABLE_CAPS_LOCK accelerator actions.
-  ui::EventType previous_event_type_;
+  // Used for NEXT_IME and DISABLE_CAPS_LOCK accelerator actions.
+  ui::Accelerator previous_accelerator_;
 
   DISALLOW_COPY_AND_ASSIGN(AcceleratorControllerContext);
 };
diff --git a/ash/accelerators/accelerator_controller_unittest.cc b/ash/accelerators/accelerator_controller_unittest.cc
index 69977c3..070dd02 100644
--- a/ash/accelerators/accelerator_controller_unittest.cc
+++ b/ash/accelerators/accelerator_controller_unittest.cc
@@ -497,27 +497,28 @@ TEST_F(AcceleratorControllerTest, ControllerContext) {
   accelerator_b.set_type(ui::ET_KEY_PRESSED);
 
   EXPECT_FALSE(GetController()->context()->repeated());
-  EXPECT_EQ(ui::ET_UNKNOWN, GetController()->context()->previous_event_type());
+  EXPECT_EQ(ui::ET_UNKNOWN,
+            GetController()->context()->previous_accelerator().type());
 
   GetController()->context()->UpdateContext(accelerator_a);
   EXPECT_FALSE(GetController()->context()->repeated());
-  EXPECT_EQ(ui::ET_KEY_PRESSED,
-            GetController()->context()->previous_event_type());
+  EXPECT_EQ(ui::ET_UNKNOWN,
+            GetController()->context()->previous_accelerator().type());
 
   GetController()->context()->UpdateContext(accelerator_a2);
   EXPECT_FALSE(GetController()->context()->repeated());
   EXPECT_EQ(ui::ET_KEY_PRESSED,
-            GetController()->context()->previous_event_type());
+            GetController()->context()->previous_accelerator().type());
 
   GetController()->context()->UpdateContext(accelerator_a2);
   EXPECT_TRUE(GetController()->context()->repeated());
   EXPECT_EQ(ui::ET_KEY_RELEASED,
-            GetController()->context()->previous_event_type());
+            GetController()->context()->previous_accelerator().type());
 
   GetController()->context()->UpdateContext(accelerator_b);
   EXPECT_FALSE(GetController()->context()->repeated());
   EXPECT_EQ(ui::ET_KEY_RELEASED,
-            GetController()->context()->previous_event_type());
+            GetController()->context()->previous_accelerator().type());
 }
 
 TEST_F(AcceleratorControllerTest, SuppressToggleMaximized) {
@@ -1030,6 +1031,34 @@ TEST_F(AcceleratorControllerTest, ImeGlobalAccelerators) {
     EXPECT_FALSE(ProcessWithContext(shift_alt_x));
     EXPECT_FALSE(ProcessWithContext(shift_alt));
     EXPECT_EQ(2, delegate->handle_next_ime_count());
+
+    // But we _should_ if X is either VKEY_RETURN or VKEY_SPACE.
+    // TODO(nona|mazda): Remove this when crbug.com/139556 in a better way.
+    const ui::Accelerator shift_alt_return_press(
+        ui::VKEY_RETURN,
+        ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN);
+    const ReleaseAccelerator shift_alt_return(
+        ui::VKEY_RETURN,
+        ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN);
+
+    EXPECT_FALSE(ProcessWithContext(shift_alt_press));
+    EXPECT_FALSE(ProcessWithContext(shift_alt_return_press));
+    EXPECT_FALSE(ProcessWithContext(shift_alt_return));
+    EXPECT_TRUE(ProcessWithContext(shift_alt));
+    EXPECT_EQ(3, delegate->handle_next_ime_count());
+
+    const ui::Accelerator shift_alt_space_press(
+        ui::VKEY_SPACE,
+        ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN);
+    const ReleaseAccelerator shift_alt_space(
+        ui::VKEY_SPACE,
+        ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN);
+
+    EXPECT_FALSE(ProcessWithContext(shift_alt_press));
+    EXPECT_FALSE(ProcessWithContext(shift_alt_space_press));
+    EXPECT_FALSE(ProcessWithContext(shift_alt_space));
+    EXPECT_TRUE(ProcessWithContext(shift_alt));
+    EXPECT_EQ(4, delegate->handle_next_ime_count());
   }
 
 #if defined(OS_CHROMEOS)
@@ -1068,6 +1097,20 @@ TEST_F(AcceleratorControllerTest, ImeGlobalAccelerators) {
 #endif
 }
 
+// TODO(nona|mazda): Remove this when crbug.com/139556 in a better way.
+TEST_F(AcceleratorControllerTest, ImeGlobalAcceleratorsWorkaround139556) {
+  // The workaround for crbug.com/139556 depends on the fact that we don't
+  // use Shift+Alt+Enter/Space with ET_KEY_PRESSED as an accelerator. Test it.
+  const ui::Accelerator shift_alt_return_press(
+      ui::VKEY_RETURN,
+      ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN);
+  EXPECT_FALSE(ProcessWithContext(shift_alt_return_press));
+  const ui::Accelerator shift_alt_space_press(
+      ui::VKEY_SPACE,
+      ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN);
+  EXPECT_FALSE(ProcessWithContext(shift_alt_space_press));
+}
+
 TEST_F(AcceleratorControllerTest, ReservedAccelerators) {
   // (Shift+)Alt+Tab and Chrome OS top-row keys are reserved.
   EXPECT_TRUE(GetController()->IsReservedAccelerator(