Fix tablet mode crash when virtual keyboard is disabled.

If a physical keyboard is connected to a tablet, the virtual keyboard is
disabled and trying to show/hide it will trigger a DCHECK.

My newly introduced code in |AssistantDialogPlate| did not take that
into account (it now checks KeyboardUiController::IsEnabled()).

While writing the unittest to test this, I discovered the existing
|AshTestBase::SetTouchKeyboardEnabled| did not actually work:
   - Trying to enable the keyboard by using
     |SetTouchKeyboardEnabled(true)| does not actually enable the
     keyboard, as it was immediately disabled again as soon as the
     |DeviceDataManager| detected the presence of the actual physical
     keyboard connected to the test PC.
     To work around this, everybody who used this method also set the
     command line switch |kEnableVirtualKeyboard|, which simply
     overruled the value set by |SetTouchKeyboardEnabled|.
   - Trying to disable the keyboard later on by using
     |SetTouchKeyboardEnabled(false)| didn't disable the keyboard as
     the change was overruled by the command line switch
     |kEnableVirtualKeyboard|.
So to solve this, I removed the |SetTouchKeyboardEnabled| and replaced
it with a |AshTestBase::SetVirtualKeyboardEnabled| method which uses a
different set of flags (|kPolicyEnabled|/|kPolicyDisabled|) which are
always respected during the unittests.

Bug: b/147700828
TBR: shend@
Change-Id: I6137993a4ce5200cff400e882820580fa87d592c
Tests: New ash_unittest |AssistantPageViewTabletModeTest.ShouldNotShowKeyboardWhenItsDisabled|.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2002742
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734188}

ash/app_list/views/assistant/assistant_dialog_plate.cc

@@ -52,6 +52,8 @@ constexpr base::TimeDelta kAnimationTransformInDuration =
     base::TimeDelta::FromMilliseconds(333);
 constexpr int kAnimationTranslationDip = 30;
 
+using keyboard::KeyboardUIController;
+
 // Textfield used for inputting text based Assistant queries.
 class AssistantTextfield : public views::Textfield {
  public:
@@ -63,8 +65,18 @@ class AssistantTextfield : public views::Textfield {
   const char* GetClassName() const override { return "AssistantTextfield"; }
 };
 
-void HideKeyboard() {
-  keyboard::KeyboardUIController::Get()->HideKeyboardImplicitlyByUser();
+void ShowKeyboardIfEnabled() {
+  auto* keyboard_controller = KeyboardUIController::Get();
+
+  if (keyboard_controller->IsEnabled())
+    keyboard_controller->ShowKeyboard(/*lock=*/false);
+}
+
+void HideKeyboardIfEnabled() {
+  auto* keyboard_controller = KeyboardUIController::Get();
+
+  if (keyboard_controller->IsEnabled())
+    keyboard_controller->HideKeyboardImplicitlyByUser();
 }
 
 }  // namespace
@@ -118,7 +130,7 @@ bool AssistantDialogPlate::HandleKeyEvent(views::Textfield* textfield,
       // In tablet mode the virtual keyboard should not be sticky, so we hide it
       // when committing a query.
       if (delegate_->IsTabletMode())
-        HideKeyboard();
+        HideKeyboardIfEnabled();
 
       const base::StringPiece16& trimmed_text = base::TrimWhitespace(
           textfield_->GetText(), base::TrimPositions::TRIM_ALL);
@@ -254,7 +266,7 @@ void AssistantDialogPlate::OnUiVisibilityChanged(
     // plate so that text does not persist across Assistant launches.
     textfield_->SetText(base::string16());
 
-    HideKeyboard();
+    HideKeyboardIfEnabled();
   }
 }
 
@@ -447,9 +459,9 @@ void AssistantDialogPlate::UpdateKeyboardVisibility() {
   bool should_show_keyboard = (input_modality() == InputModality::kKeyboard);
 
   if (should_show_keyboard)
-    keyboard::KeyboardUIController::Get()->ShowKeyboard(/*lock=*/false);
+    ShowKeyboardIfEnabled();
   else
-    HideKeyboard();
+    HideKeyboardIfEnabled();
 }
 
 void AssistantDialogPlate::OnAnimationStarted(

ash/app_list/views/assistant/assistant_page_view_unittest.cc

@@ -465,6 +465,15 @@ TEST_F(AssistantPageViewTabletModeTest,
   EXPECT_TRUE(IsKeyboardShowing());
 }
 
+TEST_F(AssistantPageViewTabletModeTest, ShouldNotShowKeyboardWhenItsDisabled) {
+  // This tests the scenario where the keyboard is disabled even in tablet mode,
+  // e.g. when an external keyboard is connected to a tablet.
+  DisableKeyboard();
+  ShowAssistantUiInTextMode();
+
+  EXPECT_FALSE(IsKeyboardShowing());
+}
+
 TEST_F(AssistantPageViewTabletModeTest,
        ShouldFocusTextFieldAfterPressingKeyboardInputToggle) {
   ShowAssistantUiInVoiceMode();

ash/assistant/test/assistant_ash_test_base.cc

@@ -8,20 +8,14 @@
 #include <utility>
 
 #include "ash/app_list/app_list_controller_impl.h"
-#include "ash/app_list/views/assistant/assistant_main_view.h"
-#include "ash/app_list/views/assistant/assistant_page_view.h"
 #include "ash/assistant/assistant_controller.h"
 #include "ash/assistant/test/test_assistant_web_view_factory.h"
 #include "ash/keyboard/ui/keyboard_ui_controller.h"
 #include "ash/keyboard/ui/test/keyboard_test_util.h"
 #include "ash/public/cpp/app_list/app_list_features.h"
-#include "ash/public/cpp/keyboard/keyboard_switches.h"
 #include "ash/public/cpp/test/assistant_test_api.h"
 #include "ash/shell.h"
-#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/run_loop.h"
-#include "ui/compositor/scoped_animation_duration_scale_mode.h"
-#include "ui/views/controls/textfield/textfield.h"
 
 namespace ash {
 
@@ -72,10 +66,6 @@ void AssistantAshTestBase::SetUp() {
   scoped_feature_list_.InitAndEnableFeature(
       app_list_features::kEnableAssistantLauncherUI);
 
-  // Enable virtual keyboard.
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(
-      keyboard::switches::kEnableVirtualKeyboard);
-
   AshTestBase::SetUp();
 
   // Make the display big enough to hold the app list.
@@ -95,14 +85,13 @@ void AssistantAshTestBase::SetUp() {
 
   test_api_->DisableAnimations();
 
-  // Wait for virtual keyboard to load.
-  SetTouchKeyboardEnabled(true);
+  EnableKeyboard();
 }
 
 void AssistantAshTestBase::TearDown() {
   windows_.clear();
   widgets_.clear();
-  SetTouchKeyboardEnabled(false);
+  DisableKeyboard();
   AshTestBase::TearDown();
   scoped_feature_list_.Reset();
 }
@@ -263,7 +252,8 @@ void AssistantAshTestBase::DismissKeyboard() {
 }
 
 bool AssistantAshTestBase::IsKeyboardShowing() const {
-  return keyboard::IsKeyboardShowing();
+  auto* keyboard_controller = keyboard::KeyboardUIController::Get();
+  return keyboard_controller->IsEnabled() && keyboard::IsKeyboardShowing();
 }
 
 AssistantInteractionController* AssistantAshTestBase::interaction_controller() {

ash/assistant/test/assistant_ash_test_base.h

@@ -141,15 +141,17 @@ class AssistantAshTestBase : public AshTestBase {
   // Return the button to enable text mode.
   views::View* keyboard_input_toggle();
 
-  // Show the on-screen keyboard.
+  // Show/Dismiss the on-screen keyboard.
   void ShowKeyboard();
-
-  // Dismiss the on-screen keyboard.
   void DismissKeyboard();
 
   // Returns if the on-screen keyboard is being displayed.
   bool IsKeyboardShowing() const;
 
+  // Enable/Disable the on-screen keyboard.
+  void EnableKeyboard() { SetVirtualKeyboardEnabled(true); }
+  void DisableKeyboard() { SetVirtualKeyboardEnabled(false); }
+
  private:
   AssistantInteractionController* interaction_controller();
   TestAssistantService* assistant_service();

ash/keyboard/virtual_keyboard_controller.cc

@@ -10,7 +10,6 @@
 #include "ash/keyboard/keyboard_controller_impl.h"
 #include "ash/keyboard/ui/keyboard_ui_controller.h"
 #include "ash/keyboard/ui/keyboard_util.h"
-#include "ash/public/cpp/keyboard/keyboard_switches.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
@@ -18,7 +17,6 @@
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
-#include "base/command_line.h"
 #include "base/strings/string_util.h"
 #include "ui/base/emoji/emoji_panel_helper.h"
 #include "ui/display/display.h"
@@ -30,12 +28,6 @@
 namespace ash {
 namespace {
 
-// Checks if virtual keyboard is force-enabled by enable-virtual-keyboard flag.
-bool IsVirtualKeyboardEnabled() {
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      keyboard::switches::kEnableVirtualKeyboard);
-}
-
 void ResetVirtualKeyboard() {
   keyboard::SetKeyboardEnabledFromShelf(false);
 
@@ -136,13 +128,6 @@ void VirtualKeyboardController::UpdateDevices() {
 }
 
 void VirtualKeyboardController::UpdateKeyboardEnabled() {
-  if (IsVirtualKeyboardEnabled()) {
-    keyboard::SetTouchKeyboardEnabled(
-        Shell::Get()
-            ->tablet_mode_controller()
-            ->AreInternalInputDeviceEventsBlocked());
-    return;
-  }
   bool ignore_internal_keyboard = Shell::Get()
                                       ->tablet_mode_controller()
                                       ->AreInternalInputDeviceEventsBlocked();

ash/keyboard/virtual_keyboard_unittest.cc

@@ -9,7 +9,6 @@
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "base/bind_helpers.h"
-#include "base/command_line.h"
 #include "ui/aura/test/test_window_delegate.h"
 #include "ui/events/test/event_generator.h"
 
@@ -21,14 +20,12 @@ class VirtualKeyboardTest : public AshTestBase {
   ~VirtualKeyboardTest() override = default;
 
   void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        keyboard::switches::kEnableVirtualKeyboard);
     AshTestBase::SetUp();
-    keyboard::SetTouchKeyboardEnabled(true);
+    SetVirtualKeyboardEnabled(true);
   }
 
   void TearDown() override {
-    keyboard::SetTouchKeyboardEnabled(false);
+    SetVirtualKeyboardEnabled(false);
     AshTestBase::TearDown();
   }
 

ash/root_window_controller_unittest.cc

@@ -23,7 +23,6 @@
 #include "ash/wm/window_properties.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
-#include "base/command_line.h"
 #include "base/run_loop.h"
 #include "ui/aura/client/focus_change_observer.h"
 #include "ui/aura/client/focus_client.h"
@@ -719,14 +718,12 @@ class VirtualKeyboardRootWindowControllerTest
   ~VirtualKeyboardRootWindowControllerTest() override = default;
 
   void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        keyboard::switches::kEnableVirtualKeyboard);
     AshTestBase::SetUp();
-    SetTouchKeyboardEnabled(true);
+    SetVirtualKeyboardEnabled(true);
   }
 
   void TearDown() override {
-    SetTouchKeyboardEnabled(false);
+    SetVirtualKeyboardEnabled(false);
     AshTestBase::TearDown();
   }
 

ash/test/ash_test_base.cc

@@ -482,12 +482,25 @@ void AshTestBase::UnblockUserSession() {
   GetSessionControllerClient()->UnlockScreen();
 }
 
-void AshTestBase::SetTouchKeyboardEnabled(bool enabled) {
-  auto flag = keyboard::KeyboardEnableFlag::kTouchEnabled;
-  if (enabled)
-    Shell::Get()->keyboard_controller()->SetEnableFlag(flag);
-  else
-    Shell::Get()->keyboard_controller()->ClearEnableFlag(flag);
+void AshTestBase::SetVirtualKeyboardEnabled(bool enabled) {
+  // Note there are a lot of flags that can be set to control whether the
+  // keyboard is shown or not. You can see the logic in
+  // |KeyboardUIController::IsKeyboardEnableRequested|.
+  // The |kTouchEnabled| flag seems like a logical candidate to pick, but it
+  // does not work because the flag will automatically be toggled off once the
+  // |DeviceDataManager| detects there is a physical keyboard present. That's
+  // why I picked the |kPolicyEnabled| and |kPolicyDisabled| flags instead.
+  auto enable_flag = keyboard::KeyboardEnableFlag::kPolicyEnabled;
+  auto disable_flag = keyboard::KeyboardEnableFlag::kPolicyDisabled;
+  auto* keyboard_controller = Shell::Get()->keyboard_controller();
+
+  if (enabled) {
+    keyboard_controller->SetEnableFlag(enable_flag);
+    keyboard_controller->ClearEnableFlag(disable_flag);
+  } else {
+    keyboard_controller->ClearEnableFlag(enable_flag);
+    keyboard_controller->SetEnableFlag(disable_flag);
+  }
   // Ensure that observer methods and mojo calls between KeyboardControllerImpl,
   // keyboard::KeyboardUIController*, and AshKeyboardUI complete.
   base::RunLoop().RunUntilIdle();

ash/test/ash_test_base.h

@@ -264,9 +264,9 @@ class AshTestBase : public testing::Test {
   void BlockUserSession(UserSessionBlockReason block_reason);
   void UnblockUserSession();
 
-  // Enable or disable the keyboard for touch and run the message loop to
-  // allow observer operations to complete.
-  void SetTouchKeyboardEnabled(bool enabled);
+  // Enable or disable the virtual on-screen keyboard and run the message loop
+  // to allow observer operations to complete.
+  void SetVirtualKeyboardEnabled(bool enabled);
 
   void DisableIME();
 

ash/wm/collision_detection/collision_detection_utils_unittest.cc

@@ -17,7 +17,6 @@
 #include "ash/wm/window_state.h"
 #include "ash/wm/wm_event.h"
 #include "base/bind_helpers.h"
-#include "base/command_line.h"
 #include "ui/aura/window.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/wm/core/coordinate_conversion.h"
@@ -82,9 +81,8 @@ class CollisionDetectionUtilsDisplayTest
           std::tuple<std::string, std::size_t>> {
  public:
   void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        keyboard::switches::kEnableVirtualKeyboard);
     AshTestBase::SetUp();
+    SetVirtualKeyboardEnabled(true);
 
     const std::string& display_string = std::get<0>(GetParam());
     const std::size_t root_window_index = std::get<1>(GetParam());
@@ -206,7 +204,7 @@ TEST_P(CollisionDetectionUtilsDisplayTest, RestingPositionSnapsInsideDisplay) {
 
 TEST_P(CollisionDetectionUtilsDisplayTest,
        RestingPositionWorksIfKeyboardIsDisabled) {
-  SetTouchKeyboardEnabled(false);
+  SetVirtualKeyboardEnabled(false);
   auto display = GetDisplay();
 
   // Snap near top edge to top.

ash/wm/pip/pip_window_resizer_unittest.cc

@@ -21,7 +21,6 @@
 #include "ash/wm/window_state.h"
 #include "ash/wm/wm_event.h"
 #include "base/bind_helpers.h"
-#include "base/command_line.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window.h"
@@ -80,10 +79,8 @@ class PipWindowResizerTest : public AshTestBase,
   ~PipWindowResizerTest() override = default;
 
   void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        keyboard::switches::kEnableVirtualKeyboard);
     AshTestBase::SetUp();
-    SetTouchKeyboardEnabled(true);
+    SetVirtualKeyboardEnabled(true);
 
     const std::string& display_string = std::get<0>(GetParam());
     const std::size_t root_window_index = std::get<1>(GetParam());
@@ -96,7 +93,7 @@ class PipWindowResizerTest : public AshTestBase,
 
   void TearDown() override {
     scoped_root_.reset();
-    SetTouchKeyboardEnabled(false);
+    SetVirtualKeyboardEnabled(false);
     AshTestBase::TearDown();
   }
 

ash/wm/workspace/workspace_layout_manager_unittest.cc

@@ -48,7 +48,6 @@
 #include "ash/wm/workspace/workspace_window_resizer.h"
 #include "ash/wm/workspace_controller_test_api.h"
 #include "base/bind_helpers.h"
-#include "base/command_line.h"
 #include "base/run_loop.h"
 #include "chromeos/audio/chromeos_sounds.h"
 #include "ui/aura/client/aura_constants.h"
@@ -1906,10 +1905,8 @@ class WorkspaceLayoutManagerSystemUiAreaTest : public AshTestBase {
 
   // AshTestBase:
   void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        keyboard::switches::kEnableVirtualKeyboard);
     AshTestBase::SetUp();
-    SetTouchKeyboardEnabled(true);
+    SetVirtualKeyboardEnabled(true);
 
     window_ = CreateTestWindowInShellWithBounds(gfx::Rect(0, 0, 100, 100));
     WindowState* window_state = WindowState::Get(window_);
@@ -1919,7 +1916,7 @@ class WorkspaceLayoutManagerSystemUiAreaTest : public AshTestBase {
   }
 
   void TearDown() override {
-    SetTouchKeyboardEnabled(false);
+    SetVirtualKeyboardEnabled(false);
     AshTestBase::TearDown();
   }