Do not show navigation panel for system UI nodes.

Some system UI windows (app list, system tray menu) will auto-dismiss when they are deactivated. Since STS panel takes focus, this caused these windows to dismiss as soon as STS was activated on them. Also, navigation features do not work great in system UI, which is UI control heavy and light on content (few paragraph and sentence constructs). Will only show navigation panel on nodes within webviews, pdfs, and Android apps.

Bug: 1157148
Change-Id: I4a92614b861289a0e4a4288bc04e2daffb60c49f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2582966Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Joel Riley <joelriley@google.com>
Cr-Commit-Position: refs/heads/master@{#837546}

chrome/browser/chromeos/accessibility/select_to_speak_browsertest.cc

@@ -8,6 +8,7 @@
 #include "ash/accessibility/accessibility_focus_ring_controller_impl.h"
 #include "ash/accessibility/accessibility_focus_ring_layer.h"
 #include "ash/public/cpp/ash_features.h"
+#include "ash/public/cpp/ash_view_ids.h"
 #include "ash/public/cpp/system_tray_test_api.h"
 #include "ash/root_window_controller.h"
 #include "ash/shell.h"
@@ -32,6 +33,7 @@
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/process_manager.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "ui/accessibility/accessibility_features.h"
 #include "ui/accessibility/accessibility_switches.h"
 #include "ui/events/test/event_generator.h"
 #include "url/url_constants.h"
@@ -74,6 +76,7 @@ class SelectToSpeakTest : public InProcessBrowserTest {
 
   test::SpeechMonitor sm_;
   std::unique_ptr<ui::test::EventGenerator> generator_;
+  std::unique_ptr<ash::SystemTrayTestApi> tray_test_api_;
 
   gfx::Rect GetWebContentsBounds() const {
     // TODO(katie): Find a way to get the exact bounds programmatically.
@@ -143,7 +146,6 @@ class SelectToSpeakTest : public InProcessBrowserTest {
   }
 
  private:
-  std::unique_ptr<ash::SystemTrayTestApi> tray_test_api_;
   scoped_refptr<content::MessageLoopRunner> loop_runner_;
   scoped_refptr<content::MessageLoopRunner> tray_loop_runner_;
   base::WeakPtrFactory<SelectToSpeakTest> weak_ptr_factory_{this};
@@ -447,4 +449,34 @@ IN_PROC_BROWSER_TEST_F(SelectToSpeakTest, WorksWithStickyKeys) {
   sm_.Replay();
 }
 
+/* Test fixture enabling navigation control */
+class SelectToSpeakTestWithNavigationControl : public SelectToSpeakTest {
+ public:
+  void SetUpInProcessBrowserTestFixture() override {
+    scoped_feature_list_.InitAndEnableFeature(
+        ::features::kSelectToSpeakNavigationControl);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(SelectToSpeakTestWithNavigationControl,
+                       SelectToSpeakDoesNotDismissTrayBubble) {
+  // Open tray bubble menu.
+  tray_test_api_->ShowBubble();
+
+  // Search key + click the avatar button.
+  generator_->PressKey(ui::VKEY_LWIN, 0 /* flags */);
+  tray_test_api_->ClickBubbleView(ash::VIEW_ID_USER_AVATAR_BUTTON);
+  generator_->ReleaseKey(ui::VKEY_LWIN, 0 /* flags */);
+
+  // Should read out text.
+  sm_.ExpectSpeechPattern("*stub-user@example.com*");
+  sm_.Replay();
+
+  // Tray bubble menu should remain open.
+  ASSERT_TRUE(tray_test_api_->IsTrayBubbleOpen());
+}
+
 }  // namespace chromeos

chrome/browser/resources/chromeos/accessibility/select_to_speak/node_utils.js

@@ -87,7 +87,7 @@ class NodeUtils {
    */
   static getNearestContainingWindow(node) {
     // Go upwards to root nodes' parents until we find the first window.
-    if (node.root.role === RoleType.ROOT_WEB_AREA) {
+    if (node.root && node.root.role === RoleType.ROOT_WEB_AREA) {
       let nextRootParent = node;
       while (nextRootParent != null &&
              nextRootParent.role !== RoleType.WINDOW &&

chrome/browser/resources/chromeos/accessibility/select_to_speak/select_to_speak.js

@@ -154,7 +154,8 @@ class SelectToSpeak {
      *             currentNodeGroupStartNodeIndex: number,
      *             currentCharIndex: number,
      *             currentStartCharIndex: (number|undefined),
-     *             currentEndCharIndex: (number|undefined)}}
+     *             currentEndCharIndex: (number|undefined),
+     *             supportsNavigationPanel: boolean}}
      */
     this.navigationState_ = {
       /**
@@ -190,6 +191,11 @@ class SelectToSpeak {
        * The current end offset - see currentStartCharIndex, above.
        */
       currentEndCharIndex: undefined,
+
+      /**
+       * Whether the current nodes support use of the navigation panel.
+       */
+      supportsNavigationPanel: true,
     };
 
     /**
@@ -278,7 +284,8 @@ class SelectToSpeak {
    */
   shouldShowNavigationControls_() {
     return this.navigationControlFlag_ &&
-        this.prefsManager_.navigationControlsEnabled();
+        this.prefsManager_.navigationControlsEnabled() &&
+        this.navigationState_.supportsNavigationPanel;
   }
 
   /**
@@ -701,6 +708,7 @@ class SelectToSpeak {
       currentCharIndex: -1,
       currentStartCharIndex: undefined,
       currentEndCharIndex: undefined,
+      supportsNavigationPanel: true,
     };
   }
 
@@ -1123,6 +1131,7 @@ class SelectToSpeak {
       currentCharIndex: 0,
       currentStartCharIndex: startCharIndex,
       currentEndCharIndex: endCharIndex,
+      supportsNavigationPanel: this.isNavigationPanelSupported_(nodes),
     };
 
     // Play TTS according to the current state variables.
@@ -1738,6 +1747,22 @@ class SelectToSpeak {
     return this.overrideSpeechRate_ || this.systemSpeechRate_;
   }
 
+  /**
+   * @param {!Array<!AutomationNode>} nodes
+   * @return {boolean} Whether all given nodes support the navigation panel.
+   * @private
+   */
+  isNavigationPanelSupported_(nodes) {
+    if (nodes.length === 0) {
+      return true;
+    }
+    // Do not show panel on system UI. System UI can be problematic due to
+    // auto-dismissing behavior (see http://crbug.com/1157148), but also
+    // navigation controls do not work well control-rich interfaces that are
+    // light on text (and therefore sentence and paragraph structures).
+    return !nodes.some((n) => n.root && n.root.role === RoleType.DESKTOP);
+  }
+
   /**
    * Fires a mock key down event for testing.
    * @param {!Event} event The fake key down event to fire. The object
@@ -1795,4 +1820,4 @@ SelectToSpeak.READ_SELECTION_KEY_CODE = KeyCode.S;
  * could cause performance issues.
  * @const {number}
  */
-SelectToSpeak.NODE_STATE_TEST_INTERVAL_MS = 500;
+SelectToSpeak.NODE_STATE_TEST_INTERVAL_MS = 500;
\ No newline at end of file