ime: Ensure system PK code works ok when switching between VK/PK.

System PK and the VK extension share the same IME service and backend
decoder. This shared state can make it brittle to swap between the two,
especially since they use different APIs.

Rather than handle all the edge cases carefully, we decided to opt for a
more robust solution:

- When the extension is enabled, all events are routed to the extension,
  including PK events.

- When the extension is disabled, all events are routed to the system PK
  code.

To ensure that the extension and system PK start with a clean state, we
always reactivate the current engine (which would send an activate and
focus event) when switching between the two. This guarantees that the
extension and system PK are isolated from each other.

Tested: on real device, flipping back and forth between clamshell/tablet.
Bug: b/172771899
Change-Id: Ibef308a5b8798f5719da038d21320d11c689a3c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567013Reviewed-by: My Nguyen <myy@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835097}

chrome/browser/chromeos/input_method/native_input_method_engine.cc

@@ -38,9 +38,14 @@ bool ShouldUseRuleBasedMojoEngine(const std::string& engine_id) {
 }
 
 bool ShouldUseFstMojoEngine(const std::string& engine_id) {
+  // To avoid handling tricky cases where the user types with both the virtual
+  // and the physical keyboard, only run the native code path if the virtual
+  // keyboard is disabled. Otherwise, just let the extension handle any physical
+  // key events.
   return base::FeatureList::IsEnabled(
              chromeos::features::kSystemLatinPhysicalTyping) &&
-         base::StartsWith(engine_id, "xkb:", base::CompareCase::SENSITIVE);
+         base::StartsWith(engine_id, "xkb:", base::CompareCase::SENSITIVE) &&
+         !ChromeKeyboardControllerClient::Get()->GetKeyboardEnabled();
 }
 
 std::string NormalizeEngineId(const std::string engine_id) {
@@ -132,6 +137,10 @@ void NativeInputMethodEngine::Initialize(
   std::unique_ptr<AutocorrectManager> autocorrect_manager =
       std::make_unique<AutocorrectManager>(this);
   autocorrect_manager_ = autocorrect_manager.get();
+
+  chrome_keyboard_controller_client_observer_.Observe(
+      ChromeKeyboardControllerClient::Get());
+
   // Wrap the given observer in our observer that will decide whether to call
   // Mojo directly or forward to the extension.
   auto native_observer =
@@ -142,6 +151,12 @@ void NativeInputMethodEngine::Initialize(
                                 profile);
 }
 
+void NativeInputMethodEngine::OnKeyboardEnabledChanged(bool enabled) {
+  // Re-activate the engine whenever the virtual keyboard is enabled or disabled
+  // so that the native or extension state is reset correctly.
+  Enable(GetActiveComponentId());
+}
+
 void NativeInputMethodEngine::FlushForTesting() {
   GetNativeObserver()->FlushForTesting();
 }
@@ -164,10 +179,10 @@ NativeInputMethodEngine::GetNativeObserver() const {
 }
 
 NativeInputMethodEngine::ImeObserver::ImeObserver(
-    std::unique_ptr<InputMethodEngineBase::Observer> base_observer,
+    std::unique_ptr<InputMethodEngineBase::Observer> ime_base_observer,
     std::unique_ptr<AssistiveSuggester> assistive_suggester,
     std::unique_ptr<AutocorrectManager> autocorrect_manager)
-    : base_observer_(std::move(base_observer)),
+    : ime_base_observer_(std::move(ime_base_observer)),
       receiver_from_engine_(this),
       assistive_suggester_(std::move(assistive_suggester)),
       autocorrect_manager_(std::move(autocorrect_manager)) {}
@@ -210,7 +225,7 @@ void NativeInputMethodEngine::ImeObserver::OnActivate(
     ui::SetShowEmojiKeyboardCallback(
         base::BindRepeating(&EmojiPickerDialog::Show));
   }
-  base_observer_->OnActivate(engine_id);
+  ime_base_observer_->OnActivate(engine_id);
 }
 void NativeInputMethodEngine::ImeObserver::ProcessMessage(
     const std::vector<uint8_t>& message,
@@ -234,9 +249,9 @@ void NativeInputMethodEngine::ImeObserver::OnFocus(
         context.should_do_learning
             ? ime::mojom::PersonalizationMode::kEnabled
             : ime::mojom::PersonalizationMode::kDisabled));
+  } else {
+    ime_base_observer_->OnFocus(context);
   }
-
-  base_observer_->OnFocus(context);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnBlur(int context_id) {
@@ -246,9 +261,9 @@ void NativeInputMethodEngine::ImeObserver::OnBlur(int context_id) {
   if (active_engine_id_ && ShouldUseFstMojoEngine(*active_engine_id_) &&
       remote_to_engine_.is_bound()) {
     remote_to_engine_->OnBlur();
+  } else {
+    ime_base_observer_->OnBlur(context_id);
   }
-
-  base_observer_->OnBlur(context_id);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnKeyEvent(
@@ -282,20 +297,20 @@ void NativeInputMethodEngine::ImeObserver::OnKeyEvent(
              remote_to_engine_.is_bound()) {
     remote_to_engine_->OnKeyEvent(std::move(key_event), std::move(callback));
   } else {
-    base_observer_->OnKeyEvent(engine_id, event, std::move(callback));
+    ime_base_observer_->OnKeyEvent(engine_id, event, std::move(callback));
   }
 }
 
 void NativeInputMethodEngine::ImeObserver::OnReset(
     const std::string& engine_id) {
-  if (remote_to_engine_.is_bound()) {
-    if (ShouldUseRuleBasedMojoEngine(engine_id)) {
-      remote_to_engine_->ResetForRulebased();
-    } else if (ShouldUseFstMojoEngine(engine_id)) {
-      remote_to_engine_->OnCompositionCanceled();
-    }
+  if (remote_to_engine_.is_bound() && ShouldUseRuleBasedMojoEngine(engine_id)) {
+    remote_to_engine_->ResetForRulebased();
+  } else if (remote_to_engine_.is_bound() &&
+             ShouldUseFstMojoEngine(engine_id)) {
+    remote_to_engine_->OnCompositionCanceled();
+  } else {
+    ime_base_observer_->OnReset(engine_id);
   }
-  base_observer_->OnReset(engine_id);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnDeactivated(
@@ -303,12 +318,12 @@ void NativeInputMethodEngine::ImeObserver::OnDeactivated(
   if (ShouldUseRuleBasedMojoEngine(engine_id)) {
     remote_to_engine_.reset();
   }
-  base_observer_->OnDeactivated(engine_id);
+  ime_base_observer_->OnDeactivated(engine_id);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnCompositionBoundsChanged(
     const std::vector<gfx::Rect>& bounds) {
-  base_observer_->OnCompositionBoundsChanged(bounds);
+  ime_base_observer_->OnCompositionBoundsChanged(bounds);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnSurroundingTextChanged(
@@ -330,16 +345,17 @@ void NativeInputMethodEngine::ImeObserver::OnSurroundingTextChanged(
     selection->focus = cursor_pos;
     remote_to_engine_->OnSurroundingTextChanged(
         base::UTF16ToUTF8(text), offset_pos, std::move(selection));
+  } else {
+    ime_base_observer_->OnSurroundingTextChanged(engine_id, text, cursor_pos,
+                                                 anchor_pos, offset_pos);
   }
-  base_observer_->OnSurroundingTextChanged(engine_id, text, cursor_pos,
-                                           anchor_pos, offset_pos);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnCandidateClicked(
     const std::string& component_id,
     int candidate_id,
     InputMethodEngineBase::MouseButtonEvent button) {
-  base_observer_->OnCandidateClicked(component_id, candidate_id, button);
+  ime_base_observer_->OnCandidateClicked(component_id, candidate_id, button);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnAssistiveWindowButtonClicked(
@@ -375,7 +391,7 @@ void NativeInputMethodEngine::ImeObserver::OnAssistiveWindowButtonClicked(
       break;
     case ui::ime::ButtonId::kAddToDictionary:
     case ui::ime::ButtonId::kNone:
-      base_observer_->OnAssistiveWindowButtonClicked(button);
+      ime_base_observer_->OnAssistiveWindowButtonClicked(button);
       break;
   }
 }
@@ -383,22 +399,22 @@ void NativeInputMethodEngine::ImeObserver::OnAssistiveWindowButtonClicked(
 void NativeInputMethodEngine::ImeObserver::OnMenuItemActivated(
     const std::string& component_id,
     const std::string& menu_id) {
-  base_observer_->OnMenuItemActivated(component_id, menu_id);
+  ime_base_observer_->OnMenuItemActivated(component_id, menu_id);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnScreenProjectionChanged(
     bool is_projected) {
-  base_observer_->OnScreenProjectionChanged(is_projected);
+  ime_base_observer_->OnScreenProjectionChanged(is_projected);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnSuggestionsChanged(
     const std::vector<std::string>& suggestions) {
-  base_observer_->OnSuggestionsChanged(suggestions);
+  ime_base_observer_->OnSuggestionsChanged(suggestions);
 }
 
 void NativeInputMethodEngine::ImeObserver::OnInputMethodOptionsChanged(
     const std::string& engine_id) {
-  base_observer_->OnInputMethodOptionsChanged(engine_id);
+  ime_base_observer_->OnInputMethodOptionsChanged(engine_id);
 }
 
 void NativeInputMethodEngine::ImeObserver::CommitText(const std::string& text) {

chrome/browser/chromeos/input_method/native_input_method_engine.h

@@ -5,9 +5,11 @@
 #ifndef CHROME_BROWSER_CHROMEOS_INPUT_METHOD_NATIVE_INPUT_METHOD_ENGINE_H_
 #define CHROME_BROWSER_CHROMEOS_INPUT_METHOD_NATIVE_INPUT_METHOD_ENGINE_H_
 
+#include "base/scoped_observation.h"
 #include "chrome/browser/chromeos/input_method/assistive_suggester.h"
 #include "chrome/browser/chromeos/input_method/autocorrect_manager.h"
 #include "chrome/browser/chromeos/input_method/input_method_engine.h"
+#include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h"
 #include "chromeos/services/ime/public/mojom/input_engine.mojom-forward.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
@@ -27,7 +29,9 @@ namespace chromeos {
 // be no "ImeObserver" for the native engine either, as it is only used as
 // a way for ExtensionInputMethodEngine to delegate to the extensions code,
 // which is not required for the native engine.
-class NativeInputMethodEngine : public InputMethodEngine {
+class NativeInputMethodEngine
+    : public InputMethodEngine,
+      public ChromeKeyboardControllerClient::Observer {
  public:
   NativeInputMethodEngine();
   ~NativeInputMethodEngine() override;
@@ -37,6 +41,9 @@ class NativeInputMethodEngine : public InputMethodEngine {
                   const char* extension_id,
                   Profile* profile) override;
 
+  // ChromeKeyboardControllerClient:
+  void OnKeyboardEnabledChanged(bool enabled) override;
+
   // Flush all relevant Mojo pipes.
   void FlushForTesting();
 
@@ -60,11 +67,13 @@ class NativeInputMethodEngine : public InputMethodEngine {
   class ImeObserver : public InputMethodEngineBase::Observer,
                       public ime::mojom::InputChannel {
    public:
-    // |base_observer| is to forward events to extension during this migration.
-    // It will be removed when the official extension is completely migrated.
-    ImeObserver(std::unique_ptr<InputMethodEngineBase::Observer> base_observer,
-                std::unique_ptr<AssistiveSuggester> assistive_suggester,
-                std::unique_ptr<AutocorrectManager> autocorrect_manager);
+    // |ime_base_observer| is to forward events to extension during this
+    // migration. It will be removed when the official extension is completely
+    // migrated.
+    ImeObserver(
+        std::unique_ptr<InputMethodEngineBase::Observer> ime_base_observer,
+        std::unique_ptr<AssistiveSuggester> assistive_suggester,
+        std::unique_ptr<AutocorrectManager> autocorrect_manager);
     ~ImeObserver() override;
 
     // InputMethodEngineBase::Observer:
@@ -146,7 +155,7 @@ class NativeInputMethodEngine : public InputMethodEngine {
         ui::IMEEngineHandlerInterface::KeyEventDoneCallback callback,
         ime::mojom::KeypressResponseForRulebasedPtr response);
 
-    std::unique_ptr<InputMethodEngineBase::Observer> base_observer_;
+    std::unique_ptr<InputMethodEngineBase::Observer> ime_base_observer_;
     mojo::Remote<ime::mojom::InputEngineManager> remote_manager_;
     mojo::Receiver<ime::mojom::InputChannel> receiver_from_engine_;
     mojo::Remote<ime::mojom::InputChannel> remote_to_engine_;
@@ -160,6 +169,9 @@ class NativeInputMethodEngine : public InputMethodEngine {
 
   AssistiveSuggester* assistive_suggester_ = nullptr;
   AutocorrectManager* autocorrect_manager_ = nullptr;
+  base::ScopedObservation<ChromeKeyboardControllerClient,
+                          ChromeKeyboardControllerClient::Observer>
+      chrome_keyboard_controller_client_observer_{this};
 };
 
 }  // namespace chromeos

chrome/browser/chromeos/input_method/native_input_method_engine_unittest.cc

@@ -6,7 +6,7 @@
 
 #include "base/feature_list.h"
 #include "chrome/browser/chromeos/input_method/stub_input_method_engine_observer.h"
-#include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h"
+#include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client_test_helper.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "chromeos/services/ime/mock_input_channel.h"
@@ -96,14 +96,15 @@ class NativeInputMethodEngineTest : public ::testing::Test {
     ui::IMEBridge::Initialize();
 
     // Needed by NativeInputMethodEngine for the virtual keyboard.
-    keyboard_controller_client_ =
-        ChromeKeyboardControllerClient::CreateForTest();
+    keyboard_controller_client_test_helper_ =
+        ChromeKeyboardControllerClientTestHelper::InitializeWithFake();
   }
 
  private:
   content::BrowserTaskEnvironment task_environment_;
   base::test::ScopedFeatureList feature_list_;
-  std::unique_ptr<ChromeKeyboardControllerClient> keyboard_controller_client_;
+  std::unique_ptr<ChromeKeyboardControllerClientTestHelper>
+      keyboard_controller_client_test_helper_;
 };
 
 TEST_F(NativeInputMethodEngineTest, EnableCallsRightMojoFunctions) {

chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.cc

@@ -272,6 +272,10 @@ void ChromeKeyboardControllerClient::OnKeyboardEnabledChanged(bool enabled) {
 
   bool was_enabled = is_keyboard_enabled_;
   is_keyboard_enabled_ = enabled;
+
+  for (auto& observer : observers_)
+    observer.OnKeyboardEnabledChanged(is_keyboard_enabled_);
+
   if (enabled || !was_enabled)
     return;
 

chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h

@@ -37,16 +37,16 @@ class ChromeKeyboardControllerClient
       public session_manager::SessionManagerObserver {
  public:
   // Convenience observer allowing UI classes to observe the global instance of
-  // this class instead of setting up mojo bindings.
+  // this class.
   class Observer : public base::CheckedObserver {
    public:
     ~Observer() override = default;
 
-    // Forwards the 'OnKeyboardVisibilityChanged' mojo observer method.
+    // Forwards the 'OnKeyboardVisibilityChanged' observer method.
     // This is used by oobe and login to adjust the UI.
     virtual void OnKeyboardVisibilityChanged(bool visible) {}
 
-    // Forwards the 'OnKeyboardOccludedBoundsChanged' mojo observer method.
+    // Forwards the 'OnKeyboardOccludedBoundsChanged' observer method.
     // This is used to update the insets of browser and app windows when the
     // keyboard is shown.
     virtual void OnKeyboardOccludedBoundsChanged(
@@ -56,6 +56,9 @@ class ChromeKeyboardControllerClient
     // loaded. Note: if the content is already loaded when the observer is
     // added, this will not be triggered, but see is_keyboard_loaded().
     virtual void OnKeyboardLoaded() {}
+
+    // Forwards the 'OnKeyboardEnabledChanged' observer method.
+    virtual void OnKeyboardEnabledChanged(bool enabled) {}
   };
 
   // Creates the singleton instance for chrome or browser tests.