|
| 1 | +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Keeley Hammond <khammond@slack-corp.com> |
| 3 | +Date: Thu, 19 Mar 2026 00:34:37 -0700 |
| 4 | +Subject: fix: fire MENU_POPUP_START for dynamically created ARIA menus |
| 5 | + |
| 6 | +When an ARIA menu element is dynamically created (e.g. via appendChild) |
| 7 | +rather than being shown by toggling visibility, the AXMenuOpened event |
| 8 | +was not fired. The OnIgnoredChanged path handles the visibility toggle |
| 9 | +case, but OnAtomicUpdateFinished did not fire MENU_POPUP_START for |
| 10 | +newly created menu nodes. |
| 11 | + |
| 12 | +Previous attempts to fix this (crbug.com/1254875) were reverted because |
| 13 | +they fired the event too eagerly in OnNodeCreated (before the tree was |
| 14 | +fully formed) and without filtering, causing regressions with screen |
| 15 | +readers on pages that misused role="menu". |
| 16 | + |
| 17 | +This fix addresses both issues: |
| 18 | +1. Fires MENU_POPUP_START in OnAtomicUpdateFinished (after the tree |
| 19 | + update is complete) rather than in OnNodeCreated. |
| 20 | +2. Only fires if the menu has at least one menuitem child, filtering |
| 21 | + out false positives from misused role="menu" elements. |
| 22 | + |
| 23 | +MENU_POPUP_END for deleted menus is already handled by |
| 24 | +AXTreeManager::OnNodeWillBeDeleted, which fires the event directly |
| 25 | +on the menu node before destruction. |
| 26 | + |
| 27 | +The change is behind the DynamicMenuPopupEvents feature flag, disabled |
| 28 | +by default, to allow stabilization before enabling by default. Enable |
| 29 | +with --enable-features=DynamicMenuPopupEvents. |
| 30 | + |
| 31 | +This patch can be removed when a CL containing the fix is accepted |
| 32 | +into Chromium. |
| 33 | + |
| 34 | +Bug: 40794596 |
| 35 | + |
| 36 | +diff --git a/ui/accessibility/ax_event_generator.cc b/ui/accessibility/ax_event_generator.cc |
| 37 | +index 597b68bccc041a6431e35817669450e38fd56153..396820b148be04b91207e2359f9e441d331ccc10 100644 |
| 38 | +--- a/ui/accessibility/ax_event_generator.cc |
| 39 | ++++ b/ui/accessibility/ax_event_generator.cc |
| 40 | +@@ -5,6 +5,7 @@ |
| 41 | + #include "ui/accessibility/ax_event_generator.h" |
| 42 | + |
| 43 | + #include "base/containers/contains.h" |
| 44 | ++#include "base/feature_list.h" |
| 45 | + #include "base/no_destructor.h" |
| 46 | + #include "ui/accessibility/ax_enums.mojom.h" |
| 47 | + #include "ui/accessibility/ax_event.h" |
| 48 | +@@ -13,6 +14,12 @@ |
| 49 | + |
| 50 | + namespace ui { |
| 51 | + |
| 52 | ++// Feature flag for firing MENU_POPUP_START for dynamically created ARIA menus. |
| 53 | ++// Disabled by default to allow stabilization before enabling globally. |
| 54 | ++BASE_FEATURE(kDynamicMenuPopupEvents, |
| 55 | ++ "DynamicMenuPopupEvents", |
| 56 | ++ base::FEATURE_DISABLED_BY_DEFAULT); |
| 57 | ++ |
| 58 | + namespace { |
| 59 | + |
| 60 | + bool HasEvent(const std::set<AXEventGenerator::EventParams>& node_events, |
| 61 | +@@ -907,12 +914,31 @@ void AXEventGenerator::OnAtomicUpdateFinished( |
| 62 | + /*new_value*/ true); |
| 63 | + } |
| 64 | + |
| 65 | +- if (IsAlert(change.node->GetRole())) |
| 66 | ++ if (IsAlert(change.node->GetRole())) { |
| 67 | + AddEvent(change.node, Event::ALERT); |
| 68 | +- else if (change.node->data().IsActiveLiveRegionRoot()) |
| 69 | ++ } else if (change.node->data().IsActiveLiveRegionRoot()) { |
| 70 | + AddEvent(change.node, Event::LIVE_REGION_CREATED); |
| 71 | +- else if (change.node->data().IsContainedInActiveLiveRegion()) |
| 72 | ++ } else if (change.node->data().IsContainedInActiveLiveRegion()) { |
| 73 | + FireLiveRegionEvents(change.node, /* is_removal */ false); |
| 74 | ++ } |
| 75 | ++ |
| 76 | ++ // Fire MENU_POPUP_START when a menu is dynamically created (e.g. via |
| 77 | ++ // appendChild). The OnIgnoredChanged path handles menus that already exist |
| 78 | ++ // in the DOM and are shown/hidden. This handles the case where the menu |
| 79 | ++ // element itself is created on the fly. |
| 80 | ++ // Only fire if the menu has at least one menuitem child, to avoid false |
| 81 | ++ // positives from elements that misuse role="menu". |
| 82 | ++ if (base::FeatureList::IsEnabled(kDynamicMenuPopupEvents) && |
| 83 | ++ change.node->GetRole() == ax::mojom::Role::kMenu && |
| 84 | ++ !change.node->IsInvisibleOrIgnored()) { |
| 85 | ++ for (auto iter = change.node->UnignoredChildrenBegin(); |
| 86 | ++ iter != change.node->UnignoredChildrenEnd(); ++iter) { |
| 87 | ++ if (IsMenuItem(iter->GetRole())) { |
| 88 | ++ AddEvent(change.node, Event::MENU_POPUP_START); |
| 89 | ++ break; |
| 90 | ++ } |
| 91 | ++ } |
| 92 | ++ } |
| 93 | + } |
| 94 | + |
| 95 | + FireActiveDescendantEvents(); |
0 commit comments