Show a toast when using a virtual keyboard in crostini

Bug: chromium:962848
Test: autoninja -C out/Default unit_tests && ./out/Default/unit_tests
Test: Launch xclock with virtual keyboard hidden and showing
Change-Id: I358c31bddd7607de161f1c71a7be4ca18eb581f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1746023
Auto-Submit: David Munro <davidmunro@google.com>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686299}

chrome/browser/chromeos/crostini/crostini_unsupported_action_notifier.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "ash/public/cpp/app_types.h"
+#include "ash/public/cpp/keyboard/keyboard_controller.h"
 #include "ash/public/cpp/tablet_mode.h"
 #include "ash/public/cpp/toast_manager.h"
 #include "base/logging.h"
@@ -29,12 +30,14 @@ CrostiniUnsupportedActionNotifier::CrostiniUnsupportedActionNotifier(
   delegate_->AddTabletModeObserver(this);
   delegate_->AddFocusObserver(this);
   delegate_->AddInputMethodObserver(this);
+  delegate_->AddKeyboardControllerObserver(this);
 }
 
 CrostiniUnsupportedActionNotifier::~CrostiniUnsupportedActionNotifier() {
   delegate_->RemoveTabletModeObserver(this);
   delegate_->RemoveFocusObserver(this);
   delegate_->RemoveInputMethodObserver(this);
+  delegate_->RemoveKeyboardControllerObserver(this);
 }
 
 // Testing on using Debian/stretch on Eve shows Crostini supports all tested xkb
@@ -62,12 +65,20 @@ void CrostiniUnsupportedActionNotifier::InputMethodChanged(
   ShowIMEUnsupportedNotifictionIfNeeded();
 }
 
+void CrostiniUnsupportedActionNotifier::OnKeyboardVisibilityChanged(
+    bool visible) {
+  if (visible) {
+    ShowVirtualKeyboardUnsupportedNotifictionIfNeeded();
+  }
+}
+
 void CrostiniUnsupportedActionNotifier::
     ShowVirtualKeyboardUnsupportedNotifictionIfNeeded() {
   if (virtual_keyboard_unsupported_message_shown_) {
     return;
   }
-  if (delegate_->IsInTabletMode() && delegate_->IsFocusedWindowCrostini()) {
+  if ((delegate_->IsInTabletMode() || delegate_->IsVirtualKeyboardVisible()) &&
+      delegate_->IsFocusedWindowCrostini()) {
     ash::ToastData data = {
         /*id=*/"VKUnsupportedInCrostini",
         /*text=*/
@@ -124,6 +135,10 @@ CrostiniUnsupportedActionNotifier::Delegate::GetCurrentInputMethod() {
       ->GetCurrentInputMethod();
 }
 
+bool CrostiniUnsupportedActionNotifier::Delegate::IsVirtualKeyboardVisible() {
+  return ash::KeyboardController::Get()->IsKeyboardVisible();
+}
+
 void CrostiniUnsupportedActionNotifier::Delegate::ShowToast(
     const ash::ToastData& toast_data) {
   ash::ToastManager::Get()->Show(toast_data);
@@ -175,4 +190,14 @@ void CrostiniUnsupportedActionNotifier::Delegate::RemoveInputMethodObserver(
   chromeos::input_method::InputMethodManager::Get()->RemoveObserver(observer);
 }
 
+void CrostiniUnsupportedActionNotifier::Delegate::AddKeyboardControllerObserver(
+    ash::KeyboardControllerObserver* observer) {
+  ash::KeyboardController::Get()->AddObserver(observer);
+}
+void CrostiniUnsupportedActionNotifier::Delegate::
+    RemoveKeyboardControllerObserver(
+        ash::KeyboardControllerObserver* observer) {
+  ash::KeyboardController::Get()->RemoveObserver(observer);
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_unsupported_action_notifier.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "ash/public/cpp/keyboard/keyboard_controller_observer.h"
 #include "ash/public/cpp/tablet_mode_observer.h"
 #include "ash/public/cpp/toast_data.h"
 #include "chrome/browser/profiles/profile.h"
@@ -23,7 +24,8 @@ namespace crostini {
 class CrostiniUnsupportedActionNotifier
     : public ash::TabletModeObserver,
       public aura::client::FocusChangeObserver,
-      public chromeos::input_method::InputMethodManager::Observer {
+      public chromeos::input_method::InputMethodManager::Observer,
+      public ash::KeyboardControllerObserver {
  public:
   // Adapter around external integrations which we can mock out for testing,
   // stateless.
@@ -42,6 +44,9 @@ class CrostiniUnsupportedActionNotifier
     virtual chromeos::input_method::InputMethodDescriptor
     GetCurrentInputMethod();
 
+    // Is the current virtual keyboard visible.
+    virtual bool IsVirtualKeyboardVisible();
+
     // Shows a toast to the user.
     virtual void ShowToast(const ash::ToastData& toast_data);
 
@@ -59,6 +64,10 @@ class CrostiniUnsupportedActionNotifier
         chromeos::input_method::InputMethodManager::Observer* observer);
     virtual void RemoveInputMethodObserver(
         chromeos::input_method::InputMethodManager::Observer* observer);
+    virtual void AddKeyboardControllerObserver(
+        ash::KeyboardControllerObserver* observer);
+    virtual void RemoveKeyboardControllerObserver(
+        ash::KeyboardControllerObserver* observer);
   };
 
   CrostiniUnsupportedActionNotifier();
@@ -78,6 +87,9 @@ class CrostiniUnsupportedActionNotifier
                           Profile* profile,
                           bool show_message) override;
 
+  // ash::KeyboardControllerObserver:
+  void OnKeyboardVisibilityChanged(bool visible) override;
+
   Delegate* get_delegate_for_testing() { return delegate_.get(); }
 
  private:
@@ -87,8 +99,8 @@ class CrostiniUnsupportedActionNotifier
   void ShowVirtualKeyboardUnsupportedNotifictionIfNeeded();
 
   // If the user is trying to use an unsupported IME with a crostini app and if
-  // they haven't already been notified that its not supported, notify them.
-  // Generally Crostini supports IMEs with 2:1 mappings betweens keys and glyphs
+  // they haven't already been notified that it's not supported, notify them.
+  // Generally Crostini supports IMEs with 1:1 mappings betweens keys and glyphs
   // e.g. Armenian, and simple combinations like US International, but doesn't
   // support CJK, handwriting, completion, etc.
   void ShowIMEUnsupportedNotifictionIfNeeded();

chrome/browser/chromeos/crostini/crostini_unsupported_action_notifier_unittest.cc

@@ -26,6 +26,7 @@ class MockDelegate : public CrostiniUnsupportedActionNotifier::Delegate {
  public:
   MOCK_METHOD(bool, IsInTabletMode, (), (override));
   MOCK_METHOD(bool, IsFocusedWindowCrostini, (), (override));
+  MOCK_METHOD(bool, IsVirtualKeyboardVisible, (), (override));
   MOCK_METHOD(void, ShowToast, (const ash::ToastData& toast_data), (override));
   MOCK_METHOD(std::string,
               GetLocalizedDisplayName,
@@ -56,6 +57,14 @@ class MockDelegate : public CrostiniUnsupportedActionNotifier::Delegate {
               RemoveInputMethodObserver,
               (chromeos::input_method::InputMethodManager::Observer * observer),
               (override));
+  MOCK_METHOD(void,
+              AddKeyboardControllerObserver,
+              (ash::KeyboardControllerObserver * observer),
+              (override));
+  MOCK_METHOD(void,
+              RemoveKeyboardControllerObserver,
+              (ash::KeyboardControllerObserver * observer),
+              (override));
 };
 
 namespace {
@@ -70,7 +79,7 @@ const InputMethodDescriptor
 }  // namespace
 
 class CrostiniUnsupportedActionNotifierTest
-    : public testing::TestWithParam<std::tuple<bool, bool, bool>> {
+    : public testing::TestWithParam<std::tuple<bool, bool, bool, bool>> {
  public:
   CrostiniUnsupportedActionNotifierTest()
       : notifier(std::make_unique<NiceMock<MockDelegate>>()) {}
@@ -87,7 +96,8 @@ class CrostiniUnsupportedActionNotifierTest
 
   bool is_tablet_mode() const { return std::get<0>(GetParam()); }
   bool is_crostini_focused() const { return std::get<1>(GetParam()); }
-  bool is_ime_unsupported() const { return std::get<2>(GetParam()); }
+  bool is_vk_visible() const { return std::get<2>(GetParam()); }
+  bool is_ime_unsupported() const { return std::get<3>(GetParam()); }
   InputMethodDescriptor ime_descriptor() const {
     return is_ime_unsupported() ? unsupported : supported;
   }
@@ -100,12 +110,18 @@ class CrostiniUnsupportedActionNotifierTest
     return data.id.compare(0, 2, "VK") == 0;
   }
 
-  void SetExpectations(bool show_tablet_toast, bool show_ime_toast) {
-    int num_tablet_toasts = show_tablet_toast ? 1 : 0;
+  void SetExpectations(bool show_tablet_toast,
+                       bool show_vk_toast,
+                       bool show_ime_toast) {
+    // We show the same toast for tablet mode and manually triggered virtual
+    // keyboard, so showing either counts as showing both.
+    int num_tablet_toasts = (show_tablet_toast || show_vk_toast) ? 1 : 0;
     int num_ime_toasts = show_ime_toast ? 1 : 0;
 
     EXPECT_CALL(get_delegate(), IsInTabletMode)
         .WillRepeatedly(Return(is_tablet_mode()));
+    EXPECT_CALL(get_delegate(), IsVirtualKeyboardVisible)
+        .WillRepeatedly(Return(is_vk_visible()));
     EXPECT_CALL(get_delegate(), IsFocusedWindowCrostini)
         .WillRepeatedly(Return(is_crostini_focused()));
     EXPECT_CALL(get_delegate(), GetCurrentInputMethod)
@@ -123,19 +139,39 @@ class CrostiniUnsupportedActionNotifierTest
 TEST_P(CrostiniUnsupportedActionNotifierTest,
        ToastShownOnceOnlyWhenEnteringTabletMode) {
   bool show_tablet_toast = is_tablet_mode() && is_crostini_focused();
+  // Since tablet and vk toasts are the same we can trigger the toast
+  // OnTabletModeStarted even if not in tablet mode.
+  bool show_vk_toast = is_vk_visible() && is_crostini_focused();
   bool show_ime_toast = false;
-  SetExpectations(show_tablet_toast, show_ime_toast);
+
+  SetExpectations(show_tablet_toast, show_vk_toast, show_ime_toast);
 
   notifier.OnTabletModeStarted();
   notifier.OnTabletModeStarted();
 }
 
+TEST_P(CrostiniUnsupportedActionNotifierTest,
+       ToastShownOnceOnlyWhenShowingVirtualKeyboard) {
+  // Since tablet and vk toasts are the same we can trigger the toast
+  // OnKeyboardVisibilityChanged even if the virtual keyboard isn't visible.
+  bool show_tablet_toast = is_tablet_mode() && is_crostini_focused();
+  bool show_vk_toast = is_vk_visible() && is_crostini_focused();
+  bool show_ime_toast = false;
+
+  SetExpectations(show_tablet_toast, show_vk_toast, show_ime_toast);
+
+  notifier.OnKeyboardVisibilityChanged(true);
+  notifier.OnKeyboardVisibilityChanged(false);
+  notifier.OnKeyboardVisibilityChanged(true);
+}
+
 TEST_P(CrostiniUnsupportedActionNotifierTest,
        ToastShownOnceOnlyWhenChangingIME) {
   bool show_tablet_toast = false;
+  bool show_vk_toast = false;
   bool show_ime_toast = is_ime_unsupported() && is_crostini_focused();
 
-  SetExpectations(show_tablet_toast, show_ime_toast);
+  SetExpectations(show_tablet_toast, show_vk_toast, show_ime_toast);
 
   notifier.InputMethodChanged({}, {}, {});
   notifier.InputMethodChanged({}, {}, {});
@@ -144,9 +180,10 @@ TEST_P(CrostiniUnsupportedActionNotifierTest,
 TEST_P(CrostiniUnsupportedActionNotifierTest,
        ToastsShownOnceOnlyWhenFocusingCrostiniApp) {
   bool show_tablet_toast = is_tablet_mode() && is_crostini_focused();
+  bool show_vk_toast = is_vk_visible() && is_crostini_focused();
   bool show_ime_toast = is_ime_unsupported() && is_crostini_focused();
 
-  SetExpectations(show_tablet_toast, show_ime_toast);
+  SetExpectations(show_tablet_toast, show_vk_toast, show_ime_toast);
 
   notifier.OnWindowFocused({}, {});
   notifier.OnWindowFocused({}, {});
@@ -154,6 +191,6 @@ TEST_P(CrostiniUnsupportedActionNotifierTest,
 
 INSTANTIATE_TEST_CASE_P(CrostiniUnsupportedActionNotifierTestCombination,
                         CrostiniUnsupportedActionNotifierTest,
-                        Combine(Bool(), Bool(), Bool()));
+                        Combine(Bool(), Bool(), Bool(), Bool()));
 
 }  // namespace crostini