CrOS Settings: Only show shelf navigation setting in tablet mode

Currently the shelf navigation setting is supposed to be hidden on
clamshell-only devices. PowerManagerClient is used to detect when tablet
mode is unsupported but has not been accurately been detecting when
this is the case.

After discussion with UX, it has been decided to instead only show the
setting when in tablet mode. This will ensure that clamshell-only users
are not shown a setting intended for tablet mode users.

This change removes usage of PowerManagerClient and instead uses
ash::TabletMode::Get()->InTabletMode() to determine when the shelf
navigation setting should be shown in a11y settings.
ash::TabletModeObserver is also used so that when tablet mode is changed
the setting is shown in tablet mode and hidden in clamshell mode.

Bug: 1062213
Change-Id: I01a7c3946c557dc65eebd69b52b63240aa39461e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2197868Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Matthew Mourgos <mmourgos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#768513}

chrome/browser/resources/settings/chromeos/os_a11y_page/manage_a11y_page.js

@@ -211,6 +211,9 @@ Polymer({
         'initial-data-ready', this.onManageAllyPageReady_.bind(this));
     chrome.send('manageA11yPageReady');
 
+    this.addWebUIListener(
+        'tablet-mode-changed', this.onTabletModeChanged_.bind(this));
+
     const r = settings.routes;
     this.addFocusConfig_(r.MANAGE_TTS_SETTINGS, '#ttsSubpageButton');
     this.addFocusConfig_(r.MANAGE_CAPTION_SETTINGS, '#captionsSubpageButton');
@@ -380,16 +383,27 @@ Polymer({
         /* dynamicParams */ null, /* removeSearch */ true);
   },
 
+  /**
+   * Called when tablet mode is changed. Handles updating the visibility of the
+   * shelf navigation buttons setting.
+   * @param {boolean} tabletModeEnabled Whether tablet mode is enabled.
+   * @private
+   */
+  onTabletModeChanged_(tabletModeEnabled) {
+    this.showShelfNavigationButtonsSettings_ = tabletModeEnabled &&
+        loadTimeData.getBoolean('showTabletModeShelfNavigationButtonsSettings');
+  },
+
   /**
    * Handles updating the visibility of the shelf navigation buttons setting
    * and updating whether startupSoundEnabled is checked.
    * @param {boolean} startup_sound_enabled Whether startup sound is enabled.
-   * @param {boolean} tablet_mode_supported Whether tablet mode is supported.
+   * @param {boolean} tabletModeEnabled Whether tablet mode is enabled.
    * @private
    */
-  onManageAllyPageReady_(startup_sound_enabled, tablet_mode_supported) {
+  onManageAllyPageReady_(startup_sound_enabled, tabletModeEnabled) {
     this.$.startupSoundEnabled.checked = startup_sound_enabled;
-    this.showShelfNavigationButtonsSettings_ = tablet_mode_supported &&
+    this.showShelfNavigationButtonsSettings_ = tabletModeEnabled &&
         loadTimeData.getBoolean(
             'showTabletModeShelfNavigationButtonsSettings') &&
         !this.isKioskModeActive_;

chrome/browser/ui/webui/settings/chromeos/accessibility_handler.cc

@@ -29,14 +29,24 @@ void RecordShowShelfNavigationButtonsValueChange(bool enabled) {
       enabled);
 }
 
+bool IsTabletModeEnabled() {
+  return ash::TabletMode::Get() && ash::TabletMode::Get()->InTabletMode();
+}
+
 }  // namespace
 
 AccessibilityHandler::AccessibilityHandler(Profile* profile)
-    : profile_(profile) {}
+    : profile_(profile) {
+  if (ash::TabletMode::Get())
+    ash::TabletMode::Get()->AddObserver(this);
+}
 
 AccessibilityHandler::~AccessibilityHandler() {
   if (a11y_nav_buttons_toggle_metrics_reporter_timer_.IsRunning())
     a11y_nav_buttons_toggle_metrics_reporter_timer_.FireNow();
+
+  if (ash::TabletMode::Get())
+    ash::TabletMode::Get()->RemoveObserver(this);
 }
 
 void AccessibilityHandler::RegisterMessages() {
@@ -100,31 +110,18 @@ void AccessibilityHandler::HandleManageA11yPageReady(
     const base::ListValue* args) {
   AllowJavascript();
 
-  // When tablet mode is active we can return early since tablet mode
-  // is supported.
-  if (ash::TabletMode::Get()->InTabletMode()) {
-    FireWebUIListener(
-        "initial-data-ready",
-        base::Value(AccessibilityManager::Get()->GetStartupSoundEnabled()),
-        base::Value(true /* tablet_mode_supported */));
-    return;
-  }
-
-  PowerManagerClient::Get()->GetSwitchStates(
-      base::BindOnce(&AccessibilityHandler::OnReceivedSwitchStates,
-                     weak_ptr_factory_.GetWeakPtr()));
-}
-
-void AccessibilityHandler::OnReceivedSwitchStates(
-    base::Optional<PowerManagerClient::SwitchStates> switch_states) {
-  bool tablet_mode_supported =
-      switch_states.has_value() &&
-      switch_states->tablet_mode != PowerManagerClient::TabletMode::UNSUPPORTED;
-
   FireWebUIListener(
       "initial-data-ready",
       base::Value(AccessibilityManager::Get()->GetStartupSoundEnabled()),
-      base::Value(tablet_mode_supported));
+      base::Value(IsTabletModeEnabled()));
+}
+
+void AccessibilityHandler::OnTabletModeStarted() {
+  FireWebUIListener("tablet-mode-changed", base::Value(IsTabletModeEnabled()));
+}
+
+void AccessibilityHandler::OnTabletModeEnded() {
+  FireWebUIListener("tablet-mode-changed", base::Value(IsTabletModeEnabled()));
 }
 
 void AccessibilityHandler::OpenExtensionOptionsPage(const char extension_id[]) {

chrome/browser/ui/webui/settings/chromeos/accessibility_handler.h

@@ -5,10 +5,10 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_ACCESSIBILITY_HANDLER_H_
 #define CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_ACCESSIBILITY_HANDLER_H_
 
+#include "ash/public/cpp/tablet_mode_observer.h"
 #include "base/macros.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
-#include "chromeos/dbus/power/power_manager_client.h"
 
 namespace base {
 class ListValue;
@@ -19,7 +19,8 @@ class Profile;
 namespace chromeos {
 namespace settings {
 
-class AccessibilityHandler : public ::settings::SettingsPageUIHandler {
+class AccessibilityHandler : public ::settings::SettingsPageUIHandler,
+                             public ash::TabletModeObserver {
  public:
   explicit AccessibilityHandler(Profile* profile);
   ~AccessibilityHandler() override;
@@ -33,6 +34,10 @@ class AccessibilityHandler : public ::settings::SettingsPageUIHandler {
   // mode is supported. Visible for testing.
   void HandleManageA11yPageReady(const base::ListValue* args);
 
+  // ash::TabletModeObserver:
+  void OnTabletModeStarted() override;
+  void OnTabletModeEnded() override;
+
  private:
   // Callback for the messages to show settings for ChromeVox or
   // Select To Speak.
@@ -42,11 +47,6 @@ class AccessibilityHandler : public ::settings::SettingsPageUIHandler {
   void HandleRecordSelectedShowShelfNavigationButtonsValue(
       const base::ListValue* args);
 
-  // Callback which updates visibility for the shelf navigation buttons
-  // accessibility setting, depending on whether tablet mode is supported.
-  void OnReceivedSwitchStates(
-      base::Optional<chromeos::PowerManagerClient::SwitchStates> switch_states);
-
   void OpenExtensionOptionsPage(const char extension_id[]);
 
   Profile* profile_;  // Weak pointer.

chrome/browser/ui/webui/settings/chromeos/accessibility_handler_unittest.cc

@@ -9,7 +9,6 @@
 #include "ash/public/cpp/test/test_tablet_mode.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/in_process_browser_test.h"
-#include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "content/public/test/test_web_ui.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -35,7 +34,6 @@ class AccessibilityHandlerTest : public ChromeRenderViewHostTestHarness {
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
-    PowerManagerClient::InitializeFake();
     test_tablet_mode_ = std::make_unique<ash::TestTabletMode>();
     handler_ = std::make_unique<TestingAccessibilityHandler>(&web_ui_);
   }
@@ -43,7 +41,6 @@ class AccessibilityHandlerTest : public ChromeRenderViewHostTestHarness {
   void TearDown() override {
     handler_.reset();
     test_tablet_mode_.reset();
-    PowerManagerClient::Shutdown();
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
@@ -52,38 +49,15 @@ class AccessibilityHandlerTest : public ChromeRenderViewHostTestHarness {
   std::unique_ptr<TestingAccessibilityHandler> handler_;
 };
 
-// Test that when tablet mode is supported, the correct data is returned by
+// Test that when tablet mode is disabled, the correct data is returned by
 // HandleManageA11yPageReady().
-TEST_F(AccessibilityHandlerTest, ManageA11yPageReadyTabletModeSupported) {
-  // Set tablet mode as supported.
-  chromeos::FakePowerManagerClient::Get()->SetTabletMode(
-      chromeos::PowerManagerClient::TabletMode::OFF, base::TimeTicks());
-
+TEST_F(AccessibilityHandlerTest, ManageA11yPageReadyTabletModeDisabled) {
+  test_tablet_mode_->SetEnabledForTest(false);
   handler_->HandleManageA11yPageReady(/* args */ nullptr);
 
-  // Wait for the AccessibilityHandler to receive data from PowerManagerClient.
-  base::RunLoop().RunUntilIdle();
-
-  const content::TestWebUI::CallData& call_data = *web_ui_.call_data().back();
-
-  // Ensure tablet mode is returned as supported.
-  EXPECT_TRUE(call_data.arg3()->GetBool());
-}
-
-// Test that when tablet mode is unsupported, the correct data is returned by
-// HandleManageA11yPageReady().
-TEST_F(AccessibilityHandlerTest, ManageA11yPageReadyTabletModeUnsupported) {
-  // Set tablet mode as unsupported.
-  chromeos::FakePowerManagerClient::Get()->SetTabletMode(
-      chromeos::PowerManagerClient::TabletMode::UNSUPPORTED, base::TimeTicks());
-  handler_->HandleManageA11yPageReady(/* args */ nullptr);
-
-  // Wait for the AccessibilityHandler to receive data from PowerManagerClient.
-  base::RunLoop().RunUntilIdle();
-
   const content::TestWebUI::CallData& call_data = *web_ui_.call_data().back();
 
-  // Ensure tablet mode is returned as unsupported.
+  // Ensure tablet mode is returned as disabled.
   EXPECT_FALSE(call_data.arg3()->GetBool());
 }
 
@@ -95,7 +69,7 @@ TEST_F(AccessibilityHandlerTest, ManageA11yPageReadyTabletModeEnabled) {
 
   const content::TestWebUI::CallData& call_data = *web_ui_.call_data().back();
 
-  // Ensure tablet mode is returned as supported.
+  // Ensure tablet mode is returned as enabled.
   EXPECT_TRUE(call_data.arg3()->GetBool());
 }