Skip to content

Commit

Permalink
Revert "Reland "Fire accessible MENU_POPUP_START when menu is created…
Browse files Browse the repository at this point in the history
… on the fly""

This reverts commit 2e456a6.

Reason for revert: More unexpected screen reader behavior problems.
See crbug.com/1327427 and crbug.com/1327443.

AX-Relnotes: Fixes some regressions in VoiceOver's presentation
when ARIA's "menu" role is used in more complex UI.

Bugs: 1327427, 1327443

Original change's description:
> Reland "Fire accessible MENU_POPUP_START when menu is created on the fly"
>
> Relanding now that Gmail has changed their spelling suggestions to
> no longer use menu/menuitem. That had triggered bugs in JAWS/NVDA.
> The original Chrome code was actually correct.
>
> Original change's description:
> > Fire accessible MENU_POPUP_START when menu is created on the fly
> >
> > AXEventGenerator::OnIgnoredChanged fires MENU_POPUP_START in addition
> > to SUBTREE_CREATED for the ARIA menu role. That does not cover the case
> > where the menu subtree did not already exist in the DOM, but was created
> > on the fly.
> >
> > Fire the missing event in AXEventGenerator::OnNodeCreated, which
> > should also handle the case where the new menu has ancestor elements
> > also being added to the DOM.
> >
> > AX-Relnotes: Assistive technologies are now notified when an ARIA
> > menu is created on the fly.
>
> Bug: 1305443, 1254875
> Change-Id: I17ba3d9154af685f2f8415232e26d0004e5aa24d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3557830
> Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
> Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#993034}

(cherry picked from commit abc5e18)

Bug: 1305443, 1254875
Change-Id: I6ea507dfb8b7664055f18f20573f04e3c51dc0ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3679742
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Auto-Submit: Joanmarie Diggs <jdiggs@igalia.com>
Commit-Queue: Joanmarie Diggs <jdiggs@igalia.com>
Cr-Original-Commit-Position: refs/heads/main@{#1009148}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3693146
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Prudhvikumar Bommana <pbommana@google.com>
Cr-Commit-Position: refs/branch-heads/5060@{#652}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
joanmarie authored and Chromium LUCI CQ committed Jun 7, 2022
1 parent 591cbce commit 6ba6a01
Show file tree
Hide file tree
Showing 8 changed files with 2 additions and 93 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1490,11 +1490,8 @@ void BrowserAccessibilityManager::OnNodeWillBeDeleted(ui::AXTree* tree,
if (wrapper == GetLastFocusedNode())
SetLastFocusedNode(nullptr);

// TODO(accessibility): Move this to the AXEventGenerator which fires
// MENU_POPUP_START when a node with the menu role is created. The issue to
// be solved is that after the AXEventGenerator adds MENU_POPUP_END, the
// node gets removed from the tree. Then PostprocessEvents removes the
// events from that now-removed node, thus MENU_POPUP_END never gets fired.
// We fire these here, immediately, to ensure we can send platform
// notifications prior to the actual destruction of the object.
if (node->GetRole() == ax::mojom::Role::kMenu)
FireGeneratedEvent(ui::AXEventGenerator::Event::MENU_POPUP_END, wrapper);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1137,11 +1137,6 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
RunEventTest(FILE_PATH_LITERAL("menu-opened-closed.html"));
}

IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
AccessibilityEventsMenuOpenedClosedViaInnerText) {
RunEventTest(FILE_PATH_LITERAL("menu-opened-closed-via-inner-text.html"));
}

#if BUILDFLAG(IS_WIN) && defined(ADDRESS_SANITIZER)
// TODO(crbug.com/1198056#c16): Test is flaky on Windows ASAN.
#define MAYBE_AccessibilityEventsMenubarShowHideMenus \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -870,12 +870,6 @@ public void test_menuOpenedClosed() {
performTest("menu-opened-closed.html", EMPTY_EXPECTATIONS_FILE);
}

@Test
@SmallTest
public void test_menuOpenedClosedViaInnerText() {
performTest("menu-opened-closed-via-inner-text.html", EMPTY_EXPECTATIONS_FILE);
}

@Test
@SmallTest
public void test_multipleAriaPropertiesChanged() {
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

13 changes: 0 additions & 13 deletions ui/accessibility/ax_event_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -784,15 +784,6 @@ void AXEventGenerator::OnNodeWillBeDeleted(AXTree* tree, AXNode* node) {
DCHECK_EQ(tree_, tree);
live_region_tracker_->OnNodeWillBeDeleted(*node);
FireValueInTextFieldChangedEventIfNecessary(tree, node);

// TODO(accessibility): This should also handle firing MENU_POPUP_END when a
// node with the menu role is removed. The issue to be solved is that after we
// add MENU_POPUP_END here, the node gets removed from the tree. Then
// PostprocessEvents removes the events from that now-removed node, thus
// MENU_POPUP_END never gets fired. We work around this issue currently by
// firing the event from BrowserAccessibilityManager. Adding the ability to
// fire generated events immediately should make it possible to fire
// MENU_POPUP_END here.
}

void AXEventGenerator::OnSubtreeWillBeDeleted(AXTree* tree, AXNode* node) {
Expand Down Expand Up @@ -821,10 +812,6 @@ void AXEventGenerator::OnNodeReparented(AXTree* tree, AXNode* node) {
void AXEventGenerator::OnNodeCreated(AXTree* tree, AXNode* node) {
DCHECK_EQ(tree_, tree);
FireValueInTextFieldChangedEventIfNecessary(tree, node);
if (node->GetRole() == ax::mojom::Role::kMenu &&
!node->IsInvisibleOrIgnored()) {
AddEvent(node, Event::MENU_POPUP_START);
}
}

void AXEventGenerator::OnAtomicUpdateFinished(
Expand Down

0 comments on commit 6ba6a01

Please sign in to comment.