Commit 7b6916c2 authored by Abigail Klein's avatar Abigail Klein Committed by Chromium LUCI CQ

[Mac A11y] Do not enable extra mac nodes on the AXTree.

Do not enable extra mac nodes on the AXTree because it caused a
crash.

Bug: 1136813
Change-Id: I46ce64b3af5ec5d4bf3abb82e30bbb96b71637e5
AX-Relnotes: Fix a browser crash on Mac.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555559
Commit-Queue: Abigail Klein <abigailbklein@google.com>
Reviewed-by: default avatarAaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#831999}
parent 94664a81
...@@ -185,6 +185,9 @@ IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest, ...@@ -185,6 +185,9 @@ IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest,
IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest, IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest,
ParameterizedAttributes_IntArray) { ParameterizedAttributes_IntArray) {
// This line is needed until we turn extra mac nodes back on by default.
BrowserAccessibilityManager::AllowExtraMacNodesForTesting();
TestAndCheck(R"~~(data:text/html, TestAndCheck(R"~~(data:text/html,
<table role="grid"><tr><td>CELL</td></tr></table>)~~", <table role="grid"><tr><td>CELL</td></tr></table>)~~",
{"AXCellForColumnAndRow([0, 0])=*"}, R"~~(AXWebArea {"AXCellForColumnAndRow([0, 0])=*"}, R"~~(AXWebArea
...@@ -201,6 +204,9 @@ IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest, ...@@ -201,6 +204,9 @@ IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest,
IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest, IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest,
ParameterizedAttributes_IntArray_NilValue) { ParameterizedAttributes_IntArray_NilValue) {
// This line is needed until we turn extra mac nodes back on by default.
BrowserAccessibilityManager::AllowExtraMacNodesForTesting();
TestAndCheck(R"~~(data:text/html, TestAndCheck(R"~~(data:text/html,
<table role="grid"></table>)~~", <table role="grid"></table>)~~",
{"AXCellForColumnAndRow([0, 0])=*"}, R"~~(AXWebArea {"AXCellForColumnAndRow([0, 0])=*"}, R"~~(AXWebArea
...@@ -211,6 +217,9 @@ IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest, ...@@ -211,6 +217,9 @@ IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest,
IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest, IN_PROC_BROWSER_TEST_F(AccessibilityTreeFormatterMacBrowserTest,
ParameterizedAttributes_IntArray_WrongParameters) { ParameterizedAttributes_IntArray_WrongParameters) {
// This line is needed until we turn extra mac nodes back on by default.
BrowserAccessibilityManager::AllowExtraMacNodesForTesting();
TestWrongParameters(R"~~(data:text/html, TestWrongParameters(R"~~(data:text/html,
<table role="grid"><tr><td>CELL</td></tr></table>)~~", <table role="grid"><tr><td>CELL</td></tr></table>)~~",
{"0, 0", "{1, 2}", "[1, NaN]", "[NaN, 1]"}, {"0, 0", "{1, 2}", "[1, NaN]", "[NaN, 1]"},
......
...@@ -246,6 +246,9 @@ TEST_F(BrowserAccessibilityMacTest, TestComputeTextEdit) { ...@@ -246,6 +246,9 @@ TEST_F(BrowserAccessibilityMacTest, TestComputeTextEdit) {
// Test Mac-specific table APIs. // Test Mac-specific table APIs.
TEST_F(BrowserAccessibilityMacTest, TableAPIs) { TEST_F(BrowserAccessibilityMacTest, TableAPIs) {
// This line is needed until we turn extra mac nodes back on by default.
BrowserAccessibilityManager::AllowExtraMacNodesForTesting();
ui::AXTreeUpdate initial_state; ui::AXTreeUpdate initial_state;
initial_state.root_id = 1; initial_state.root_id = 1;
initial_state.nodes.resize(7); initial_state.nodes.resize(7);
......
...@@ -233,6 +233,9 @@ void BrowserAccessibilityManager::Initialize( ...@@ -233,6 +233,9 @@ void BrowserAccessibilityManager::Initialize(
bool BrowserAccessibilityManager::never_suppress_or_delay_events_for_testing_ = bool BrowserAccessibilityManager::never_suppress_or_delay_events_for_testing_ =
false; false;
// A flag for use in tests to indicate that extra mac nodes are allowed.
bool BrowserAccessibilityManager::allow_extra_mac_nodes_for_testing_ = false;
// static // static
base::Optional<int32_t> BrowserAccessibilityManager::last_focused_node_id_ = {}; base::Optional<int32_t> BrowserAccessibilityManager::last_focused_node_id_ = {};
...@@ -770,6 +773,16 @@ void BrowserAccessibilityManager::NeverSuppressOrDelayEventsForTesting() { ...@@ -770,6 +773,16 @@ void BrowserAccessibilityManager::NeverSuppressOrDelayEventsForTesting() {
never_suppress_or_delay_events_for_testing_ = true; never_suppress_or_delay_events_for_testing_ = true;
} }
// static
void BrowserAccessibilityManager::AllowExtraMacNodesForTesting() {
allow_extra_mac_nodes_for_testing_ = true;
}
// static
bool BrowserAccessibilityManager::GetExtraMacNodesAllowed() {
return allow_extra_mac_nodes_for_testing_;
}
void BrowserAccessibilityManager::Decrement(const BrowserAccessibility& node) { void BrowserAccessibilityManager::Decrement(const BrowserAccessibility& node) {
if (!delegate_) if (!delegate_)
return; return;
......
...@@ -256,6 +256,12 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver, ...@@ -256,6 +256,12 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver,
// had focus. // had focus.
static void NeverSuppressOrDelayEventsForTesting(); static void NeverSuppressOrDelayEventsForTesting();
// Extra mac nodes are temporarily disabled, except for in tests.
static void AllowExtraMacNodesForTesting();
// Are extra mac nodes allowed at all? Currently only allowed in tests.
// Even when returning true, platforms other than Mac OS do not enable them.
static bool GetExtraMacNodesAllowed();
// Accessibility actions. All of these are implemented asynchronously // Accessibility actions. All of these are implemented asynchronously
// by sending a message to the renderer to perform the respective action // by sending a message to the renderer to perform the respective action
// on the given node. See the definition of |ui::AXActionData| for more // on the given node. See the definition of |ui::AXActionData| for more
...@@ -582,6 +588,9 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver, ...@@ -582,6 +588,9 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver,
// flakiness. See NeverSuppressOrDelayEventsForTesting() for details. // flakiness. See NeverSuppressOrDelayEventsForTesting() for details.
static bool never_suppress_or_delay_events_for_testing_; static bool never_suppress_or_delay_events_for_testing_;
// Extra mac nodes are disabled even on mac currently, except for in tests.
static bool allow_extra_mac_nodes_for_testing_;
const ui::AXEventGenerator& event_generator() const { const ui::AXEventGenerator& event_generator() const {
return event_generator_; return event_generator_;
} }
......
...@@ -54,7 +54,10 @@ BrowserAccessibilityManagerMac::BrowserAccessibilityManagerMac( ...@@ -54,7 +54,10 @@ BrowserAccessibilityManagerMac::BrowserAccessibilityManagerMac(
BrowserAccessibilityDelegate* delegate) BrowserAccessibilityDelegate* delegate)
: BrowserAccessibilityManager(delegate) { : BrowserAccessibilityManager(delegate) {
Initialize(initial_tree); Initialize(initial_tree);
ax_tree()->SetEnableExtraMacNodes(true); // Temporary fix. Disable extra mac nodes, which only affects column
// navigation but fixes a number of crash bugs seen only with VoiceOver.
// This does not affect verbalization of columns headers in cell navigation.
ax_tree()->SetEnableExtraMacNodes(GetExtraMacNodesAllowed());
} }
BrowserAccessibilityManagerMac::~BrowserAccessibilityManagerMac() {} BrowserAccessibilityManagerMac::~BrowserAccessibilityManagerMac() {}
......
...@@ -228,6 +228,10 @@ void DumpAccessibilityTestBase::RunTestForPlatform( ...@@ -228,6 +228,10 @@ void DumpAccessibilityTestBase::RunTestForPlatform(
// flaky. // flaky.
BrowserAccessibilityManager::NeverSuppressOrDelayEventsForTesting(); BrowserAccessibilityManager::NeverSuppressOrDelayEventsForTesting();
// Extra mac nodes are disabled temporarily for stability purposes, but keep
// them on for tests.
BrowserAccessibilityManager::AllowExtraMacNodesForTesting();
EXPECT_TRUE(NavigateToURL(shell(), GURL(url::kAboutBlankURL))); EXPECT_TRUE(NavigateToURL(shell(), GURL(url::kAboutBlankURL)));
// Exit without running the test if we can't find an expectation file. // Exit without running the test if we can't find an expectation file.
......
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