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..d7d52e4cedddd214b876c60f64e545454dc4d1ed 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 char kKeyNameDelimiter[] = "|";

[tapted, 2016/07/11 03:46:47]

hum - this should probably be a const char16 []. Then you shouldn't need to wrap
it in base::ASCIIToUTF16(..) -- base::StringPiece16 will implicitly convert from
a char16[]. Try a string literal, but to initialize it you might need something
like

const char16 kKeyNameDelimiter[] = { '|', 0 };

[Patti Lor, 2016/07/12 00:04:36]

Done.

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

Hmm, I don't really like this (I wrote a whole comment asking to change it then
realised tapted had already asked for it).

At first I read this as "here is an array of characters that we will split on:
{'|', '\0'}", and thought, "why the heck are we splitting on NUL?"

I think readability trumps performance here: I'd prefer going back to a char
array string literal.

Alternatively you can store it as a wchar_t array and use WideToUTF16 which will
have no conversion overhead on Windows (but will on Mac and Linux). I don't
really see the point of doing that.

[tapted, 2016/07/14 01:48:40]

my brain sees arrays and string literals as interchangeable :p

I guess what we really need is this:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/gcoUbcjfsII/_Fndx...

Then this could be something like

const char16 kKeyNameDelimiter[] = u"|";

but that's currently on the banned list - https://chromium-cpp.appspot.com/ 
(although web_input_event_builders_mac_unittest.mm uses it already *gasp*)

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

Have taken Matt's suggestion to keep it as a char array string literal for
readability, but yeah the banned one seems to be the best solution :(

+
 }  // namespace
 
 // Class containing the instruction text. Contains fancy styling on the keyboard
@@ -42,13 +48,17 @@ class SubtleNotificationView::InstructionView : public views::View {
  public:
   // Creates an InstructionView with specific text. |text| may contain one or
   // more segments delimited by a pair of pipes ('|'); each of these segments
-  // will be displayed as a keyboard key. e.g., "Press |Alt|+|Q| to exit" will
+  // will be displayed as a keyboard key. e.g. "Press |Alt|+|Q| to exit" will

[tapted, 2016/07/11 03:46:47]

(ubernit: technically, a comma should "always" come after "For example," even
if, for example, it's abbreviated. For example, like this.)

[Patti Lor, 2016/07/12 00:04:36]

Oops, didn't notice that! Noted for future reference and done :)

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

Also I had a comma there in the first place :)

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

Haha, yup, that's what I meant D: Didn't copy and paste properly!

   // have "Alt" and "Q" rendered as keys.
   InstructionView(const base::string16& text,
                   const gfx::FontList& font_list,
                   SkColor foreground_color,
                   SkColor background_color);
 
+  base::string16 accessible_name() { return accessible_name_; }

[tapted, 2016/07/11 03:46:47]

nit: const method

[Patti Lor, 2016/07/12 00:04:36]

Done.

+  void SetAccessibleName(const base::string16& accessible_name);
+
+  base::string16 text() { return text_; }

[tapted, 2016/07/11 03:46:47]

nit: const

[Patti Lor, 2016/07/12 00:04:36]

Done.

   void SetText(const base::string16& text);
 
  private:
@@ -61,6 +71,10 @@ class SubtleNotificationView::InstructionView : public views::View {
   SkColor foreground_color_;
   SkColor background_color_;
 
+  // Text to read out on displaying this notification for screenreaders, with
+  // unicode characters expanded.
+  base::string16 accessible_name_;

[tapted, 2016/07/11 03:46:47]

If it achieves the same thing, I think it makes more sense just to store this on
SubtleNotificationView instead of the InstructionView, since it's
SubtleNotificationView that's populating AXViewState

[Patti Lor, 2016/07/12 00:04:36]

Done.

+
   base::string16 text_;
 
   DISALLOW_COPY_AND_ASSIGN(InstructionView);
@@ -82,6 +96,12 @@ SubtleNotificationView::InstructionView::InstructionView(
   SetText(text);
 }
 
+void SubtleNotificationView::InstructionView::SetAccessibleName(
+    const base::string16& accessible_name) {
+  base::RemoveChars(accessible_name, base::ASCIIToUTF16(kKeyNameDelimiter),
+                    &accessible_name_);
+}
+
 void SubtleNotificationView::InstructionView::SetText(
     const base::string16& text) {
   // Avoid replacing the contents with the same text.
@@ -92,8 +112,8 @@ void SubtleNotificationView::InstructionView::SetText(
 
   // Parse |text|, looking for pipe-delimited segment.
   std::vector<base::string16> segments =
-      base::SplitString(text, base::ASCIIToUTF16("|"), base::TRIM_WHITESPACE,
-                        base::SPLIT_WANT_ALL);
+      base::SplitString(text, base::ASCIIToUTF16(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 +202,9 @@ void SubtleNotificationView::UpdateContent(
   instruction_view_->SetVisible(!instruction_text.empty());
   link_->SetText(link_text);
   link_->SetVisible(!link_text.empty());
+
+  NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true);
+  NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true);
 }
 
 // static
@@ -206,5 +229,33 @@ 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) {
+  instruction_view_->SetAccessibleName(accessible_name);
+}
+
+void SubtleNotificationView::GetAccessibleState(ui::AXViewState* state) {
+  state->role = ui::AX_ROLE_LABEL_TEXT;
+
+  // If there is no accessible name set, fall back to the text displayed.
+  if (instruction_view_->accessible_name().empty())

[tapted, 2016/07/11 03:46:47]

Can this happen?

[Patti Lor, 2016/07/12 00:04:36]

Yes, currently the new backspace shortcut bubble is the only one that explicitly
sets the accessible name (in NewBackShortcutBubble::UpdateContent). But since
the exclusive access bubbles don't require a different string or anything, they
will fall back to this.

[tapted, 2016/07/12 00:51:39]

Ah. So, it's "generally" good to avoid side-effects in getters. Some would
consider it a bug that View::GetAccessibleState isn't a const method (and if it
was, you wouldn't be able to set the accessible name here).

I think this use is OK, but it needs a better comment - it's not clear why we
set it instead of just assigning it. Something like

// If there is no accessible name set, use the displayed text, but ensure the
'|' delimiters are removed.

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

This is a good point. How about we *pretend* that GetAccessibleState is const
and design accordingly?

- Make |accessible_name_| mutable, which implies the correct semantics (it is a
cached value that can be written by a const method).
- Rename SetAccessibleName to GetAccessibleName and make it const. Move this if
statement into GetAccessibleName (so GAN updates |accessible_name_| only if it's
empty, and returns the value of |accessible_name_|.
- Have GetAccessibleState just do state->name = GetAccessibleName().

Now GetAccessibleState has no visible side-effects, so behaves as a const
method.

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

A point Matt brought up in one of his other comments made me remove the side
effect entirely and just calculate the |accessible_name_| on the fly when it's
not set manually. Since |accessible_name_| can get stale, it means it will need
to be cleared on calls to SetUpdate(). But this also means manually set
|accessible_name_| will get cleared as well, which might be bad because the
caller would have to call methods in a certain order (they would have to call
UpdateContent() before calling SetAccessibleName() otherwise the
|accessible_name_| they'd just set would be cleared by UpdateContent()).

So either you would have to clear |accessible_name_| only if it had been set
inside GetAccessibleState(), or just don't make the fallback text permanent (the
solution I have chosen).

Thanks for the suggestion though Matt! It was a good idea :)

+    SetAccessibleName(instruction_view_->text());
+  state->name = instruction_view_->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);
+}