a11y/Mac: Add screenreader support for SubtleNotificationView announcements.

chrome/browser/ui/views/subtle_notification_view.cc

Index: chrome/browser/ui/views/subtle_notification_view.cc
diff --git a/chrome/browser/ui/views/subtle_notification_view.cc b/chrome/browser/ui/views/subtle_notification_view.cc
index 8d42713d9d59bbfab42d6ea49c257883f5927e4d..5b49381699ff80abb1a8308ca34e65d8186d9646 100644
--- a/chrome/browser/ui/views/subtle_notification_view.cc
+++ b/chrome/browser/ui/views/subtle_notification_view.cc
@@ -7,8 +7,10 @@
 #include <memory>
 
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/accessibility/ax_view_state.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/font_list.h"
 #include "ui/views/bubble/bubble_border.h"
@@ -16,6 +18,7 @@
 #include "ui/views/controls/link.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace {
 
@@ -34,6 +37,9 @@ const int kKeyNameBorderPx = 1;
 const int kKeyNameCornerRadius = 2;
 const int kKeyNamePaddingPx = 5;
 
+// Delimiter indicating there should be a segment displayed as a keyboard key.
+const base::char16 kKeyNameDelimiter[] = {'|', '\0'};
+
 }  // namespace
 
 // Class containing the instruction text. Contains fancy styling on the keyboard
@@ -49,6 +55,7 @@ class SubtleNotificationView::InstructionView : public views::View {
                   SkColor foreground_color,
                   SkColor background_color);
 
+  base::string16 text() const { return text_; }
   void SetText(const base::string16& text);
 
  private:
@@ -91,9 +98,8 @@ void SubtleNotificationView::InstructionView::SetText(
   RemoveAllChildViews(true);
 
   // Parse |text|, looking for pipe-delimited segment.
-  std::vector<base::string16> segments =
-      base::SplitString(text, base::ASCIIToUTF16("|"), base::TRIM_WHITESPACE,
-                        base::SPLIT_WANT_ALL);
+  std::vector<base::string16> segments = base::SplitString(
+      text, kKeyNameDelimiter, base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   // SplitString() returns empty strings for zero-length segments, so given an
   // even number of pipes, there should always be an odd number of segments.
   // The exception is if |text| is entirely empty, in which case the returned
@@ -182,6 +188,9 @@ void SubtleNotificationView::UpdateContent(
   instruction_view_->SetVisible(!instruction_text.empty());
   link_->SetText(link_text);
   link_->SetVisible(!link_text.empty());

[Matt Giuca, 2016/07/14 01:16:02]

I think you have to clear accessible_name_ here, or else GetAccessibleState may
return stale data (it only updates the cache if it is empty).

[Patti Lor, 2016/07/19 01:31:51]

:O Good catch thank you! I ended up changing the way |accessible_name_| works
entirely because of this, see my other comment (on patch set 6).

+
+  NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true);

[dmazzoni, 2016/07/13 23:41:35]

I think AX_EVENT_CHILDREN_CHANGED would be better since |instruction_view_| and
|link_| are the ones changing.

[Patti Lor, 2016/07/19 01:31:51]

Done, thanks. Would you say |link_| and |instruction_view_| need calls to
NotifyAccessibilityEvent() in their SetText() implementations as well, or is
that fine?

[tapted, 2016/07/19 02:46:53]

Yeah - this may depend on how we solve http://crbug.com/610585 (moving the
VoiceOver cursor) and http://crbug.com/610589 (keeping the views::Label inside a
views::Button out of the a11y tree).

In this case, |instruction_view_| has alternating views::Labels and views::Views
(containing a single views:Label). But we wouldn't want the VoiceOver cursor to
iterate over those separately -- they should stop at SubtleNotificationView [or,
possibly, InstructionView], and it should populate AXViewState. a11y hit-tests
on the broken up labels should fail (e.g. by omitting them from as nodes from
the a11y tree)

|link_| isn't used on Mac, but if it was, that can still be a child of
SubtleNotificationView. And I think views::Link::SetText should be responsible
for emitting AX_EVENT_TEXT_CHANGED (and perhaps, e.g., only if has focus).

To emit a AX_EVENT_CHILDREN_CHANGED mapped to a
NSAccessibilityLayoutChangedNotification, we'd need (a) functional a11y children
[InstructionView doesn't properly populate AXViewState right now), and (b) a way
to populate "a userInfo dictionary with the key NSAccessibilityUIElementsKey and
an array containing the UI elements that have been added or changed". That may
still be the right fix, but it should be a TODO

So I think we need a concrete use-case for this - I don't know how OSX assistive
apps will react to NSAccessibilityLayoutChangedNotification without the attached
dictionary.

I'm not even sure we should add the separate AX_EVENT_ALERT below -- is it
needed to make the native VoiceOver work correctly? Should it check that the
widget is visible first (e.g. are we sending 2 identical notifications right
now)? Is UpdateContent ever actually called whilst the widget is already
visible?  If the answers are: no, yes (yes), and no, then the stuff in
OnWidgetVisibilityChanged should be sufficient - doing extra stuff here might
tickle weird bugs.


(But if we can point to an assistive application that doesn't work correctly
without this [either on Mac or elsewhere] then we should definitely explore
this)

+  NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true);
 }
 
 // static
@@ -206,5 +215,34 @@ views::Widget* SubtleNotificationView::CreatePopupWidget(
   // animation. Remove it.
   popup->GetRootView()->SetLayoutManager(nullptr);
 
+  // Allow accessibility events to be sent when the view is shown.
+  popup->AddObserver(view);
+
   return popup;
 }
+
+void SubtleNotificationView::SetAccessibleName(
+    const base::string16& accessible_name) {
+  base::RemoveChars(accessible_name, kKeyNameDelimiter, &accessible_name_);
+}
+
+void SubtleNotificationView::GetAccessibleState(ui::AXViewState* state) {
+  state->role = ui::AX_ROLE_LABEL_TEXT;
+
+  // If there is no accessible name set, use the displayed text, but ensure the
+  // '|' delimiters are removed.

[dmazzoni, 2016/07/13 23:41:35]

What code removes the | delimiters?

[Matt Giuca, 2016/07/14 01:16:02]

SetAccessibleName, just above, does this.

I agree this is a bit confusing and the comment probably should be split up
across the two functions.

Here:
// If there is no accessible name set, use the displayed text.

And above in SetAccessibleName:
// Ensure the '|' delimiters are removed.

[Patti Lor, 2016/07/19 01:31:51]

Have removed the side effect here, and left the comment because the code it
describes is now also in the same place. (See my other comment on patchset 6.)

+  if (accessible_name_.empty())
+    SetAccessibleName(instruction_view_->text());
+  state->name = accessible_name_;
+}
+
+void SubtleNotificationView::OnWidgetClosing(views::Widget* widget) {
+  if (widget && widget == GetWidget())
+    widget->RemoveObserver(this);
+}
+
+void SubtleNotificationView::OnWidgetVisibilityChanged(views::Widget* widget,
+                                                       bool visible) {
+  if (visible)
+    NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true);
+}