Accessibility for Autofill Popup View in native code

chrome/browser/ui/views/autofill/autofill_popup_view_views.cc

 // Copyright (c) 2012 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/autofill/autofill_popup_view_views.h"
 
 #include "chrome/browser/ui/autofill/autofill_popup_controller.h"
 #include "chrome/browser/ui/autofill/autofill_popup_layout_model.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/browser/suggestion.h"
+#include "ui/accessibility/ax_node_data.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/views/border.h"
+#include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
 namespace autofill {
 
+namespace {
+
+// Child view only for triggering accessibility events. Rendering is handled
+// by |AutofillPopupViewViews|.
+class AutofillPopupChildView : public views::View {
+ public:
+  // |controller| should not be NULL.
+  AutofillPopupChildView(AutofillPopupController* controller, size_t index)
+      : controller_(controller), index_(index) {
+    SetFocusBehavior(FocusBehavior::ALWAYS);
+  }
+
+ private:
+  ~AutofillPopupChildView() override {}
+
+  // views::Views implementation
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    // We do not special case |POPUP_ITEM_ID_SEPARATOR| suggestion because we
+    // skip that suggestion when the user navigates through the popup
+    // suggestions. See | AutofillPopupControllerImpl::CanAccept|.
+    node_data->role = ui::AX_ROLE_MENU_ITEM;
+    node_data->SetName(controller_->GetElidedValueAt(index_));
+  }
+
+  AutofillPopupController* controller_;  // Weak reference.
+  const size_t index_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutofillPopupChildView);
+};
+
+}  // namespace
+
 AutofillPopupViewViews::AutofillPopupViewViews(
-    AutofillPopupController* controller,
-    views::Widget* parent_widget)
+    AutofillPopupController* controller, views::Widget* parent_widget)
     : AutofillPopupBaseView(controller, parent_widget),
-      controller_(controller) {}
+      controller_(controller) {
+  for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
+    AutofillPopupChildView* child = new AutofillPopupChildView(controller, i);
+    AddChildViewAt(child, static_cast<int>(i));

[sky, 2017/02/27 21:05:57]

optional: AddChildViewAt(child); works the same here.

[csashi, 2017/02/27 21:32:38]

Done.

+  }
+  SetFocusBehavior(FocusBehavior::ALWAYS);
+}
 
 AutofillPopupViewViews::~AutofillPopupViewViews() {}
 
 void AutofillPopupViewViews::Show() {
   DoShow();
+  NotifyAccessibilityEvent(ui::AX_EVENT_MENU_START, true);
 }
 
 void AutofillPopupViewViews::Hide() {

[sky, 2017/02/27 21:05:57]

As DoHide is public, might that be called instead of this? Also, what about the
case of the widget being destroyed but hide not called?

[csashi, 2017/02/27 21:32:37]

If I am reading correctly, DoHide is protected - I guess DoHide was meant as a
helper function.

If the widget gets destroyed but Hide not called, worst case we don't get an
accessibility event. I could do something like the following in the destructor
but I have to look into whether that would be safe (i.e. if
NotifyAccessibilityEvent can access memory that has been destroyed).

In the destructor:

if (controller_) { // |Hide| was not called. 
  NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true);
}

[sky, 2017/02/27 23:31:04]

I'm not sure either. Maybe Dominic has an opinion here?

[sky, 2017/02/28 04:03:32]

Did Dominic respond to this?

[dmazzoni, 2017/02/28 16:53:18]

We should be cautious about calling NotifyAccessibilityEvent from a
destructor. It will definitely trigger calling back into the View to
query things like its location, visibility, etc.

It's not necessary to notify MENU_END in exceptional
circumstances like if the whole owning window is
destroyed or during app shutdown.

So as long as the normal code path (where the user closes the
pop-up menu and returns to the same web page) calls Hide()
and fires the event, I wouldn't worry about the exceptional
cases.

   // The controller is no longer valid after it hides us.
   controller_ = NULL;
-
   DoHide();
+  NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true);
 }
 
 void AutofillPopupViewViews::UpdateBoundsAndRedrawPopup() {
   DoUpdateBoundsAndRedrawPopup();
 }
 
 void AutofillPopupViewViews::OnPaint(gfx::Canvas* canvas) {
   if (!controller_)
     return;
 
@@ skipping 13 matching lines @@
     } else {
       DrawAutofillEntry(canvas, i, line_rect);
     }
   }
 }
 
 void AutofillPopupViewViews::InvalidateRow(size_t row) {
   SchedulePaintInRect(controller_->layout_model().GetRowBounds(row));
 }
 
+void AutofillPopupViewViews::NotifyAccessibilityEventForRow(
+    ui::AXEvent event_type, size_t row) {
+  AutofillPopupChildView *child_view =
+      static_cast<AutofillPopupChildView*>(child_at(row));

[sky, 2017/02/27 21:05:57]

This code makes me nervous. How do you know some other views haven't been added
so that the cast is still valid? One option is to check GetClassName().

[csashi, 2017/02/27 21:32:38]

This class hierarchy is currently not deep. Hence, added a debug check. Please
let me know if you wanted a check in opt mode also.

[sky, 2017/02/27 23:31:04]

My concern is the static_cast, but I'm ok with it.

[csashi, 2017/02/28 00:03:06]

Acknowledged.

+  child_view->NotifyAccessibilityEvent(event_type, true);
+}
+
 /**
 * Autofill entries in ltr.
 *
 * ............................................................................
 * . ICON | HTTP WARNING MESSAGE VALUE                                | LABEL .
 * ............................................................................
 * . OTHER AUTOFILL ENTRY VALUE |                                LABEL | ICON .
 * ............................................................................
 *
 * Autofill entries in rtl.
@@ skipping 99 matching lines @@
         controller_->GetElidedLabelAt(index),
         controller_->layout_model().GetLabelFontListForRow(index),
         GetNativeTheme()->GetSystemColor(
             ui::NativeTheme::kColorId_ResultsTableNormalDimmedText),
         gfx::Rect(label_x_align_left, entry_rect.y(), label_width,
                   entry_rect.height()),
         text_align);
   }
 }
 
+void AutofillPopupViewViews::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  node_data->role = ui::AX_ROLE_MENU;
+  node_data->SetName(l10n_util::GetStringUTF16(
+      IDS_AUTOFILL_POPUP_ACCESSIBLE_NODE_DATA));
+}
+
 AutofillPopupView* AutofillPopupView::Create(
     AutofillPopupController* controller) {
   views::Widget* observing_widget =
       views::Widget::GetTopLevelWidgetForNativeView(
           controller->container_view());
 
   // If the top level widget can't be found, cancel the popup since we can't
   // fully set it up.
   if (!observing_widget)
     return NULL;
 
   return new AutofillPopupViewViews(controller, observing_widget);
 }
 
 }  // namespace autofill