Commit 0588cb28 authored by Aaron Leventhal's avatar Aaron Leventhal Committed by Chromium LUCI CQ

Avoid stale hypertext when non-text children change

Hypertext is updated when a node changes, or on a parent of a
textnode that changes. There are some cases where the hypertexts
do not match the available children.

In some cases this results in returning a null hyperlink, because
cached data in AXHyperText was stale. A DumpWithoutCrashing() was
added to help narrow these situations down, and the situation occurs
quite often.

The problem is that when AXPlatformNodeBase::UpdateComputedHypertext()
is called, IsLeaf() is not updated nor are the platform children.
This occurs because although the SendPendingAccessibiityEvents()
has sent an updated root, the change for the root is not merged
with other changes (TreeUpdatesCanBeMerged() returns false for any
root update, even if it has the same id as the previous root).
This causes all updates to be processed separately. When the root is
processed, other updates have not yet occurred and the root object
still appears to be a leaf.

Other possibilities to fix this:
- Do not send root_id in an update unless it's actually a new root.
This may not catch all cases of the issue, but perhaps it's a good
thing to do anyway.
- Allow updates to be merged, even when there is a change on the root.
- Compute hypertext on demand and memoize, so that it's computed after
all atomic updates are finished
- Instead of calling OnAtomicUpdateFinished() at the end of
AXTree::Unserialize(), return the updates back to the caller and allow
the caller to collect the updates and process a single call to
OnAtomicUpdateFinished();
With any of these alternate approaches, it should be possible to
remove the logic to ensure hypertext updates on parents in
CollectChangedNodesAndParentsForAtomicUpdate(), because the
renderer already sends parents nodes to the serializer
with ChildrenChanged or TextChanged().

Bug: 1157731
Change-Id: I4e9eefa9b0f90d80daa1b7db2305ca92caaf32b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587371Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836717}
parent 58c5823d
...@@ -1680,14 +1680,16 @@ void BrowserAccessibilityManager::CollectChangedNodesAndParentsForAtomicUpdate( ...@@ -1680,14 +1680,16 @@ void BrowserAccessibilityManager::CollectChangedNodesAndParentsForAtomicUpdate(
if (obj) if (obj)
nodes_needing_update->insert(obj->GetAXPlatformNode()); nodes_needing_update->insert(obj->GetAXPlatformNode());
// When a node is a text node or line break, update its parent, because
// its text is part of its hypertext.
const ui::AXNode* parent = changed_node->GetUnignoredParent(); const ui::AXNode* parent = changed_node->GetUnignoredParent();
if (!parent) if (!parent)
continue; continue;
if (changed_node->IsText() && // Update changed nodes' parents, including their hypertext:
changed_node->data().role != ax::mojom::Role::kInlineTextBox) { // Any child that changes, whether text or not, can affect the parent's
// hypertext. Hypertext uses embedded object characters to represent
// child objects, and the AXHyperText caches relevant object at
// each embedded object character offset.
if (!changed_node->IsChildOfLeaf()) {
BrowserAccessibility* parent_obj = GetFromAXNode(parent); BrowserAccessibility* parent_obj = GetFromAXNode(parent);
if (parent_obj) if (parent_obj)
nodes_needing_update->insert(parent_obj->GetAXPlatformNode()); nodes_needing_update->insert(parent_obj->GetAXPlatformNode());
......
...@@ -2124,6 +2124,16 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, ...@@ -2124,6 +2124,16 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
RunHtmlTest(FILE_PATH_LITERAL("modal-dialog-stack.html")); RunHtmlTest(FILE_PATH_LITERAL("modal-dialog-stack.html"));
} }
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
AccessibilityMoveChildHypertext) {
RunHtmlTest(FILE_PATH_LITERAL("move-child-hypertext.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
AccessibilityMoveChildHypertext2) {
RunHtmlTest(FILE_PATH_LITERAL("move-child-hypertext-2.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityNavigation) { IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityNavigation) {
RunHtmlTest(FILE_PATH_LITERAL("navigation.html")); RunHtmlTest(FILE_PATH_LITERAL("navigation.html"));
} }
......
CHILDREN-CHANGED:REMOVE index:0 CHILD:(role=ROLE_SECTION) role=ROLE_DOCUMENT_WEB ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE CHILDREN-CHANGED:REMOVE index:0 CHILD:(role=ROLE_SECTION) role=ROLE_DOCUMENT_WEB ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-CARET-MOVED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE TEXT-CARET-MOVED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
TEXT-SELECTION-CHANGED role=ROLE_DOCUMENT_WEB name='(null)' ENABLED,FOCUSABLE,FOCUSED,SENSITIVE,SHOWING,VISIBLE
\ No newline at end of file
[document web] name='done' character_count=1
++[section] character_count=0
rootWebArea name='done'
++genericContainer ignored
++++genericContainer ignored
++++++genericContainer ignored
++++++++genericContainer ignored
++++++++++genericContainer
ROLE_SYSTEM_DOCUMENT name='done' READONLY FOCUSABLE ia2_hypertext='<obj0>'
++IA2_ROLE_SECTION FOCUSABLE
<!--
@WIN-ALLOW:ia2_hypertext=*
@AURALINUX-ALLOW:character_count*
@WAIT-FOR:done
-->
<!-- This test makes sure the number of embedded object characters is correct -->
<!DOCTYPE html>
<html>
<body>
<div><div id="move-me" tabindex="0"></div> <!-- The parent div is not in the platform tree -->
<div data-hello="1"></div>
<script>
function go() {
document.querySelector('div[data-hello]').appendChild(document.getElementById("move-me"));
document.title = 'done';
}
setTimeout(go, 50);
</script>
</body>
</html>
[document web] name='done' character_count=1
++[section] character_count=0
rootWebArea name='done'
++genericContainer ignored
++++genericContainer ignored
++++++genericContainer ignored
++++++++genericContainer
++++++genericContainer ignored
ROLE_SYSTEM_DOCUMENT name='done' READONLY FOCUSABLE ia2_hypertext='<obj0>'
++IA2_ROLE_SECTION FOCUSABLE
\ No newline at end of file
<!--
@WIN-ALLOW:ia2_hypertext=*
@AURALINUX-ALLOW:character_count*
@WAIT-FOR:done
-->
<!-- This test makes sure the number of embedded object characters is correct -->
<!DOCTYPE html>
<html>
<body>
<div></div> <!-- Not in platform tree, but the moved-in contents will be -->
<div><div id="move-me" tabindex="0"></div></div>
<script>
function go() {
document.querySelector('div').appendChild(document.getElementById("move-me"));
document.title = 'done';
}
setTimeout(go, 50);
</script>
</body>
</html>
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