Set the accessible role prior to setting the accessible name

AXNodeData::SetName expects callers to have set a valid role prior to setting the name. The existing DCHECK checks for role kNone; but not kUnknown (which is the default). Ultimately that DCHECK will be modified to also check for role KUnknown. In the meantime, in order to minimize the size of commits, this change moves setting the role before setting the name for all views which set both. In addition, it reorders ax::mojom::Role so that it is more clear that the default role value is kUnknown; not kNone. AX-Relnotes: n/a Bug: 1348081 Change-Id: Ib7a07ab5016f0b7263acc65930142eafcf0efde4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3810374 Reviewed-by: David Dorwin <ddorwin@chromium.org> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Peter Boström <pbos@chromium.org> Reviewed-by: David Tseng <dtseng@chromium.org> Reviewed-by: Mitsuru Oshima <oshima@chromium.org> Commit-Queue: Joanmarie Diggs <jdiggs@igalia.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/main@{#1035115}
chromium · Aug 15, 2022 · b727d0b · b727d0b
1 parent 0432c8b
commit b727d0b
Show file tree

Hide file tree

Showing 38 changed files with 111 additions and 43 deletions.
diff --git a/ash/accelerators/exit_warning_handler.cc b/ash/accelerators/exit_warning_handler.cc
@@ -78,8 +78,8 @@ class ExitWarningWidgetDelegateView : public views::WidgetDelegateView {
   }
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->SetName(accessible_name_);
     node_data->role = ax::mojom::Role::kAlert;
+    node_data->SetName(accessible_name_);
   }
 
  private:

diff --git a/ash/capture_mode/capture_mode_camera_preview_view.cc b/ash/capture_mode/capture_mode_camera_preview_view.cc
@@ -338,9 +338,9 @@ void CameraPreviewView::Layout() {
 
 void CameraPreviewView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   views::View::GetAccessibleNodeData(node_data);
+  node_data->role = ax::mojom::Role::kVideo;
   node_data->SetName(
       l10n_util::GetStringUTF16(IDS_ASH_SCREEN_CAPTURE_CAMERA_PREVIEW_FOCUSED));
-  node_data->role = ax::mojom::Role::kVideo;
 }
 
 views::View* CameraPreviewView::GetView() {

diff --git a/ash/capture_mode/capture_mode_menu_group.cc b/ash/capture_mode/capture_mode_menu_group.cc
@@ -122,8 +122,8 @@ class CaptureModeMenuHeader
   // views::View:
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
     View::GetAccessibleNodeData(node_data);
-    node_data->SetName(GetHeaderLabel());
     node_data->role = ax::mojom::Role::kHeader;
+    node_data->SetName(GetHeaderLabel());
   }
 
   // CaptureModeSessionFocusCycler::HighlightableView:
@@ -249,8 +249,8 @@ class CaptureModeOption
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
     Button::GetAccessibleNodeData(node_data);
-    node_data->SetName(GetOptionLabel());
     node_data->role = ax::mojom::Role::kRadioButton;
+    node_data->SetName(GetOptionLabel());
     node_data->SetCheckedState(IsOptionChecked()
                                    ? ax::mojom::CheckedState::kTrue
                                    : ax::mojom::CheckedState::kFalse);

diff --git a/ash/capture_mode/capture_mode_toggle_button.cc b/ash/capture_mode/capture_mode_toggle_button.cc
@@ -44,8 +44,8 @@ void CaptureModeToggleButton::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   ImageButton::GetAccessibleNodeData(node_data);
   const std::u16string tooltip = GetTooltipText(gfx::Point());
   DCHECK(!tooltip.empty());
-  node_data->SetName(tooltip);
   node_data->role = ax::mojom::Role::kToggleButton;
+  node_data->SetName(tooltip);
   node_data->SetCheckedState(GetToggled() ? ax::mojom::CheckedState::kTrue
                                           : ax::mojom::CheckedState::kFalse);
 }

diff --git a/ash/login/ui/lock_contents_view.cc b/ash/login/ui/lock_contents_view.cc
@@ -904,11 +904,11 @@ void LockContentsView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   ShelfWidget* shelf_widget = shelf->shelf_widget();
   GetViewAccessibility().OverrideNextFocus(shelf_widget);
   GetViewAccessibility().OverridePreviousFocus(shelf->GetStatusAreaWidget());
+  node_data->role = ax::mojom::Role::kWindow;
   node_data->SetName(
       l10n_util::GetStringUTF16(screen_type_ == LockScreen::ScreenType::kLogin
                                     ? IDS_ASH_LOGIN_SCREEN_ACCESSIBLE_NAME
                                     : IDS_ASH_LOCK_SCREEN_ACCESSIBLE_NAME));
-  node_data->role = ax::mojom::Role::kWindow;
 }
 
 bool LockContentsView::AcceleratorPressed(const ui::Accelerator& accelerator) {

diff --git a/ash/login/ui/login_pin_view.cc b/ash/login/ui/login_pin_view.cc
@@ -177,8 +177,8 @@ class BasePinButton : public views::View {
   }
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->SetName(accessible_name_);
     node_data->role = ax::mojom::Role::kButton;
+    node_data->SetName(accessible_name_);
   }
 
  protected:

diff --git a/ash/login/ui/login_remove_account_dialog.cc b/ash/login/ui/login_remove_account_dialog.cc
@@ -279,6 +279,7 @@ const char* LoginRemoveAccountDialog::GetClassName() const {
 
 void LoginRemoveAccountDialog::GetAccessibleNodeData(
     ui::AXNodeData* node_data) {
+  node_data->role = ax::mojom::Role::kDialog;
   if (remove_user_button_) {
     node_data->SetName(l10n_util::GetStringUTF16(
         IDS_ASH_LOGIN_POD_REMOVE_ACCOUNT_ACCESSIBLE_NAME));
@@ -294,7 +295,6 @@ void LoginRemoveAccountDialog::GetAccessibleNodeData(
       node_data->SetDescription(email_label_->GetText());
     }
   }
-  node_data->role = ax::mojom::Role::kDialog;
   node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kModal, true);
 }
 

diff --git a/ash/system/holding_space/holding_space_item_view.cc b/ash/system/holding_space/holding_space_item_view.cc
@@ -134,6 +134,7 @@ HoldingSpaceItemView::HoldingSpaceItemView(HoldingSpaceViewDelegate* delegate,
   SetNotifyEnterExitOnChild(true);
 
   // Accessibility.
+  GetViewAccessibility().OverrideRole(ax::mojom::Role::kListItem);
   GetViewAccessibility().OverrideName(item->GetAccessibleName());
 
   // When the description is not specified, tooltip text will be used.
@@ -144,8 +145,6 @@ HoldingSpaceItemView::HoldingSpaceItemView(HoldingSpaceViewDelegate* delegate,
   GetViewAccessibility().OverrideDescription(
       std::u16string(), ax::mojom::DescriptionFrom::kAttributeExplicitlyEmpty);
 
-  GetViewAccessibility().OverrideRole(ax::mojom::Role::kListItem);
-
   // Layer.
   SetPaintToLayer();
   layer()->SetFillsBoundsOpaquely(false);

diff --git a/chrome/browser/ash/input_method/ui/suggestion_accessibility_label.cc b/chrome/browser/ash/input_method/ui/suggestion_accessibility_label.cc
@@ -17,8 +17,8 @@ SuggestionAccessibilityLabel::~SuggestionAccessibilityLabel() = default;
 
 void SuggestionAccessibilityLabel::GetAccessibleNodeData(
     ui::AXNodeData* node_data) {
-  node_data->SetName(GetAccessibleName());
   node_data->role = ax::mojom::Role::kImeCandidate;
+  node_data->SetName(GetAccessibleName());
   node_data->AddStringAttribute(
       ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
 }

diff --git a/chrome/browser/ash/login/ui/webui_login_view.cc b/chrome/browser/ash/login/ui/webui_login_view.cc
@@ -407,9 +407,9 @@ void WebUILoginView::OnLoginPromptVisible() {
 }
 
 void WebUILoginView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  node_data->role = ax::mojom::Role::kWindow;
   node_data->SetName(
       l10n_util::GetStringUTF16(IDS_OOBE_ACCESSIBLE_SCREEN_NAME));
-  node_data->role = ax::mojom::Role::kWindow;
 }
 
 BEGIN_METADATA(WebUILoginView, views::View)

diff --git a/chrome/browser/ash/notifications/idle_app_name_notification_view.cc b/chrome/browser/ash/notifications/idle_app_name_notification_view.cc
@@ -178,8 +178,8 @@ class IdleAppNameNotificationDelegateView
   }
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->SetName(spoken_text_);
     node_data->role = ax::mojom::Role::kAlert;
+    node_data->SetName(spoken_text_);
   }
 
   // ImplicitAnimationObserver overrides

diff --git a/chrome/browser/resources/chromeos/accessibility/common/automation_predicate.js b/chrome/browser/resources/chromeos/accessibility/common/automation_predicate.js
@@ -594,6 +594,7 @@ export class AutomationPredicate {
              Role.GROUP,
              Role.IMAGE,
              Role.PARAGRAPH,
+             Role.SCROLL_VIEW,
              Role.STATIC_TEXT,
              Role.SVG_ROOT,
              Role.TABLE_HEADER_CONTAINER,
@@ -919,6 +920,7 @@ AutomationPredicate.structuralContainer = AutomationPredicate.roles([
   Role.PLUGIN_OBJECT,
   Role.UNKNOWN,
   Role.PANE,
+  Role.SCROLL_VIEW,
 ]);
 
 

diff --git a/chrome/browser/ui/aura/accessibility/automation_manager_aura_browsertest.cc b/chrome/browser/ui/aura/accessibility/automation_manager_aura_browsertest.cc
@@ -262,8 +262,8 @@ IN_PROC_BROWSER_TEST_F(AutomationManagerAuraBrowserTest,
   widget->GetRootView()->AddChildView(view2);
   views::AXAuraObjWrapper* wrapper2 = cache_ptr->GetOrCreate(view2);
   views::View* view3 = new views::View();
-  view3->GetViewAccessibility().OverrideName("view3");
   view3->GetViewAccessibility().OverrideRole(ax::mojom::Role::kDialog);
+  view3->GetViewAccessibility().OverrideName("view3");
   view3->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
   widget->GetRootView()->AddChildView(view3);
   views::AXAuraObjWrapper* wrapper3 = cache_ptr->GetOrCreate(view3);

diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_native_views.cc b/chrome/browser/ui/views/autofill/autofill_popup_view_native_views.cc
@@ -1272,9 +1272,9 @@ void AutofillPopupWarningView::GetAccessibleNodeData(
   if (!controller)
     return;
 
+  node_data->role = ax::mojom::Role::kStaticText;
   node_data->SetName(
       controller->GetSuggestionAt(GetLineNumber()).main_text.value);
-  node_data->role = ax::mojom::Role::kStaticText;
 }
 
 void AutofillPopupWarningView::CreateContent() {

diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
@@ -448,8 +448,8 @@ class BookmarkBarView::ButtonSeparatorView : public views::Separator {
   ~ButtonSeparatorView() override = default;
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->SetName(l10n_util::GetStringUTF8(IDS_ACCNAME_SEPARATOR));
     node_data->role = ax::mojom::Role::kSplitter;
+    node_data->SetName(l10n_util::GetStringUTF8(IDS_ACCNAME_SEPARATOR));
   }
 };
 using ButtonSeparatorView = BookmarkBarView::ButtonSeparatorView;

diff --git a/chrome/browser/ui/views/borealis/borealis_installer_view.cc b/chrome/browser/ui/views/borealis/borealis_installer_view.cc
@@ -78,8 +78,8 @@ class BorealisInstallerView::TitleLabel : public views::Label {
   ~TitleLabel() override = default;
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->SetName(GetText());
     node_data->role = ax::mojom::Role::kStatus;
+    node_data->SetName(GetText());
   }
 };
 

diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc
@@ -467,8 +467,8 @@ std::u16string DownloadItemView::GetTooltipText(const gfx::Point& p) const {
 }
 
 void DownloadItemView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  node_data->SetName(accessible_name_);
   node_data->role = ax::mojom::Role::kGroup;
+  node_data->SetName(accessible_name_);
 
   // Set the description to the empty string, otherwise the tooltip will be
   // used, which is redundant with the accessible name.

diff --git a/chrome/browser/ui/views/find_bar_view.cc b/chrome/browser/ui/views/find_bar_view.cc
@@ -80,6 +80,7 @@ class FindBarMatchCountLabel : public views::Label {
   }
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    node_data->role = ax::mojom::Role::kStatus;
     if (!last_result_) {
       node_data->SetNameExplicitlyEmpty();
     } else if (last_result_->number_of_matches() < 1) {
@@ -91,7 +92,6 @@ class FindBarMatchCountLabel : public views::Label {
           base::FormatNumber(last_result_->active_match_ordinal()),
           base::FormatNumber(last_result_->number_of_matches())));
     }
-    node_data->role = ax::mojom::Role::kStatus;
   }
 
   void SetResult(const find_in_page::FindNotificationDetails& result) {

diff --git a/chrome/browser/ui/views/infobars/infobar_view.cc b/chrome/browser/ui/views/infobars/infobar_view.cc
@@ -164,8 +164,8 @@ void InfoBarView::Layout() {
 }
 
 void InfoBarView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
-  node_data->SetName(l10n_util::GetStringUTF8(IDS_ACCNAME_INFOBAR));
   node_data->role = ax::mojom::Role::kAlertDialog;
+  node_data->SetName(l10n_util::GetStringUTF8(IDS_ACCNAME_INFOBAR));
   node_data->AddStringAttribute(ax::mojom::StringAttribute::kKeyShortcuts,
                                 "Alt+Shift+A");
 }

diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -76,11 +76,11 @@ class OmniboxRemoveSuggestionButton : public views::ImageButton {
   }
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->SetName(
-        l10n_util::GetStringUTF16(IDS_ACC_REMOVE_SUGGESTION_BUTTON));
     // Although this appears visually as a button, expose as a list box option
     // so that it matches the other options within its list box container.
     node_data->role = ax::mojom::Role::kListBoxOption;
+    node_data->SetName(
+        l10n_util::GetStringUTF16(IDS_ACC_REMOVE_SUGGESTION_BUTTON));
   }
 };
 
@@ -470,6 +470,8 @@ void OmniboxResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   // ax::mojom::IntAttribute::kPosInSet/SET_SIZE and providing it via text as
   // well would result in duplicate announcements.
 
+  node_data->role = ax::mojom::Role::kListBoxOption;
+
   // TODO(tommycli): We re-fetch the original match from the popup model,
   // because |match_| already has its contents and description swapped by this
   // class, and we don't want that for the bubble. We should improve this.
@@ -486,7 +488,6 @@ void OmniboxResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
     node_data->SetName(label);
   }
 
-  node_data->role = ax::mojom::Role::kListBoxOption;
   node_data->AddIntAttribute(ax::mojom::IntAttribute::kPosInSet,
                              model_index_ + 1);
   node_data->AddIntAttribute(ax::mojom::IntAttribute::kSetSize,

diff --git a/chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.cc b/chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.cc
@@ -126,10 +126,10 @@ class OmniboxSuggestionRowButton : public views::MdTextButton {
   }
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->SetName(GetAccessibleName());
     // Although this appears visually as a button, expose as a list box option
     // so that it matches the other options within its list box container.
     node_data->role = ax::mojom::Role::kListBoxOption;
+    node_data->SetName(GetAccessibleName());
   }
 
   void SetIcon(const gfx::VectorIcon& icon) {

diff --git a/chrome/browser/ui/views/page_info/page_info_bubble_view.cc b/chrome/browser/ui/views/page_info/page_info_bubble_view.cc
@@ -295,6 +295,7 @@ void PageInfoBubbleView::AnnouncePageOpened(std::u16string announcement) {
   // Announce that the subpage was opened to inform the user about the changes
   // in the UI.
 #if BUILDFLAG(IS_MAC)
+  GetViewAccessibility().OverrideRole(ax::mojom::Role::kAlert);
   GetViewAccessibility().OverrideName(announcement);
   NotifyAccessibilityEvent(ax::mojom::Event::kAlert, true);
 #else

diff --git a/chrome/browser/ui/views/passwords/password_generation_popup_view_views.cc b/chrome/browser/ui/views/passwords/password_generation_popup_view_views.cc
@@ -318,10 +318,10 @@ void PasswordGenerationPopupViewViews::GetAccessibleNodeData(
   if (!controller_) {
     return;
   }
+  node_data->role = ax::mojom::Role::kMenuItem;
   node_data->SetName(base::JoinString(
       {controller_->SuggestedText(), controller_->password()}, u" "));
   node_data->SetDescription(controller_->HelpText());
-  node_data->role = ax::mojom::Role::kMenuItem;
 }
 
 gfx::Size PasswordGenerationPopupViewViews::CalculatePreferredSize() const {

diff --git a/chrome/browser/ui/views/profiles/profile_menu_view_base.cc b/chrome/browser/ui/views/profiles/profile_menu_view_base.cc
@@ -679,9 +679,9 @@ void ProfileMenuViewBase::BuildSyncInfoWithCallToAction(
   // Set sync info description as the name of the parent container, so
   // accessibility tools can read it together with the button text. The role
   // change is required by Windows ATs.
-  sync_info_container_->GetViewAccessibility().OverrideName(description);
   sync_info_container_->GetViewAccessibility().OverrideRole(
       ax::mojom::Role::kGroup);
+  sync_info_container_->GetViewAccessibility().OverrideName(description);
 
   // Add the prominent button at the bottom.
   auto* button =
@@ -805,10 +805,10 @@ void ProfileMenuViewBase::AddAvailableProfile(const ui::ImageModel& image_model,
     // Give the container an accessible name so accessibility tools can provide
     // context for the buttons inside it. The role change is required by Windows
     // ATs.
-    selectable_profiles_container_->GetViewAccessibility().OverrideName(
-        profile_mgmt_heading_);
     selectable_profiles_container_->GetViewAccessibility().OverrideRole(
         ax::mojom::Role::kGroup);
+    selectable_profiles_container_->GetViewAccessibility().OverrideName(
+        profile_mgmt_heading_);
   }
 
   DCHECK(!image_model.IsEmpty());

diff --git a/chrome/renderer/accessibility/read_anything_app_controller.cc b/chrome/renderer/accessibility/read_anything_app_controller.cc
@@ -132,9 +132,9 @@ ui::AXTreeUpdate GetSnapshotFromV8SnapshotLite(
       continue;
     ui::AXNodeData ax_node_data;
     SetAXNodeDataId(isolate, &v8_node_dict, &ax_node_data);
+    SetAXNodeDataRole(isolate, &v8_node_dict, &ax_node_data);
     SetAXNodeDataName(isolate, &v8_node_dict, &ax_node_data);
     SetAXNodeDataChildIds(isolate, &v8_node_dict, &ax_node_data);
-    SetAXNodeDataRole(isolate, &v8_node_dict, &ax_node_data);
     SetAXNodeDataHierarchicalLevel(isolate, &v8_node_dict, &ax_node_data);
     SetAXNodeDataUrl(isolate, &v8_node_dict, &ax_node_data);
     snapshot.nodes.push_back(ax_node_data);

diff --git a/chrome/renderer/accessibility/read_anything_app_controller_browsertest.cc b/chrome/renderer/accessibility/read_anything_app_controller_browsertest.cc
@@ -144,10 +144,13 @@ TEST_F(ReadAnythingAppControllerTest, GetTextContent) {
   std::string text_content = "Hello";
   std::string missing_text_content = "";
   std::string more_text_content = " world";
+  basic_snapshot_.nodes[1].role = ax::mojom::Role::kStaticText;
   basic_snapshot_.nodes[1].SetName(text_content);
   basic_snapshot_.nodes[1].SetNameFrom(ax::mojom::NameFrom::kContents);
+  basic_snapshot_.nodes[2].role = ax::mojom::Role::kStaticText;
   basic_snapshot_.nodes[2].SetName(missing_text_content);
   basic_snapshot_.nodes[2].SetNameFrom(ax::mojom::NameFrom::kContents);
+  basic_snapshot_.nodes[3].role = ax::mojom::Role::kStaticText;
   basic_snapshot_.nodes[3].SetName(more_text_content);
   basic_snapshot_.nodes[3].SetNameFrom(ax::mojom::NameFrom::kContents);
   OnAXTreeDistilled(basic_snapshot_, {});

diff --git a/content/browser/accessibility/browser_accessibility_manager_unittest.cc b/content/browser/accessibility/browser_accessibility_manager_unittest.cc
@@ -1421,6 +1421,7 @@ TEST_F(BrowserAccessibilityManagerTest, TestHitTestScaled) {
   ui::AXNodeData parent_childtree;
   parent_childtree.id = 3;
   parent_childtree.AddChildTreeId(child_update.tree_data.tree_id);
+  parent_childtree.role = ax::mojom::Role::kGenericContainer;
   parent_childtree.SetName("parent_childtree");
   parent_childtree.relative_bounds.bounds = gfx::RectF(100, 100, 100, 100);