Commit 066982f1 authored by Findit's avatar Findit

Revert "[fuchsia][a11y] Refactors convert node ID logic."

This reverts commit c14fae86.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 811926 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2MxNGZhZTg2Y2M0MzIyMDcyN2Q2NzNmYjJjMDRiNDJhZTI4YTI3NTQM

Sample Failed Build: https://ci.chromium.org/b/8867766288975154688

Sample Failed Step: compile

Original change's description:
> [fuchsia][a11y] Refactors convert node ID logic.
> 
> This change refactors the logic to convert an ax node ID to a fuchsia semantic node ID.
> 
> It also fixes the bug where if a regular ax node had ID == 0, it would conflict with the root ID.
> 
> R=yangsharon
> 
> TEST: web_engine_unittests
> Change-Id: Ie4ee52c65fbd5ad623986e015ad521e66c5e67fe
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2430264
> Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
> Reviewed-by: Sharon Yang <yangsharon@chromium.org>
> Commit-Queue: Lucas Radaelli <lucasradaelli@google.com>
> Cr-Commit-Position: refs/heads/master@{#811926}


Change-Id: Ide2c5fe9720fffc79de88a7b462906a4edc93d2c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439702
Cr-Commit-Position: refs/heads/master@{#811934}
parent 06e95b53
...@@ -115,6 +115,17 @@ void AccessibilityBridge::OnCommitComplete() { ...@@ -115,6 +115,17 @@ void AccessibilityBridge::OnCommitComplete() {
TryCommit(); TryCommit();
} }
uint32_t AccessibilityBridge::ConvertToFuchsiaNodeId(int32_t ax_node_id) {
if (ax_node_id == root_id_) {
// On the Fuchsia side, the root node is indicated by id
// |kSemanticNodeRootId|, which is 0.
return kSemanticNodeRootId;
} else {
// AXNode ids are signed, Semantic Node ids are unsigned.
return bit_cast<uint32_t>(ax_node_id);
}
}
void AccessibilityBridge::AccessibilityEventReceived( void AccessibilityBridge::AccessibilityEventReceived(
const content::AXEventNotificationDetails& details) { const content::AXEventNotificationDetails& details) {
// Updates to AXTree must be applied first. // Updates to AXTree must be applied first.
...@@ -132,7 +143,7 @@ void AccessibilityBridge::AccessibilityEventReceived( ...@@ -132,7 +143,7 @@ void AccessibilityBridge::AccessibilityEventReceived(
pending_hit_test_callbacks_.find(event.action_request_id) != pending_hit_test_callbacks_.find(event.action_request_id) !=
pending_hit_test_callbacks_.end()) { pending_hit_test_callbacks_.end()) {
fuchsia::accessibility::semantics::Hit hit; fuchsia::accessibility::semantics::Hit hit;
hit.set_node_id(ConvertToFuchsiaNodeId(event.id, root_id_)); hit.set_node_id(ConvertToFuchsiaNodeId(event.id));
// Run the pending callback with the hit. // Run the pending callback with the hit.
pending_hit_test_callbacks_[event.action_request_id](std::move(hit)); pending_hit_test_callbacks_[event.action_request_id](std::move(hit));
...@@ -217,7 +228,7 @@ void AccessibilityBridge::DeleteSubtree(ui::AXNode* node) { ...@@ -217,7 +228,7 @@ void AccessibilityBridge::DeleteSubtree(ui::AXNode* node) {
if (node->id() != root_id_) { if (node->id() != root_id_) {
to_send_.push_back( to_send_.push_back(
SemanticUpdateOrDelete(SemanticUpdateOrDelete::Type::DELETE, {}, SemanticUpdateOrDelete(SemanticUpdateOrDelete::Type::DELETE, {},
ConvertToFuchsiaNodeId(node->id(), root_id_))); ConvertToFuchsiaNodeId(node->id())));
} }
for (ui::AXNode* child : node->children()) for (ui::AXNode* child : node->children())
DeleteSubtree(child); DeleteSubtree(child);
......
...@@ -86,6 +86,10 @@ class WEB_ENGINE_EXPORT AccessibilityBridge ...@@ -86,6 +86,10 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
// Callback for SemanticTree::CommitUpdates. // Callback for SemanticTree::CommitUpdates.
void OnCommitComplete(); void OnCommitComplete();
// Converts AXNode ids to Semantic Node ids, and handles special casing of
// the root.
uint32_t ConvertToFuchsiaNodeId(int32_t ax_node_id);
// Deletes all nodes in subtree rooted at and including |node|, unless // Deletes all nodes in subtree rooted at and including |node|, unless
// |node| is the root of the tree. |tree| and |node| are owned by the // |node| is the root of the tree. |tree| and |node| are owned by the
// accessibility bridge. // accessibility bridge.
......
...@@ -8,11 +8,9 @@ ...@@ -8,11 +8,9 @@
#include <fuchsia/ui/gfx/cpp/fidl.h> #include <fuchsia/ui/gfx/cpp/fidl.h>
#include <fuchsia/ui/views/cpp/fidl.h> #include <fuchsia/ui/views/cpp/fidl.h>
#include <lib/ui/scenic/cpp/commands.h> #include <lib/ui/scenic/cpp/commands.h>
#include <stdint.h>
#include <vector> #include <vector>
#include "base/bit_cast.h" #include "base/bit_cast.h"
#include "base/numerics/safe_conversions.h"
#include "ui/accessibility/ax_enums.mojom.h" #include "ui/accessibility/ax_enums.mojom.h"
#include "ui/accessibility/ax_tree_id.h" #include "ui/accessibility/ax_tree_id.h"
#include "ui/gfx/geometry/rect_f.h" #include "ui/gfx/geometry/rect_f.h"
...@@ -21,14 +19,6 @@ using fuchsia::accessibility::semantics::MAX_LABEL_SIZE; ...@@ -21,14 +19,6 @@ using fuchsia::accessibility::semantics::MAX_LABEL_SIZE;
namespace { namespace {
// Fuchsia's default root node ID.
constexpr uint32_t kFuchsiaRootNodeId = 0;
// Remmaped value for an AxNode ID that is 0 and is not a root.
// Value is chosen specifically to be outside the range of a 32-bit signed int,
// so as to not conflict with other values used by Chromium.
constexpr uint32_t kZeroIdRemappedForFuchsia = 1u + INT32_MAX;
fuchsia::accessibility::semantics::Role ConvertRole(ax::mojom::Role role) { fuchsia::accessibility::semantics::Role ConvertRole(ax::mojom::Role role) {
if (role == ax::mojom::Role::kButton) if (role == ax::mojom::Role::kButton)
return fuchsia::accessibility::semantics::Role::BUTTON; return fuchsia::accessibility::semantics::Role::BUTTON;
...@@ -137,7 +127,7 @@ std::vector<uint32_t> ConvertChildIds(std::vector<int32_t> ids) { ...@@ -137,7 +127,7 @@ std::vector<uint32_t> ConvertChildIds(std::vector<int32_t> ids) {
std::vector<uint32_t> child_ids; std::vector<uint32_t> child_ids;
child_ids.reserve(ids.size()); child_ids.reserve(ids.size());
for (auto i : ids) { for (auto i : ids) {
child_ids.push_back(base::checked_cast<uint32_t>(i)); child_ids.push_back(bit_cast<uint32_t>(i));
} }
return child_ids; return child_ids;
} }
...@@ -167,7 +157,7 @@ fuchsia::ui::gfx::mat4 ConvertTransform(gfx::Transform* transform) { ...@@ -167,7 +157,7 @@ fuchsia::ui::gfx::mat4 ConvertTransform(gfx::Transform* transform) {
fuchsia::accessibility::semantics::Node AXNodeDataToSemanticNode( fuchsia::accessibility::semantics::Node AXNodeDataToSemanticNode(
const ui::AXNodeData& node) { const ui::AXNodeData& node) {
fuchsia::accessibility::semantics::Node fuchsia_node; fuchsia::accessibility::semantics::Node fuchsia_node;
fuchsia_node.set_node_id(base::checked_cast<uint32_t>(node.id)); fuchsia_node.set_node_id(bit_cast<uint32_t>(node.id));
fuchsia_node.set_role(ConvertRole(node.role)); fuchsia_node.set_role(ConvertRole(node.role));
fuchsia_node.set_states(ConvertStates(node)); fuchsia_node.set_states(ConvertStates(node));
fuchsia_node.set_attributes(ConvertAttributes(node)); fuchsia_node.set_attributes(ConvertAttributes(node));
...@@ -202,16 +192,3 @@ bool ConvertAction(fuchsia::accessibility::semantics::Action fuchsia_action, ...@@ -202,16 +192,3 @@ bool ConvertAction(fuchsia::accessibility::semantics::Action fuchsia_action,
return false; return false;
} }
} }
uint32_t ConvertToFuchsiaNodeId(int32_t ax_node_id, int32_t ax_root_node_id) {
if (ax_node_id == ax_root_node_id)
return kFuchsiaRootNodeId;
if (ax_node_id == kFuchsiaRootNodeId) {
// This AxNode is not the root, but its ID is the same as Fuchsia's root ID.
// We remap it to something different to not create a confflict.
return kZeroIdRemappedForFuchsia;
}
return base::checked_cast<uint32_t>(ax_node_id);
}
...@@ -24,12 +24,4 @@ AXNodeDataToSemanticNode(const ui::AXNodeData& node); ...@@ -24,12 +24,4 @@ AXNodeDataToSemanticNode(const ui::AXNodeData& node);
bool ConvertAction(fuchsia::accessibility::semantics::Action fuchsia_action, bool ConvertAction(fuchsia::accessibility::semantics::Action fuchsia_action,
ax::mojom::Action* mojom_action); ax::mojom::Action* mojom_action);
// Converts |ax_node_id|, which is signed, to a Fuchsia node ID, which is
// unsigned.
// Fuchsia requires root node IDs to be zero. This function ensures
// that the conversion takes that into account.
// If |ax_node_id| is 0 and is not the root, we return the
// MAX(int32_t) + 1, which is a number that will never conflict with other IDs.
uint32_t ConvertToFuchsiaNodeId(int32_t ax_node_id, int32_t ax_root_node_id);
#endif // FUCHSIA_ENGINE_BROWSER_AX_TREE_CONVERTER_H_ #endif // FUCHSIA_ENGINE_BROWSER_AX_TREE_CONVERTER_H_
...@@ -41,9 +41,6 @@ ui::AXNodeData CreateAXNodeData(ax::mojom::Role role, ...@@ -41,9 +41,6 @@ ui::AXNodeData CreateAXNodeData(ax::mojom::Role role,
base::StringPiece description, base::StringPiece description,
ax::mojom::CheckedState checked_state) { ax::mojom::CheckedState checked_state) {
ui::AXNodeData node; ui::AXNodeData node;
// Important! ID must be set to zero here because its default value (-1), will
// fail when getting converted to an unsigned int (Fuchsia's ID format).
node.id = 0;
node.role = role; node.role = role;
node.AddAction(action); node.AddAction(action);
node.AddIntAttribute(ax::mojom::IntAttribute::kCheckedState, node.AddIntAttribute(ax::mojom::IntAttribute::kCheckedState,
...@@ -116,9 +113,8 @@ TEST_F(AXTreeConverterTest, AllFieldsSetAndEqual) { ...@@ -116,9 +113,8 @@ TEST_F(AXTreeConverterTest, AllFieldsSetAndEqual) {
states.set_hidden(false); states.set_hidden(false);
states.set_selected(false); states.set_selected(false);
auto expected_node = CreateSemanticNode( auto expected_node = CreateSemanticNode(
static_cast<uint32_t>(source_node_data.id), Role::BUTTON, source_node_data.id, Role::BUTTON, std::move(attributes),
std::move(attributes), std::move(states), std::move(states), std::vector<Action>{Action::SET_FOCUS},
std::vector<Action>{Action::SET_FOCUS},
std::vector<uint32_t>{kChildId1, kChildId2, kChildId3}, box, mat.value); std::vector<uint32_t>{kChildId1, kChildId2, kChildId3}, box, mat.value);
EXPECT_TRUE(fidl::Equals(converted_node, expected_node)); EXPECT_TRUE(fidl::Equals(converted_node, expected_node));
...@@ -126,7 +122,6 @@ TEST_F(AXTreeConverterTest, AllFieldsSetAndEqual) { ...@@ -126,7 +122,6 @@ TEST_F(AXTreeConverterTest, AllFieldsSetAndEqual) {
TEST_F(AXTreeConverterTest, SomeFieldsSetAndEqual) { TEST_F(AXTreeConverterTest, SomeFieldsSetAndEqual) {
ui::AXNodeData source_node_data; ui::AXNodeData source_node_data;
source_node_data.id = 0;
source_node_data.AddAction(ax::mojom::Action::kFocus); source_node_data.AddAction(ax::mojom::Action::kFocus);
source_node_data.AddAction(ax::mojom::Action::kSetValue); source_node_data.AddAction(ax::mojom::Action::kSetValue);
source_node_data.child_ids = std::vector<int32_t>{kChildId1}; source_node_data.child_ids = std::vector<int32_t>{kChildId1};
...@@ -138,11 +133,10 @@ TEST_F(AXTreeConverterTest, SomeFieldsSetAndEqual) { ...@@ -138,11 +133,10 @@ TEST_F(AXTreeConverterTest, SomeFieldsSetAndEqual) {
converted_node.node_id()); converted_node.node_id());
Node expected_node; Node expected_node;
expected_node.set_node_id(static_cast<uint32_t>(source_node_data.id)); expected_node.set_node_id(source_node_data.id);
expected_node.set_actions( expected_node.set_actions(
std::vector<Action>{Action::SET_FOCUS, Action::SET_VALUE}); std::vector<Action>{Action::SET_FOCUS, Action::SET_VALUE});
expected_node.set_child_ids( expected_node.set_child_ids(std::vector<uint32_t>{kChildId1});
std::vector<uint32_t>{static_cast<uint32_t>(kChildId1)});
expected_node.set_role(Role::IMAGE); expected_node.set_role(Role::IMAGE);
States states; States states;
states.set_hidden(false); states.set_hidden(false);
...@@ -204,19 +198,4 @@ TEST_F(AXTreeConverterTest, FieldMismatch) { ...@@ -204,19 +198,4 @@ TEST_F(AXTreeConverterTest, FieldMismatch) {
EXPECT_FALSE(fidl::Equals(converted_node, expected_node)); EXPECT_FALSE(fidl::Equals(converted_node, expected_node));
} }
TEST_F(AXTreeConverterTest, ConvertToFuchsiaNodeId) {
// Root AxNode is 0, Fuchsia is also 0.
EXPECT_EQ(0u, ConvertToFuchsiaNodeId(0, 0));
// Root AxNode is not 0, Fuchsia is still 0.
EXPECT_EQ(0u, ConvertToFuchsiaNodeId(2, 2));
// Regular AxNode is 0, Fuchsia can't be 0.
EXPECT_EQ(static_cast<uint32_t>(std::numeric_limits<int32_t>::max()) + 1,
ConvertToFuchsiaNodeId(0, 2));
// Regular AxNode is not 0, Fuchsia is same value.
EXPECT_EQ(10u, ConvertToFuchsiaNodeId(10, 0));
}
} // namespace } // namespace
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