Fix slider accessibility

1. the initial value change on sliders sometimes did not get dispatched because
a client user of the Slider class can and does set the initial slider value
before attaching it to a widget and making that widget visible. Fire the value
change when visibility changes for the slider if needed.

An example of where this occurs is the Chrome OS volume slider which changes
without it actually receiving focus. The system tray briefly shows the volume
control.

2. ChromeVox specific: make ChromeVox output the entire node including role and
any states when it first encounters the node. Note that these nodes are
transient so disappear quickly. We don't reset the value of
|lastValueChangedTarget_ as a result.

Test: manual.
Bug: 755392
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I86da8d1f5aed933dcc36a460202402f576a8a5f9
Reviewed-on: https://chromium-review.googlesource.com/777599
Commit-Queue: David Tseng <dtseng@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520681}

chrome/browser/chromeos/accessibility/spoken_feedback_browsertest.cc

@@ -389,8 +389,10 @@ IN_PROC_BROWSER_TEST_P(SpokenFeedbackTest, OpenStatusTray) {
   }
   EXPECT_TRUE(base::MatchPattern(speech_monitor_.GetNextUtterance(), "time *"));
   EXPECT_TRUE(base::MatchPattern(speech_monitor_.GetNextUtterance(),
-                                 "Battery is*full.,"));
-  EXPECT_EQ("window", speech_monitor_.GetNextUtterance());
+                                 "Battery is*full."));
+  EXPECT_EQ("Dialog", speech_monitor_.GetNextUtterance());
+  EXPECT_TRUE(
+      base::MatchPattern(speech_monitor_.GetNextUtterance(), "*window"));
 }
 
 // Fails on ASAN. See http://crbug.com/776308 . (Note MAYBE_ doesn't work well
@@ -407,7 +409,7 @@ IN_PROC_BROWSER_TEST_P(SpokenFeedbackTest, NavigateSystemTray) {
   }
   while (true) {
     std::string utterance = speech_monitor_.GetNextUtterance();
-    if (base::MatchPattern(utterance, "window"))
+    if (base::MatchPattern(utterance, "*window"))
       break;
   }
 

chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js

@@ -44,6 +44,9 @@ DesktopAutomationHandler = function(node) {
    */
   this.lastValueChanged_ = new Date(0);
 
+  /** @private {AutomationNode} */
+  this.lastValueTarget_ = null;
+
   this.addListener_(
       EventType.ACTIVEDESCENDANTCHANGED, this.onActiveDescendantChanged);
   this.addListener_(EventType.ALERT, this.onAlert);
@@ -449,7 +452,8 @@ DesktopAutomationHandler.prototype = {
       return;
 
     var t = evt.target;
-    if (t.state.focused || t.root.role == RoleType.DESKTOP ||
+    var fromDesktop = t.root.role == RoleType.DESKTOP;
+    if (t.state.focused || fromDesktop ||
         AutomationUtil.isDescendantOf(
             ChromeVoxState.instance.currentRange.start.node, t)) {
       if (new Date() - this.lastValueChanged_ <=
@@ -460,10 +464,17 @@ DesktopAutomationHandler.prototype = {
 
       var output = new Output();
 
-      if (t.root.role == RoleType.DESKTOP)
+      if (fromDesktop &&
+          (!this.lastValueTarget_ || this.lastValueTarget_ !== t)) {
         output.withQueueMode(cvox.QueueMode.FLUSH);
-
-      output.format('$value', evt.target).go();
+        var range = cursors.Range.fromNode(t);
+        output.withRichSpeechAndBraille(
+            range, range, Output.EventType.NAVIGATE);
+        this.lastValueTarget_ = t;
+      } else {
+        output.format('$value', t);
+      }
+      output.go();
     }
   },
 

ui/views/controls/slider.cc

@@ -67,7 +67,9 @@ constexpr int kSlideHighlightChangeDurationMs = 150;
 const char Slider::kViewClassName[] = "Slider";
 
 Slider::Slider(SliderListener* listener)
-    : listener_(listener), highlight_animation_(this) {
+    : listener_(listener),
+      highlight_animation_(this),
+      pending_accessibility_value_change_(false) {
   highlight_animation_.SetSlideDuration(kSlideHighlightChangeDurationMs);
   EnableCanvasFlippingForRTLUI(true);
 #if defined(OS_MACOSX)
@@ -152,8 +154,15 @@ void Slider::SetValueInternal(float value, SliderChangeReason reason) {
   } else {
     SchedulePaint();
   }
-  if (accessibility_events_enabled_ && GetWidget())
-    NotifyAccessibilityEvent(ui::AX_EVENT_VALUE_CHANGED, true);
+
+  if (accessibility_events_enabled_) {
+    if (GetWidget() && GetWidget()->IsVisible()) {
+      DCHECK(!pending_accessibility_value_change_);
+      NotifyAccessibilityEvent(ui::AX_EVENT_VALUE_CHANGED, true);
+    } else {
+      pending_accessibility_value_change_ = true;
+    }
+  }
 }
 
 void Slider::PrepareForMove(const int new_x) {
@@ -318,6 +327,24 @@ void Slider::OnBlur() {
   SchedulePaint();
 }
 
+void Slider::VisibilityChanged(View* starting_from, bool is_visible) {
+  if (is_visible)
+    NotifyPendingAccessibilityValueChanged();
+}
+
+void Slider::AddedToWidget() {
+  if (GetWidget()->IsVisible())
+    NotifyPendingAccessibilityValueChanged();
+}
+
+void Slider::NotifyPendingAccessibilityValueChanged() {
+  if (!pending_accessibility_value_change_)
+    return;
+
+  NotifyAccessibilityEvent(ui::AX_EVENT_VALUE_CHANGED, true);
+  pending_accessibility_value_change_ = false;
+}
+
 void Slider::OnGestureEvent(ui::GestureEvent* event) {
   switch (event->type()) {
     // In a multi point gesture only the touch point will generate

ui/views/controls/slider.h

@@ -104,6 +104,8 @@ class VIEWS_EXPORT Slider : public View, public gfx::AnimationDelegate {
   void OnPaint(gfx::Canvas* canvas) override;
   void OnFocus() override;
   void OnBlur() override;
+  void VisibilityChanged(View* starting_from, bool is_visible) override;
+  void AddedToWidget() override;
 
   // ui::EventHandler:
   void OnGestureEvent(ui::GestureEvent* event) override;
@@ -112,6 +114,8 @@ class VIEWS_EXPORT Slider : public View, public gfx::AnimationDelegate {
     listener_ = listener;
   }
 
+  void NotifyPendingAccessibilityValueChanged();
+
   SliderListener* listener_;
 
   std::unique_ptr<gfx::SlideAnimation> move_animation_;
@@ -135,6 +139,8 @@ class VIEWS_EXPORT Slider : public View, public gfx::AnimationDelegate {
 
   gfx::SlideAnimation highlight_animation_;
 
+  bool pending_accessibility_value_change_;
+
   DISALLOW_COPY_AND_ASSIGN(Slider);
 };
 

ui/views/controls/slider_unittest.cc

@@ -19,6 +19,7 @@
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/views/test/slider_test_api.h"
+#include "ui/views/test/test_views_delegate.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
@@ -123,6 +124,19 @@ namespace views {
 // Base test fixture for Slider tests.
 class SliderTest : public views::ViewsTestBase {
  public:
+  class SliderTestViewsDelegate : public views::TestViewsDelegate {
+   public:
+    bool has_value_changed() { return has_value_changed_; }
+
+   private:
+    void NotifyAccessibilityEvent(View* view, ui::AXEvent event_type) override {
+      if (event_type == ui::AX_EVENT_VALUE_CHANGED)
+        has_value_changed_ = true;
+    }
+
+    bool has_value_changed_ = false;
+  };
+
   SliderTest();
   ~SliderTest() override;
 
@@ -153,6 +167,8 @@ class SliderTest : public views::ViewsTestBase {
     return event_generator_.get();
   }
 
+  SliderTestViewsDelegate* delegate() { return delegate_; }
+
  private:
   // The Slider to be tested.
   Slider* slider_;
@@ -169,12 +185,21 @@ class SliderTest : public views::ViewsTestBase {
   views::Widget* widget_;
   // An event generator.
   std::unique_ptr<ui::test::EventGenerator> event_generator_;
+  // A TestViewsDelegate that intercepts accessibility notifications. Weak.
+  SliderTestViewsDelegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(SliderTest);
 };
 
 SliderTest::SliderTest()
-    : slider_(NULL), default_locale_(), max_x_(0), max_y_(0) {}
+    : slider_(NULL),
+      default_locale_(),
+      max_x_(0),
+      max_y_(0),
+      delegate_(new SliderTestViewsDelegate()) {
+  std::unique_ptr<views::TestViewsDelegate> delegate(delegate_);
+  set_views_delegate(std::move(delegate));
+}
 
 SliderTest::~SliderTest() {
 }
@@ -371,6 +396,32 @@ TEST_F(SliderTest, SliderListenerEventsForMultiFingerScrollGesture) {
   EXPECT_EQ(slider(), slider_listener().last_drag_ended_sender());
 }
 
+// Verifies the correct SliderListener events are raised for an accessible
+// slider.
+TEST_F(SliderTest, SliderRaisesA11yEvents) {
+  EXPECT_FALSE(delegate()->has_value_changed());
+
+  // First, detach/reattach the slider without setting value.
+  // Temporarily detach the slider.
+  View* root_view = slider()->parent();
+  root_view->RemoveChildView(slider());
+
+  // Re-attachment should cause nothing to get fired.
+  root_view->AddChildView(slider());
+  EXPECT_FALSE(delegate()->has_value_changed());
+
+  // Now, set value before reattaching.
+  root_view->RemoveChildView(slider());
+
+  // Value changes won't trigger accessibility events before re-attachment.
+  slider()->SetValue(22);
+  EXPECT_FALSE(delegate()->has_value_changed());
+
+  // Re-attachment should trigger the value change.
+  root_view->AddChildView(slider());
+  EXPECT_TRUE(delegate()->has_value_changed());
+}
+
 #endif  // !defined(OS_MACOSX) || defined(USE_AURA)
 
 }  // namespace views