Skip to content

Commit

Permalink
Re-land: Fire live region events when a node is removed.
Browse files Browse the repository at this point in the history 
Adds an AXLiveRegionTracker class to keep track of live regions
in an AXTree. Uses it to fix a bug where we weren't firing the
LIVE_REGION_CHANGED event on the live root when a node was removed,
only when a node was added or changed.

I'm going to follow this up with code that optionally computes
the text of a live region change, that we can use on Android,
Chrome OS, and some older versions of macOS. So AXLiveRegionTracker
is simple now, but it will be a convenient place to put that
logic.

Originally landed as https://crrev.com/c/1988742, but reverted due to
a UAF. The issue was that node deletion always called
FireLiveRegionEvents for the deleted node. In the case that the
deleted node was live region root, this would trigger a
LIVE_REGION_CHANGED on the deleted node. Instead of calling
FireLiveRegionEvents for deleted nodes, this version will only trigger
a LIVE_REGION_CHANGED event for the root on deletion and only when the
deleted node is not itself the root of the live region.

Bug: 560599, 930763
Change-Id: I3fbcd9c122a55e7a59f6cfbe61a116409cefaba6
AX-RelNotes: Fix an issue where VoiceOver failed to announce cleared live regions that were filled with the previous text.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3013676
Commit-Queue: Martin Robinson <mrobinson@igalia.com>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#908399}
  • Loading branch information
@mrobinson
mrobinson authored and Chromium LUCI CQ committed Aug 4, 2021
1 parent 5adf3de commit c2df125
Show file tree
Hide file tree
Showing 6 changed files with 351 additions and 16 deletions.
2 changes: 2 additions & 0 deletions ui/accessibility/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ component("accessibility") {
"ax_hypertext.h",
"ax_language_detection.cc",
"ax_language_detection.h",
"ax_live_region_tracker.cc",
"ax_live_region_tracker.h",
"ax_mode_observer.h",
"ax_node.cc",
"ax_node.h",
Expand Down
56 changes: 40 additions & 16 deletions ui/accessibility/ax_event_generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "base/containers/contains.h"
#include "ui/accessibility/ax_enums.mojom.h"
#include "ui/accessibility/ax_live_region_tracker.h"
#include "ui/accessibility/ax_node.h"
#include "ui/accessibility/ax_role_properties.h"

Expand Down Expand Up @@ -200,8 +201,10 @@ void swap(AXEventGenerator::Iterator& lhs, AXEventGenerator::Iterator& rhs) {
AXEventGenerator::AXEventGenerator() = default;

AXEventGenerator::AXEventGenerator(AXTree* tree) : tree_(tree) {
if (tree_)
if (tree_) {
tree_event_observation_.Observe(tree_);
live_region_tracker_ = std::make_unique<AXLiveRegionTracker>(*tree_);
}
}

AXEventGenerator::~AXEventGenerator() = default;
Expand All @@ -210,10 +213,13 @@ void AXEventGenerator::SetTree(AXTree* new_tree) {
if (tree_) {
DCHECK(tree_event_observation_.IsObservingSource(tree_));
tree_event_observation_.Reset();
live_region_tracker_.reset();
}
tree_ = new_tree;
if (tree_)
if (tree_) {
tree_event_observation_.Observe(tree_);
live_region_tracker_ = std::make_unique<AXLiveRegionTracker>(*tree_);
}
}

void AXEventGenerator::ReleaseTree() {
Expand Down Expand Up @@ -730,6 +736,12 @@ void AXEventGenerator::OnTreeDataChanged(AXTree* tree,
}

void AXEventGenerator::OnNodeWillBeDeleted(AXTree* tree, AXNode* node) {
if (AXNode* root = live_region_tracker_->GetLiveRoot(*node)) {
if (root != node)
AddEvent(root, Event::LIVE_REGION_CHANGED);
}
live_region_tracker_->OnNodeWillBeDeleted(*node);

DCHECK_EQ(tree_, tree);
tree_events_.erase(node);
}
Expand Down Expand Up @@ -768,6 +780,15 @@ void AXEventGenerator::OnAtomicUpdateFinished(

for (const auto& change : changes) {
DCHECK(change.node);

if ((change.type == NODE_CREATED || change.type == SUBTREE_CREATED ||
change.type == NODE_REPARENTED || change.type == SUBTREE_REPARENTED)) {
if (change.node->data().HasStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus)) {
live_region_tracker_->TrackNode(*change.node);
}
}

if (change.type == SUBTREE_CREATED) {
AddEvent(change.node, Event::SUBTREE_CREATED);
} else if (change.type != NODE_CREATED) {
Expand All @@ -786,6 +807,8 @@ void AXEventGenerator::OnAtomicUpdateFinished(

FireActiveDescendantEvents();

live_region_tracker_->OnAtomicUpdateFinished();

PostprocessEvents();
}

Expand All @@ -797,21 +820,22 @@ void AXEventGenerator::AddEventsForTesting(
}

void AXEventGenerator::FireLiveRegionEvents(AXNode* node) {
AXNode* live_root = node;
while (live_root && !live_root->HasStringAttribute(
ax::mojom::StringAttribute::kLiveStatus))
live_root = live_root->parent();

if (live_root &&
!live_root->GetBoolAttribute(ax::mojom::BoolAttribute::kBusy) &&
live_root->GetStringAttribute(ax::mojom::StringAttribute::kLiveStatus) !=
"off") {
// Fire LIVE_REGION_NODE_CHANGED on each node that changed.
if (!node->GetStringAttribute(ax::mojom::StringAttribute::kName).empty())
AddEvent(node, Event::LIVE_REGION_NODE_CHANGED);
// Fire LIVE_REGION_NODE_CHANGED on the root of the live region.
AddEvent(live_root, Event::LIVE_REGION_CHANGED);
AXNode* live_root = live_region_tracker_->GetLiveRootIfNotBusy(*node);

// Note that |live_root| might be nullptr if a live region was just added,
// or if it has aria-busy="true".
if (!live_root)
return;

// Fire LIVE_REGION_NODE_CHANGED on each node that changed.
if (!node->data()
.GetStringAttribute(ax::mojom::StringAttribute::kName)
.empty()) {
AddEvent(node, Event::LIVE_REGION_NODE_CHANGED);
}

// Fire LIVE_REGION_CHANGED on the root of the live region.
AddEvent(live_root, Event::LIVE_REGION_CHANGED);
}

void AXEventGenerator::FireActiveDescendantEvents() {
Expand Down
6 changes: 6 additions & 0 deletions ui/accessibility/ax_event_generator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <bitset>
#include <map>
#include <memory>
#include <ostream>
#include <set>
#include <string>
Expand All @@ -20,6 +21,8 @@

namespace ui {

class AXLiveRegionTracker;

// Subclass of AXTreeObserver that automatically generates AXEvents to fire
// based on changes to an accessibility tree. Every platform
// tends to want different events, so this class lets each platform
Expand Down Expand Up @@ -340,6 +343,9 @@ class AX_EXPORT AXEventGenerator : public AXTreeObserver {
// Please make sure that this ScopedObserver is always declared last in order
// to prevent any use-after-free.
base::ScopedObservation<AXTree, AXTreeObserver> tree_event_observation_{this};

// Helper that tracks live regions.
std::unique_ptr<AXLiveRegionTracker> live_region_tracker_;
};

AX_EXPORT std::ostream& operator<<(std::ostream& os,
Expand Down
178 changes: 178 additions & 0 deletions ui/accessibility/ax_event_generator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2742,4 +2742,182 @@ TEST(AXEventGeneratorTest, CheckedStateDescriptionChanged) {
AXEventGenerator::Event::CHECKED_STATE_DESCRIPTION_CHANGED, 1)));
}

TEST(AXEventGeneratorTest, LiveRegionNodeRemoved) {
AXTreeUpdate initial_state;
initial_state.root_id = 1;
initial_state.nodes.resize(3);
initial_state.nodes[0].id = 1;
initial_state.nodes[0].AddStringAttribute(
ax::mojom::StringAttribute::kLiveStatus, "polite");
initial_state.nodes[0].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[0].child_ids = {2, 3};
initial_state.nodes[1].id = 2;
initial_state.nodes[1].role = ax::mojom::Role::kStaticText;
initial_state.nodes[1].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[1].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Before 1");
initial_state.nodes[2].id = 3;
initial_state.nodes[2].role = ax::mojom::Role::kStaticText;
initial_state.nodes[2].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[2].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Before 2");
AXTree tree(initial_state);

AXEventGenerator event_generator(&tree);
AXTreeUpdate update = initial_state;
update.nodes.resize(1);
update.nodes[0].child_ids = {2};

EXPECT_TRUE(tree.Unserialize(update));
EXPECT_THAT(
event_generator,
UnorderedElementsAre(
HasEventAtNode(AXEventGenerator::Event::CHILDREN_CHANGED, 1),
HasEventAtNode(AXEventGenerator::Event::LIVE_REGION_CHANGED, 1)));
}

TEST(AXEventGeneratorTest, LiveRegionNodeReparented) {
AXTreeUpdate initial_state;
initial_state.root_id = 1;
initial_state.nodes.resize(5);
initial_state.nodes[0].id = 1;
initial_state.nodes[0].child_ids = {2, 3};

initial_state.nodes[1].id = 2;
initial_state.nodes[1].child_ids = {4};
initial_state.nodes[1].AddStringAttribute(
ax::mojom::StringAttribute::kLiveStatus, "polite");
initial_state.nodes[1].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");

initial_state.nodes[2].id = 3;
initial_state.nodes[2].AddStringAttribute(
ax::mojom::StringAttribute::kLiveStatus, "polite");
initial_state.nodes[2].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");

initial_state.nodes[3].id = 4;
initial_state.nodes[3].child_ids = {5};
initial_state.nodes[3].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[3].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Live child");

initial_state.nodes[4].id = 5;
initial_state.nodes[4].role = ax::mojom::Role::kStaticText;
initial_state.nodes[4].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[4].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Live child");

AXTree tree(initial_state);

AXEventGenerator event_generator(&tree);
AXTreeUpdate update = initial_state;
update.nodes[1].child_ids = {};
update.nodes[2].child_ids = {4};

EXPECT_TRUE(tree.Unserialize(update));
EXPECT_THAT(
event_generator,
UnorderedElementsAre(
HasEventAtNode(AXEventGenerator::Event::PARENT_CHANGED, 4),
HasEventAtNode(AXEventGenerator::Event::CHILDREN_CHANGED, 2),
HasEventAtNode(AXEventGenerator::Event::CHILDREN_CHANGED, 3)));

update.nodes[4].string_attributes.clear();
update.nodes[4].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
update.nodes[4].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Live child after");
event_generator.ClearEvents();
EXPECT_TRUE(tree.Unserialize(update));
EXPECT_THAT(
event_generator,
UnorderedElementsAre(
HasEventAtNode(AXEventGenerator::Event::LIVE_REGION_CHANGED, 3),
HasEventAtNode(AXEventGenerator::Event::LIVE_REGION_NODE_CHANGED, 5),
HasEventAtNode(AXEventGenerator::Event::NAME_CHANGED, 5)));
}

TEST(AXEventGeneratorTest, LiveRegionRootRemoved) {
AXTreeUpdate initial_state;
initial_state.root_id = 1;
initial_state.nodes.resize(3);
initial_state.nodes[0].id = 1;
initial_state.nodes[0].child_ids = {2};

initial_state.nodes[1].id = 2;
initial_state.nodes[1].role = ax::mojom::Role::kGenericContainer;
initial_state.nodes[1].AddStringAttribute(
ax::mojom::StringAttribute::kLiveStatus, "polite");
initial_state.nodes[1].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[1].child_ids = {3};
initial_state.nodes[2].id = 3;
initial_state.nodes[2].role = ax::mojom::Role::kStaticText;
initial_state.nodes[2].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[2].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Live");
AXTree tree(initial_state);

AXEventGenerator event_generator(&tree);
AXTreeUpdate update = initial_state;
update.nodes.resize(1);
update.nodes[0].child_ids = {};

EXPECT_TRUE(tree.Unserialize(update));
EXPECT_THAT(event_generator,
UnorderedElementsAre(HasEventAtNode(
AXEventGenerator::Event::CHILDREN_CHANGED, 1)));
}

TEST(AXEventGeneratorTest, LiveRootsNested) {
AXTreeUpdate initial_state;
initial_state.root_id = 1;
initial_state.nodes.resize(4);
initial_state.nodes[0].id = 1;
initial_state.nodes[0].child_ids = {2};

initial_state.nodes[1].id = 2;
initial_state.nodes[1].child_ids = {3};
initial_state.nodes[1].AddStringAttribute(
ax::mojom::StringAttribute::kLiveStatus, "polite");
initial_state.nodes[1].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");

initial_state.nodes[2].id = 3;
initial_state.nodes[2].AddStringAttribute(
ax::mojom::StringAttribute::kLiveStatus, "polite");
initial_state.nodes[2].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[2].child_ids = {4};

initial_state.nodes[3].id = 4;
initial_state.nodes[3].AddStringAttribute(
ax::mojom::StringAttribute::kContainerLiveStatus, "polite");
initial_state.nodes[3].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Live child");

AXTree tree(initial_state);

AXEventGenerator event_generator(&tree);
AXTreeUpdate update = initial_state;

update.nodes[3].AddStringAttribute(ax::mojom::StringAttribute::kName,
"Live child after");

EXPECT_TRUE(tree.Unserialize(update));
EXPECT_THAT(
event_generator,
UnorderedElementsAre(
HasEventAtNode(AXEventGenerator::Event::LIVE_REGION_CHANGED, 3),
HasEventAtNode(AXEventGenerator::Event::LIVE_REGION_NODE_CHANGED, 4),
HasEventAtNode(AXEventGenerator::Event::NAME_CHANGED, 4)));
}

} // namespace ui
Loading

0 comments on commit c2df125

Please sign in to comment.