a11y/Mac: Add screenreader support for SubtleNotificationView announcements.

chrome/browser/ui/views/subtle_notification_view.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/subtle_notification_view.h"
 
 #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"
 #include "ui/views/controls/label.h"
 #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 {
 
 // Space between the site info label and the link.
 const int kMiddlePaddingPx = 30;
 
 const int kOuterPaddingHorizPx = 40;
 const int kOuterPaddingVertPx = 8;
 
 // Partially-transparent background color.
 const SkColor kBackgroundColor = SkColorSetARGB(0xcc, 0x28, 0x2c, 0x32);
 
 // Spacing around the key name.
 const int kKeyNameMarginHorizPx = 7;
 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
 // key (not just a simple label).
 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:
   // Adds a label to the end of the notification text. If |format_as_key|,
   // surrounds the label in a rounded-rect border to indicate that it is a
   // keyboard key.
   void AddTextSegment(const base::string16& text, bool format_as_key);
 
   const gfx::FontList& font_list_;
   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);
 };
 
 SubtleNotificationView::InstructionView::InstructionView(
     const base::string16& text,
     const gfx::FontList& font_list,
     SkColor foreground_color,
     SkColor background_color)
     : font_list_(font_list),
       foreground_color_(foreground_color),
       background_color_(background_color) {
   // The |between_child_spacing| is the horizontal margin of the key name.
   views::BoxLayout* layout = new views::BoxLayout(views::BoxLayout::kHorizontal,
                                                   0, 0, kKeyNameMarginHorizPx);
   SetLayoutManager(layout);
 
   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.
   if (text == text_)
     return;
 
   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);
+      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
   // list is also empty (rather than containing a single empty string).
   DCHECK(segments.empty() || segments.size() % 2 == 1);
 
   // Add text segment, alternating between non-key (no border) and key (border)
   // formatting.
   bool format_as_key = false;
   for (const auto& segment : segments) {
@@ skipping 68 matching lines @@
 
 SubtleNotificationView::~SubtleNotificationView() {}
 
 void SubtleNotificationView::UpdateContent(
     const base::string16& instruction_text,
     const base::string16& link_text) {
   instruction_view_->SetText(instruction_text);
   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
 views::Widget* SubtleNotificationView::CreatePopupWidget(
     gfx::NativeView parent_view,
     SubtleNotificationView* view,
     bool accept_events) {
   // Initialize the popup.
   views::Widget* popup = new views::Widget;
   views::Widget::InitParams params(views::Widget::InitParams::TYPE_POPUP);
   params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
   params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
   params.parent = parent_view;
   params.accept_events = accept_events;
   popup->Init(params);
   popup->SetContentsView(view);
   // We set layout manager to nullptr to prevent the widget from sizing its
   // contents to the same size as itself. This prevents the widget contents from
   // shrinking while we animate the height of the popup to give the impression
   // that it is sliding off the top of the screen.
   // TODO(mgiuca): This probably isn't necessary now that there is no slide
   // 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);
+}