Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making...

Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog.
Also, since the ContentsView doesn't include the title and dialog
buttons, it creates a nested group that excludes the dialog buttons and
is hard to navigate with a11y tools since it hides the dialog contents
inside the group.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285

Review-Url: https://codereview.chromium.org/2980713002
Cr-Commit-Position: refs/heads/master@{#486153}

chrome/browser/ui/views/conflicting_module_view_win.cc

@@ -106,6 +106,10 @@ void ConflictingModuleView::ShowBubble() {
   bubble_shown.SetValue(bubble_shown.GetValue() + 1);
 }
 
+ui::AXRole ConflictingModuleView::GetAccessibleWindowRole() const {
+  return ui::AX_ROLE_ALERT_DIALOG;
+}
+
 void ConflictingModuleView::OnWidgetClosing(views::Widget* widget) {
   views::BubbleDialogDelegateView::OnWidgetClosing(widget);
   base::RecordAction(
@@ -156,10 +160,6 @@ void ConflictingModuleView::Init() {
       EnumerateModulesModel::ACTION_BOUNDARY);
 }
 
-void ConflictingModuleView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  node_data->role = ui::AX_ROLE_ALERT_DIALOG;
-}
-
 void ConflictingModuleView::OnConflictsAcknowledged() {
   EnumerateModulesModel* model = EnumerateModulesModel::GetInstance();
   if (!model->ShouldShowConflictWarning())

chrome/browser/ui/views/conflicting_module_view_win.h

@@ -32,15 +32,13 @@ class ConflictingModuleView : public views::BubbleDialogDelegateView,
   // Shows the bubble and updates the counter for how often it has been shown.
   void ShowBubble();
 
-  // views::BubbleDialogDelegateView implementation:
+  // views::BubbleDialogDelegateView:
+  ui::AXRole GetAccessibleWindowRole() const override;
   void OnWidgetClosing(views::Widget* widget) override;
   bool Accept() override;
   base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
   void Init() override;
 
-  // views::View implementation.
-  void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
-
   // EnumerateModulesModel::Observer:
   void OnConflictsAcknowledged() override;
 

chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc

@@ -132,7 +132,7 @@ void ExtensionMessageBubbleViewBrowserTest::ClickLearnMoreButton(
   // platform events may happen before the close completes and the dialog needs
   // to report a valid state.
   ui::AXNodeData node_data;
-  bubble->GetAccessibleNodeData(&node_data);
+  bubble->GetWidget()->GetRootView()->GetAccessibleNodeData(&node_data);
   EXPECT_EQ(ui::AX_ROLE_DIALOG, node_data.role);
 }
 

chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc

@@ -58,11 +58,11 @@ class PermissionsBubbleDialogDelegateView
   void CloseBubble();
 
   // BubbleDialogDelegateView:
+  ui::AXRole GetAccessibleWindowRole() const override;
   bool ShouldShowCloseButton() const override;
   base::string16 GetWindowTitle() const override;
   void AddedToWidget() override;
   void OnWidgetDestroying(views::Widget* widget) override;
-  void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
   bool Cancel() override;
   bool Accept() override;
   bool Close() override;
@@ -147,6 +147,11 @@ void PermissionsBubbleDialogDelegateView::CloseBubble() {
   GetWidget()->Close();
 }
 
+ui::AXRole PermissionsBubbleDialogDelegateView::GetAccessibleWindowRole()
+    const {
+  return ui::AX_ROLE_ALERT_DIALOG;
+}
+
 bool PermissionsBubbleDialogDelegateView::ShouldShowCloseButton() const {
   return true;
 }
@@ -177,12 +182,6 @@ void PermissionsBubbleDialogDelegateView::OnWidgetDestroying(
   }
 }
 
-void PermissionsBubbleDialogDelegateView::GetAccessibleNodeData(
-    ui::AXNodeData* node_data) {
-  views::BubbleDialogDelegateView::GetAccessibleNodeData(node_data);
-  node_data->role = ui::AX_ROLE_ALERT_DIALOG;
-}
-
 int PermissionsBubbleDialogDelegateView::GetDefaultDialogButton() const {
   // To prevent permissions being accepted accidentally, and as a security
   // measure against crbug.com/619429, permission prompts should not be accepted

ui/views/bubble/bubble_dialog_delegate.cc

@@ -310,10 +310,8 @@ void BubbleDialogDelegateView::HandleVisibilityChanged(Widget* widget,
   // than just its title and initially focused view.  See
   // http://crbug.com/474622 for details.
   if (widget == GetWidget() && visible) {
-    ui::AXNodeData node_data;
-    GetAccessibleNodeData(&node_data);
-    if (node_data.role == ui::AX_ROLE_ALERT_DIALOG)
-      NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true);
+    if (GetAccessibleWindowRole() == ui::AX_ROLE_ALERT_DIALOG)
+      widget->GetRootView()->NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true);
   }
 }
 

ui/views/window/dialog_delegate.cc

@@ -257,11 +257,6 @@ View* DialogDelegateView::GetContentsView() {
   return this;
 }
 
-void DialogDelegateView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  node_data->SetName(GetWindowTitle());
-  node_data->role = ui::AX_ROLE_DIALOG;
-}
-
 void DialogDelegateView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   if (details.is_add && details.child == this && GetWidget())

ui/views/window/dialog_delegate.h

@@ -143,7 +143,6 @@ class VIEWS_EXPORT DialogDelegateView : public DialogDelegate,
   View* GetContentsView() override;
 
   // Overridden from View:
-  void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
   void ViewHierarchyChanged(
       const ViewHierarchyChangedDetails& details) override;