Uses child views in Autofill Popup so we can trigger

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

Index: chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
index 89925cee23efa839e82e4105a6ff2f6b6bc68806..019e91d06591ca08572e7e24ec406adf1f340199 100644
--- a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
+++ b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
@@ -6,8 +6,11 @@
 
 #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"
@@ -16,30 +19,87 @@
 #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:
+  // Internal class name.
+  static const char kViewClassName[];
+
+  // |controller| should not be NULL.
+  AutofillPopupChildView(AutofillPopupController* controller, size_t index)
+      : controller_(controller), index_(index) {
+    SetFocusBehavior(FocusBehavior::ALWAYS);
+  }
+
+ private:
+  ~AutofillPopupChildView() override {}
+
+  // views::Views implementation
+  const char* GetClassName() const override { return kViewClassName; }
+
+  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);
+};
+
+// static
+const char AutofillPopupChildView::kViewClassName[] = "AutofillPopupChildView";
+
+}  // namespace
+
 AutofillPopupViewViews::AutofillPopupViewViews(
     AutofillPopupController* controller,
     views::Widget* parent_widget)
     : AutofillPopupBaseView(controller, parent_widget),
-      controller_(controller) {}
+      controller_(controller) {
+  CreateChildViews();
+  SetFocusBehavior(FocusBehavior::ALWAYS);
+}
 
 AutofillPopupViewViews::~AutofillPopupViewViews() {}
 
 void AutofillPopupViewViews::Show() {
   DoShow();
+  NotifyAccessibilityEvent(ui::AX_EVENT_MENU_START, true);
 }
 
 void AutofillPopupViewViews::Hide() {
   // The controller is no longer valid after it hides us.
   controller_ = NULL;
-
   DoHide();
+  NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true);
 }
 
 void AutofillPopupViewViews::UpdateBoundsAndRedrawPopup() {
+  // We recreate the child views so we can be sure the |controller_|'s
+  // |GetLineCount()| will match the number of child views. Otherwise,
+  // the number of suggestions i.e. |GetLineCount()| may not match 1x1 with the
+  // child views. See crbug.com/697466.
+  // Currently, child views get the data from 'controller_'. If the child views
+  // save additional state corresponding to the suggestion, we must recreate the
+  // child views even when |GetLineCount()| == |child_count()| because the
+  // suggestion content may change.

[Evan Stade, 2017/03/06 21:45:33]

this comment makes me even more skeptical you want a hierarchy of views here.
The views amount to a container for an index. You could just make
GetAccessibleNodeData for |this| smarter (i.e. it knows it's handling a row
selection event) and call NotifyAccessibilityEvent on |this|. Or, you could
create just one child view to represent the selected row and update its index as
needed.

I would also think that since these child views don't have any visible bounds,
they aren't playing nicely with the screen reader which shows a very big and
obvious orange highlight on the thing that's selected (row in a menu, link with
keyboard focus, etc.). See the |location| property of AxNodeData.

[csashi, 2017/03/07 01:53:53]

The child views are limited to accessibility events for now in order to limit
the scope of this change, but we want to move more of the rendering to the child
views. 

If we still want to eliminate child views, we have to make an assumption that
GetAccessibleNodeData corresponds 1-1 with a row selection event. For e.g., we
will return the incorrect AXNodeData for the following sequence of calls.

Select row 1
Select row 2
GetAccessibleNodeData for first selection.

Perhaps this sequence can never happen now, but I was not sure about it. FWIW,
Dominic suggested firing the event on the specific selection rather than the
containing view (see
https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views...).

Reg. the visible bounds - yes, the orange box is not highlighting correctly but
I am not sure if a limitation of Chrome VOX (which we hope to fix) may also be
contributing to that - the orange box leaves the popup and goes to the HTML
elements (see https://codereview.chromium.org/2670003002/#msg20).  In any event,
speaking the selections seemed like an improvement, even if orange box may not
be correct.

[Evan Stade, 2017/03/08 18:03:21]

this is all good info, I'm just trying to judge this change on its own merits.
It's hard to imagine what the code will look like in the future world where the
views are being used for rendering as well --- what needs to go into the child
views? Will NotifyAccessibilityEventForRow be needed at all? It seems like a lot
of the changes you're making now might be undone soon. Since we've never had
good a11y for this menu it doesn't seem urgent to get it in now before we've
solved the problematic controller/view split.

tl;dr I appreciate the desire to limit the scope of a change but this seems to
be tackling the problems we have in the wrong order.

[csashi, 2017/03/08 20:40:41]

The intent is to move the code from OnPaint in the parent view to the child
view. I have not looked at this in detail because the immediate need was fixing
accessibility but I don't think the code for accessibility will change when we
move the rendering. 

Having said this, I am happy to remove the child views if we can be sure that
there is a 1-1 correspondence between |NotifyAccessibilityEvent| and
|GetAXNodeData|. I did not want to assume that.

I have removed NotifyAccessibilityEventForRow from this class and other wrapper
functions. It wasn't needed. But I had to one more bool parameter to
|InvalidateRow|. I think that is a reasonable parameter to add even without the
accessibility change - for e.g. we way want to render the selected row
differently.

+  if (controller_->GetLineCount() != static_cast<size_t>(child_count())) {
+    RemoveAllChildViews(true /* delete_children */);
+    CreateChildViews();
+  }
   DoUpdateBoundsAndRedrawPopup();
 }
 
@@ -51,6 +111,7 @@ void AutofillPopupViewViews::OnPaint(gfx::Canvas* canvas) {
       ui::NativeTheme::kColorId_ResultsTableNormalBackground));
   OnPaintBorder(canvas);
 
+  CHECK_EQ(controller_->GetLineCount(), static_cast<size_t>(child_count()));
   for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
     gfx::Rect line_rect = controller_->layout_model().GetRowBounds(i);
 
@@ -70,6 +131,18 @@ void AutofillPopupViewViews::InvalidateRow(size_t row) {
   SchedulePaintInRect(controller_->layout_model().GetRowBounds(row));
 }
 
+void AutofillPopupViewViews::NotifyAccessibilityEventForRow(
+    ui::AXEvent event_type,
+    size_t row) {
+  CHECK_LT(row, static_cast<size_t>(child_count()));
+  views::View* child = child_at(row);
+  CHECK(child);
+  CHECK_EQ(child->GetClassName(), AutofillPopupChildView::kViewClassName);
+  AutofillPopupChildView* child_view =
+      static_cast<AutofillPopupChildView*>(child);
+  child_view->NotifyAccessibilityEvent(event_type, true);
+}
+
 /**
 * Autofill entries in ltr.
 *
@@ -189,6 +262,19 @@ void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas,
   }
 }
 
+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));
+}
+
+void AutofillPopupViewViews::CreateChildViews() {
+  for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
+    AutofillPopupChildView* child = new AutofillPopupChildView(controller_, i);
+    AddChildView(child);
+  }
+}
+
 AutofillPopupView* AutofillPopupView::Create(
     AutofillPopupController* controller) {
   views::Widget* observing_widget =