Skip to content

GitLab

T
tangled
Commit 3131846f authored by Devlin's avatar Devlin Committed by Commit Bot
Browse files
Options

Revert "Fire WIN HIDE event only on ignored ancestor" 

This reverts commit 4856b612.

Reason for revert:

New test is failing on windows asan.
https://ci.chromium.org/p/chromium/builders/ci/win-asan/13719

[ RUN      ] All/DumpAccessibilityEventsTest.AccessibilityEventsAriaHiddenDescendantsAlreadyIgnored/win

DevTools listening on ws://127.0.0.1:56078/devtools/browser/cb8ef55e-fd6c-40c9-9bb9-69a6e2f10a39
[3056:12476:0805/095807.582:ERROR:browser_test_utils.cc(712)] No committed entry.
[3056:12476:0805/095810.629:INFO:accessibility_notification_waiter.cc(147)] OnAccessibilityEvent Event::kChildrenChanged
[3056:12476:0805/095810.629:INFO:accessibility_notification_waiter.cc(147)] OnAccessibilityEvent Event::kLayoutComplete
[3056:12476:0805/095810.629:INFO:accessibility_notification_waiter.cc(147)] OnAccessibilityEvent Event::kLoadComplete
[3056:12476:0805/095810.693:INFO:accessibility_notification_waiter.cc(147)] OnAccessibilityEvent Event::kChildrenChanged
[3056:12476:0805/095810.694:ERROR:dump_accessibility_events_browsertest.cc(103)] === IsRecordingComplete#1 run_until size=0
[3056:12476:0805/095810.703:INFO:accessibility_notification_waiter.cc(147)] OnAccessibilityEvent Event::kChildrenChanged
[3056:12476:0805/095810.856:INFO:accessibility_notification_waiter.cc(147)] OnAccessibilityEvent Event::kChildrenChanged
[3056:12476:0805/095810.857:INFO:accessibility_notification_waiter.cc(147)] OnAccessibilityEvent Event::kEndOfTest
[3056:12476:0805/095810.866:INFO:dump_accessibility_test_helper.cc(93)] Testing: C:/b/s/w/ir/content/test/data/accessibility/event/aria-hidden-descendants-already-ignored.html
[3056:12476:0805/095810.867:INFO:dump_accessibility_test_helper.cc(95)] Expected output: C:/b/s/w/ir/content/test/data/accessibility/event/aria-hidden-descendants-already-ignored-expected-win.txt
[3056:12476:0805/095810.867:ERROR:dump_accessibility_test_helper.cc(127)] Diff:
* Line Expected
- ---- --------
     1 EVENT_OBJECT_HIDE on <div#heading-root.a> role=ROLE_SYSTEM_GROUPING name="Heading" INVISIBLE level=2
     2 EVENT_OBJECT_REORDER on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL
*    3 IA2_EVENT_TEXT_INSERTED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL new_text={'<obj>' start=0 end=1}
*    4 IA2_EVENT_TEXT_REMOVED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL old_text={'<obj>' start=0 end=1}

Actual
------
EVENT_OBJECT_HIDE on <div#heading-root.a> role=ROLE_SYSTEM_GROUPING name="Heading" INVISIBLE level=2
EVENT_OBJECT_REORDER on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL
EVENT_OBJECT_REORDER on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL
IA2_EVENT_TEXT_INSERTED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL new_text={'<obj>' start=0 end=1}
IA2_EVENT_TEXT_REMOVED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL old_text={'<obj>' start=0 end=1}
IA2_EVENT_TEXT_REMOVED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL old_text={'<obj>' start=0 end=1}
<-- End-of-file -->



Original change's description:
> Fire WIN HIDE event only on ignored ancestor
> 
> Google drive expand/collapse state is not being announced due to
> excessive HIDE/SHOW/REORDER events flooding NVDA, and NVDA end up
> discarding most events.
> 
> For example, if aria-hidden="true" is set on an ancestor node, which
> would cause that node many of its descendants to be set to IGNORED
> state which triggering too many EVENT_OBJECT_HIDE and causes NVDA
> to drop the events. In reality, we only want EVENT_OBJECT_HIDE to be
> generated on the ancestor node where aria-hidden="true" is set, so
> not to flood and confuse NVDA with excessive events.
> 
> This change fixes the above issue by firing EVENT_OBJECT_HIDE on the
> root should the entire subtree changes to IGNORED state.
> 
> Suppressing excessive SHOW/REORDER events will be addressed in follow up
> patches.
> 
> AX-RelNotes: NVDA can now announce Google drive "My Drive" expand/
> collapse state.
> 
> Bug: 1019420
> Change-Id: I4fdabd6740630a149c1b22cdb3180d2f75807b10
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2309450
> Commit-Queue: Victor Fei <vicfei@microsoft.com>
> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> Reviewed-by: Adam Ettenberger <Adam.Ettenberger@microsoft.com>
> Cr-Commit-Position: refs/heads/master@{#795017}

TBR=dmazzoni@chromium.org,aleventhal@chromium.org,Adam.Ettenberger@microsoft.com,vicfei@microsoft.com

Change-Id: I4354e1effb7b3fe4a01caef00f696513ecaacbbc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1019420
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339828Reviewed-by: default avatarDevlin <rdevlin.cronin@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795159}
parent 73e77f85
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 10 additions and 681 deletions
content/browser/accessibility/dump_accessibility_events_browsertest.cc
View file @ 3131846f
......@@ -544,22 +544,6 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
RunEventTest(FILE_PATH_LITERAL("css-display.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
AccessibilityEventsAriaHiddenDescendants) {
RunEventTest(FILE_PATH_LITERAL("aria-hidden-descendants.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
AccessibilityEventsAriaHiddenDescendantsAlreadyIgnored) {
RunEventTest(
FILE_PATH_LITERAL("aria-hidden-descendants-already-ignored.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
AccessibilityEventsCSSDisplayDescendants) {
RunEventTest(FILE_PATH_LITERAL("css-display-descendants.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
AccessibilityEventsCSSFlexTextUpdate) {
RunEventTest(FILE_PATH_LITERAL("css-flex-text-update.html"));
......@@ -570,11 +554,6 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
RunEventTest(FILE_PATH_LITERAL("css-visibility.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
AccessibilityEventsCSSVisibilityDescendants) {
RunEventTest(FILE_PATH_LITERAL("css-visibility-descendants.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
AccessibilityEventsCSSCollapse) {
RunEventTest(FILE_PATH_LITERAL("css-visibility-collapse.html"));
......
content/test/data/accessibility/event/aria-hidden-descendants-already-ignored-expected-win.txt deleted 100644 → 0
View file @ 73e77f85
EVENT_OBJECT_HIDE on <div#heading-root.a> role=ROLE_SYSTEM_GROUPING name="Heading" INVISIBLE level=2
EVENT_OBJECT_REORDER on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL
IA2_EVENT_TEXT_INSERTED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL new_text={'<obj>' start=0 end=1}
IA2_EVENT_TEXT_REMOVED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL old_text={'<obj>' start=0 end=1}
content/test/data/accessibility/event/aria-hidden-descendants-already-ignored.html deleted 100644 → 0
View file @ 73e77f85
<!DOCTYPE html>
<html>
<body>
<!-- Hide events only need to occur on the root of what's shown/hidden,
not for each descendant, with some descendants already ignored. -->
<div role="toolbar">
<div id="heading-root" role="heading" aria-label="Heading" class="a">
<div> <!-- already ignored-->
<div> <!-- already ignored -->
<div id="heading-child">
<div id="heading-grandchild"></div>
</div>
</div>
</div>
</div>
</div>
<script>
function go() {
document.querySelector('.a').setAttribute("aria-hidden", "true");
}
</script>
</body>
</html>
content/test/data/accessibility/event/aria-hidden-descendants-expected-win.txt deleted 100644 → 0
View file @ 73e77f85
EVENT_OBJECT_HIDE on <div#heading-root.a> role=ROLE_SYSTEM_GROUPING name="Heading" INVISIBLE level=2
EVENT_OBJECT_REORDER on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL
EVENT_OBJECT_SHOW on <div#banner-root.b> role=ROLE_SYSTEM_GROUPING name="Banner"
IA2_EVENT_TEXT_INSERTED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL new_text={'<obj>' start=0 end=1}
IA2_EVENT_TEXT_REMOVED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL old_text={'<obj>' start=0 end=1}
content/test/data/accessibility/event/aria-hidden-descendants.html deleted 100644 → 0
View file @ 73e77f85
<!DOCTYPE html>
<html>
<body>
<!-- Show/hide events only need to occur on the root of what's shown/hidden,
not for each descendant -->
<div role="toolbar">
<div id="heading-root" role="heading" aria-label="Heading" class="a">
<div id="heading-child">
<div id="heading-grandchild"></div>
</div>
</div>
<div id="banner-root" role="banner" aria-label="Banner" aria-hidden="true" class="b">
<div id="banner-child">
<div id="banner-grandchild"></div>
</div>
</div>
</div>
<script>
function go() {
document.querySelector('.a').setAttribute("aria-hidden", "true");
document.querySelector('.b').removeAttribute("aria-hidden");
}
</script>
</body>
</html>
content/test/data/accessibility/event/css-display-descendants-expected-win.txt deleted 100644 → 0
View file @ 73e77f85
EVENT_OBJECT_HIDE on <div#heading-root.a> role=ROLE_SYSTEM_GROUPING name="Heading" level=2
EVENT_OBJECT_REORDER on <div#banner-child> role=ROLE_SYSTEM_GROUPING
EVENT_OBJECT_REORDER on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL
EVENT_OBJECT_SHOW on <div#banner-root.b> role=ROLE_SYSTEM_GROUPING name="Banner"
IA2_EVENT_TEXT_INSERTED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL new_text={'<obj>' start=0 end=1}
IA2_EVENT_TEXT_REMOVED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL old_text={'<obj>' start=0 end=1}
content/test/data/accessibility/event/css-display-descendants.html deleted 100644 → 0
View file @ 73e77f85
<!DOCTYPE html>
<html>
<head>
<style>
.a { display: block; }
.b { display: none; }
body.flip .a { display: none; }
body.flip .b { display: block; }
</style>
</head>
<body>
<!-- Show/hide events only need to occur on the root of what's shown/hidden,
not for each descendant -->
<div role="toolbar">
<div id="heading-root" role="heading" aria-label="Heading" class="a">
<div id="heading-child">
<div id="heading-grandchild"></div>
</div>
</div>
<div id="banner-root" role="banner" aria-label="Banner" class="b">
<div id="banner-child">
<div id="banner-grandchild"></div>
</div>
</div>
</div>
<script>
function go() {
document.body.className = 'flip';
}
</script>
</body>
</html>
content/test/data/accessibility/event/css-visibility-descendants-expected-win.txt deleted 100644 → 0
View file @ 73e77f85
EVENT_OBJECT_HIDE on <div#heading-root.a> role=ROLE_SYSTEM_GROUPING name="Heading" INVISIBLE level=2
EVENT_OBJECT_REORDER on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL
EVENT_OBJECT_SHOW on <div#banner-root.b> role=ROLE_SYSTEM_GROUPING name="Banner"
IA2_EVENT_TEXT_INSERTED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL new_text={'<obj>' start=0 end=1}
IA2_EVENT_TEXT_REMOVED on <div> role=ROLE_SYSTEM_TOOLBAR IA2_STATE_HORIZONTAL old_text={'<obj>' start=0 end=1}
content/test/data/accessibility/event/css-visibility-descendants.html deleted 100644 → 0
View file @ 73e77f85
<!DOCTYPE html>
<html>
<head>
<style>
.a { visibility: visible; }
.b { visibility: hidden; }
body.flip .a { visibility: hidden; }
body.flip .b { visibility: visible; }
</style>
</head>
<body>
<!-- Show/hide events only need to occur on the root of what's shown/hidden,
not for each descendant -->
<div role="toolbar">
<div id="heading-root" role="heading" aria-label="Heading" class="a">
<div id="heading-child">
<div id="heading-grandchild"></div>
</div>
</div>
<div id="banner-root" role="banner" aria-label="Banner" class="b">
<div id="banner-child">
<div id="banner-grandchild"></div>
</div>
</div>
</div>
<script>
function go() {
document.body.className = 'flip';
}
</script>
</body>
</html>
ui/accessibility/ax_event_generator.cc
View file @ 3131846f
......@@ -47,19 +47,6 @@ void RemoveEvent(std::set<AXEventGenerator::EventParams>* node_events,
}
}
// If a node toggled its ignored state, don't also fire children-changed because
// platforms likely will do that in response to ignored-changed.
// Suppress name- and description-changed because those can be emitted as a side
// effect of calculating alternative text values for a newly-displayed object.
// Ditto for text attributes such as foreground and background colors.
void RemoveEventsDueToIgnoredChanged(
std::set<AXEventGenerator::EventParams>* node_events) {
RemoveEvent(node_events, AXEventGenerator::Event::CHILDREN_CHANGED);
RemoveEvent(node_events, AXEventGenerator::Event::DESCRIPTION_CHANGED);
RemoveEvent(node_events, AXEventGenerator::Event::NAME_CHANGED);
RemoveEvent(node_events, AXEventGenerator::Event::TEXT_ATTRIBUTE_CHANGED);
}
} // namespace
AXEventGenerator::EventParams::EventParams(
......@@ -678,51 +665,7 @@ bool AXEventGenerator::ShouldFireLoadEvents(AXNode* node) {
data.relative_bounds.bounds.height();
}
void AXEventGenerator::TrimEventsDueToAncestorIgnoredChanged(
AXNode* node,
std::map<AXNode*, bool>& ancestor_ignored_changed_map) {
DCHECK(node);
// Recursively compute and cache ancestor ignored changed results in
// |ancestor_ignored_changed_map|, if |node|'s ancestors have become ignored
// and the ancestor's ignored changed results have not been cached.
if (node->parent() &&
!base::Contains(ancestor_ignored_changed_map, node->parent())) {
TrimEventsDueToAncestorIgnoredChanged(node->parent(),
ancestor_ignored_changed_map);
}
// If an ancestor of |node| changed to ignored state, update the corresponding
// entry in the map for |node| based on the ancestor result (i.e. if an
// ancestor changed to ignored state, set the entry in the map to true for the
// current node). If |node|'s state changed to ignored as well, we want to
// remove its IGNORED_CHANGED event.
const auto& map_iter = ancestor_ignored_changed_map.find(node->parent());
const auto& events_iter = tree_events_.find(node);
if (map_iter != ancestor_ignored_changed_map.end() && map_iter->second) {
ancestor_ignored_changed_map.insert(std::make_pair(node, true));
if (node->IsIgnored() && events_iter != tree_events_.end()) {
RemoveEvent(&(events_iter->second), Event::IGNORED_CHANGED);
RemoveEventsDueToIgnoredChanged(&(events_iter->second));
}
return;
}
// If ignored changed results are not cached, calculate the corresponding
// entry for |node| in the map using the ignored states and events of |node|.
if (events_iter != tree_events_.end() &&
HasEvent(events_iter->second, Event::IGNORED_CHANGED) &&
node->IsIgnored()) {
ancestor_ignored_changed_map.insert(std::make_pair(node, true));
return;
}
ancestor_ignored_changed_map.insert(std::make_pair(node, false));
}
void AXEventGenerator::PostprocessEvents() {
std::map<AXNode*, bool> ancestor_ignored_changed_map;
auto iter = tree_events_.begin();
while (iter != tree_events_.end()) {
AXNode* node = iter->first;
......@@ -735,13 +678,17 @@ void AXEventGenerator::PostprocessEvents() {
RemoveEvent(&node_events, Event::LIVE_REGION_CHANGED);
}
// If a node toggled its ignored state, don't also fire children-changed
// because platforms likely will do that in response to ignored-changed.
// Suppress name- and description-changed because those can be emitted
// as a side effect of calculating alternative text values for a newly-
// displayed object. Ditto for text attributes such as foreground and
// background colors.
if (HasEvent(node_events, Event::IGNORED_CHANGED)) {
// If a node toggled its ignored state from show to hide, we only want to
// fire IGNORED_CHANGED event on the top most ancestor where this ignored
// state change takes place and suppress all the descendants's
// IGNORED_CHANGED events.
TrimEventsDueToAncestorIgnoredChanged(node, ancestor_ignored_changed_map);
RemoveEventsDueToIgnoredChanged(&node_events);
RemoveEvent(&node_events, Event::CHILDREN_CHANGED);
RemoveEvent(&node_events, Event::DESCRIPTION_CHANGED);
RemoveEvent(&node_events, Event::NAME_CHANGED);
RemoveEvent(&node_events, Event::TEXT_ATTRIBUTE_CHANGED);
}
// When the selected option in an expanded select element changes, the
......
ui/accessibility/ax_event_generator.h
View file @ 3131846f
......@@ -235,18 +235,6 @@ class AX_EXPORT AXEventGenerator : public AXTreeObserver {
void FireActiveDescendantEvents();
void FireRelationSourceEvents(AXTree* tree, AXNode* target_node);
bool ShouldFireLoadEvents(AXNode* node);
// Remove excessive events for a tree update containing node.
// We remove certain events on a node when it changes to IGNORED state and one
// of the node's ancestor has also changed to IGNORED in the same tree update.
// |ancestor_has_ignored_map| contains if a node's ancestor has changed to
// IGNORED state.
// Map's key is: an ax node.
// Map's value is:
// - True if an ancestor of node changed to IGNORED state.
// - False if no ancestor of node changed to IGNORED state.
void TrimEventsDueToAncestorIgnoredChanged(
AXNode* node,
std::map<AXNode*, bool>& ancestor_has_ignored_map);
void PostprocessEvents();
static void GetRestrictionStates(ax::mojom::Restriction restriction,
bool* is_enabled,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment