Enforce valid role prior to calling AXNodeData::SetName

Callers are expected to set a valid role prior to calling SetName. We
check for Role::kNone (the role was explicitly changed to the
presentational role) but not Role::kUnknown (the role was never set /
AKA the default).

This commit adds Role::kUnknown to the existing DCHECK and sets
roles on several Views which were missing roles.

AX-Relnotes: n/a

Bug: 1348081
Change-Id: I7511fbdfdc78944978641d19e94f368dc6d72fcc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3816142
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Joanmarie Diggs <jdiggs@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1045987}

diff --git a/ash/app_list/views/assistant/assistant_page_view.cc b/ash/app_list/views/assistant/assistant_page_view.cc
index 9518aeb..6881953 100644
--- a/ash/app_list/views/assistant/assistant_page_view.cc
+++ b/ash/app_list/views/assistant/assistant_page_view.cc

@@ -196,6 +196,9 @@
 
 void AssistantPageView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   View::GetAccessibleNodeData(node_data);
+
+  // A valid role must be set prior to setting the name.
+  node_data->role = ax::mojom::Role::kPane;
   node_data->SetName(l10n_util::GetStringUTF16(IDS_ASH_ASSISTANT_WINDOW));
 }
 

diff --git a/ash/app_list/views/search_result_list_view.cc b/ash/app_list/views/search_result_list_view.cc
index 84be004..9986e75 100644
--- a/ash/app_list/views/search_result_list_view.cc
+++ b/ash/app_list/views/search_result_list_view.cc

@@ -231,6 +231,8 @@
       break;
   }
 
+  // A valid role must be set prior to setting the name.
+  GetViewAccessibility().OverrideRole(ax::mojom::Role::kListBox);
   GetViewAccessibility().OverrideName(l10n_util::GetStringFUTF16(
       IDS_ASH_SEARCH_RESULT_CATEGORY_LABEL_ACCESSIBLE_NAME,
       title_label_->GetText()));

diff --git a/ash/clipboard/views/clipboard_history_item_view.cc b/ash/clipboard/views/clipboard_history_item_view.cc
index a6dba46..f51ae3d 100644
--- a/ash/clipboard/views/clipboard_history_item_view.cc
+++ b/ash/clipboard/views/clipboard_history_item_view.cc

@@ -160,8 +160,6 @@
 
 void ClipboardHistoryItemView::Init() {
   SetFocusBehavior(views::View::FocusBehavior::ACCESSIBLE_ONLY);
-  GetViewAccessibility().OverrideRole(ax::mojom::Role::kMenuItem);
-
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
   // Ensures that MainButton is below any other child views.
@@ -269,6 +267,9 @@
 }
 
 void ClipboardHistoryItemView::GetAccessibleNodeData(ui::AXNodeData* data) {
+  // A valid role must be set in the AXNodeData prior to setting the name
+  // via AXNodeData::SetName.
+  data->role = ax::mojom::Role::kMenuItem;
   data->SetName(GetAccessibleName());
 }
 

diff --git a/ash/login/ui/media_controls_header_view.cc b/ash/login/ui/media_controls_header_view.cc
index 7cb96264..c26c23b 100644
--- a/ash/login/ui/media_controls_header_view.cc
+++ b/ash/login/ui/media_controls_header_view.cc

@@ -8,6 +8,7 @@
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/style/ash_color_provider.h"
 #include "components/vector_icons/vector_icons.h"
+#include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_utils.h"
@@ -117,6 +118,8 @@
 }
 
 void MediaControlsHeaderView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  // A valid role must be set prior to setting the name.
+  node_data->role = ax::mojom::Role::kPane;
   node_data->SetName(app_name_view_->GetText());
 }
 

diff --git a/ash/shortcut_viewer/views/keyboard_shortcut_item_view.cc b/ash/shortcut_viewer/views/keyboard_shortcut_item_view.cc
index e4f1797..5deb212b 100644
--- a/ash/shortcut_viewer/views/keyboard_shortcut_item_view.cc
+++ b/ash/shortcut_viewer/views/keyboard_shortcut_item_view.cc

@@ -233,8 +233,8 @@
       gfx::Insets::TLBR(kVerticalPadding, 0, kVerticalPadding, 0)));
 
   // Use leaf list item role so that name is spoken by screen reader, but
-  // redundant child label text is not also spoken.
-  GetViewAccessibility().OverrideRole(ax::mojom::Role::kListItem);
+  // redundant child label text is not also spoken. (The role is set in
+  // GetAccessibleNodeData.)
   GetViewAccessibility().OverrideIsLeaf(true);
   accessible_name_ =
       description_label_view_->GetText() + u", " + accessible_string;
@@ -242,6 +242,8 @@
 
 void KeyboardShortcutItemView::GetAccessibleNodeData(
     ui::AXNodeData* node_data) {
+  // A valid role must be set prior to setting the name.
+  node_data->role = ax::mojom::Role::kListItem;
   node_data->SetName(accessible_name_);
 }
 

diff --git a/ash/system/network/network_tray_view.cc b/ash/system/network/network_tray_view.cc
index c8ea6c2..2502c7a 100644
--- a/ash/system/network/network_tray_view.cc
+++ b/ash/system/network/network_tray_view.cc

@@ -52,6 +52,8 @@
 }
 
 void NetworkTrayView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  // A valid role must be set prior to setting the name.
+  node_data->role = ax::mojom::Role::kImage;
   node_data->SetName(accessible_name_);
   node_data->SetDescription(accessible_description_);
 }

diff --git a/ash/system/power/tray_power.cc b/ash/system/power/tray_power.cc
index d70caf7..f087eaf 100644
--- a/ash/system/power/tray_power.cc
+++ b/ash/system/power/tray_power.cc

@@ -22,6 +22,7 @@
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
+#include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/chromeos/devicetype_utils.h"
@@ -60,6 +61,8 @@
 }
 
 void PowerTrayView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  // A valid role must be set prior to setting the name.
+  node_data->role = ax::mojom::Role::kImage;
   node_data->SetName(accessible_name_);
 }
 

diff --git a/chrome/browser/ash/accessibility/spoken_feedback_browsertest.cc b/chrome/browser/ash/accessibility/spoken_feedback_browsertest.cc
index 609b677f..b471d98 100644
--- a/chrome/browser/ash/accessibility/spoken_feedback_browsertest.cc
+++ b/chrome/browser/ash/accessibility/spoken_feedback_browsertest.cc

@@ -1026,8 +1026,9 @@
   widget->Init(std::move(params));
 
   views::View* view = new views::View();
-  view->GetViewAccessibility().OverrideName("hello");
+  // A valid role must be set prior to setting the name.
   view->GetViewAccessibility().OverrideRole(ax::mojom::Role::kButton);
+  view->GetViewAccessibility().OverrideName("hello");
   view->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
   widget->GetRootView()->AddChildView(view);
 
@@ -1103,8 +1104,9 @@
   widget->Init(std::move(params));
 
   views::View* view = new views::View();
-  view->GetViewAccessibility().OverrideName("hello");
+  // A valid role must be set prior to setting the name.
   view->GetViewAccessibility().OverrideRole(ax::mojom::Role::kButton);
+  view->GetViewAccessibility().OverrideName("hello");
   view->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
   widget->GetRootView()->AddChildView(view);
 

diff --git a/chrome/browser/ash/app_restore/arc_ghost_window_view.cc b/chrome/browser/ash/app_restore/arc_ghost_window_view.cc
index 35331439..28d3a81 100644
--- a/chrome/browser/ash/app_restore/arc_ghost_window_view.cc
+++ b/chrome/browser/ash/app_restore/arc_ghost_window_view.cc

@@ -44,6 +44,8 @@
   }
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    // A valid role must be set prior to setting the name.
+    node_data->role = ax::mojom::Role::kProgressIndicator;
     node_data->SetName(
         l10n_util::GetStringUTF16(IDS_ARC_GHOST_WINDOW_APP_LAUNCHING_THROBBER));
   }

diff --git a/chrome/browser/ash/arc/accessibility/drawer_layout_handler_unittest.cc b/chrome/browser/ash/arc/accessibility/drawer_layout_handler_unittest.cc
index ce4f4ffa..3b4969ab 100644
--- a/chrome/browser/ash/arc/accessibility/drawer_layout_handler_unittest.cc
+++ b/chrome/browser/ash/arc/accessibility/drawer_layout_handler_unittest.cc

@@ -154,10 +154,12 @@
   ASSERT_EQ(2, create_result.value().first);
 
   ui::AXNodeData data;
+  // A valid role must be set prior to calling SetName.
+  data.role = ax::mojom::Role::kMenu;
   data.SetName("node description");
   create_result.value().second->PostSerializeNode(&data);
 
-  // Modifier doesn't override the name by an empty string.
+  // Modifier doesn't override the name by an empty string, or change the role.
   ASSERT_EQ(ax::mojom::Role::kMenu, data.role);
   ASSERT_EQ("node description",
             data.GetStringAttribute(ax::mojom::StringAttribute::kName));

diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
index f89cfe6b..ba372a4 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc

@@ -134,6 +134,14 @@
 
 void BookmarkEditorView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   views::DialogDelegateView::GetAccessibleNodeData(node_data);
+
+  // TODO(crbug.com/1361263): Currently DialogDelegateView does not override
+  // GetAccessibleNodeData, thus the call above accomplishes nothing. We need
+  // this View to have a role before setting its name, but if we set it to
+  // dialog, we'll wind up with a dialog (this view) inside of a dialog
+  // (RootView). Note that both views also share the same accessible name.
+  // In the meantime, give it a generic role.
+  node_data->role = ax::mojom::Role::kPane;
   node_data->SetName(l10n_util::GetStringUTF8(IDS_BOOKMARK_EDITOR_TITLE));
 }
 

diff --git a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
index 03fdfde..97ad400 100644
--- a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
+++ b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc

@@ -517,6 +517,12 @@
       // instance anyway.
       alert_virtual_view_->GetCustomData().RemoveState(
           ax::mojom::State::kInvisible);
+
+      // A valid role must be set prior to setting the name.
+      // TODO(crbug.com/1361281): Consider using AnnounceText instead of a
+      // virtual view.
+      alert_virtual_view_->GetCustomData().role =
+          ax::mojom::Role::kGenericContainer;
       alert_virtual_view_->GetCustomData().SetName(label);
       alert_virtual_view_->NotifyAccessibilityEvent(ax::mojom::Event::kAlert);
     }

diff --git a/ui/accessibility/ax_node_data.cc b/ui/accessibility/ax_node_data.cc
index 41b61b4..2500c1c 100644
--- a/ui/accessibility/ax_node_data.cc
+++ b/ui/accessibility/ax_node_data.cc

@@ -620,10 +620,12 @@
 
 void AXNodeData::SetName(const std::string& name) {
   // Elements with role='presentation' have Role::kNone. They should not be
-  // named. This check is only relevant if the name is not empty.
+  // named. Objects with Role::kUnknown were never given a role. This check
+  // is only relevant if the name is not empty.
   // TODO(accessibility): It would be nice to have a means to set the name
   // and role at the same time to avoid this ordering requirement.
-  DCHECK(role != ax::mojom::Role::kNone || name.empty())
+  DCHECK(name.empty() ||
+         (role != ax::mojom::Role::kNone && role != ax::mojom::Role::kUnknown))
       << "Cannot set name to '" << name << "' on class: '"
       << GetStringAttribute(ax::mojom::StringAttribute::kClassName)
       << "' because a valid role is needed to set the default NameFrom "

diff --git a/ui/accessibility/ax_node_data_unittest.cc b/ui/accessibility/ax_node_data_unittest.cc
index 2092adb..0a8c0349 100644
--- a/ui/accessibility/ax_node_data_unittest.cc
+++ b/ui/accessibility/ax_node_data_unittest.cc

@@ -366,6 +366,11 @@
 
 TEST(AXNodeDataTest, SetName) {
   AXNodeData data;
+  // SetName should not be called on a role of kUnknown. That role means the
+  // role has not yet been set, and we need a role to identify the NameFrom on
+  // objects where it has not been set. This is enforced by a DCHECK.
+  EXPECT_DCHECK_DEATH(data.SetName("no role yet"));
+
   // SetName should not be called on a role of kNone. That role is used for
   // presentational objects which should not be included in the accessibility
   // tree. This is enforced by a DCHECK.