Skip to content

AX: Default to walking the DOM rather than the render tree when building the accessibility tree #35430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

twilco
Copy link
Contributor

@twilco twilco commented Oct 18, 2024

6a44b38

AX: Default to walking the DOM rather than the render tree when building the accessibility tree
https://bugs.webkit.org/show_bug.cgi?id=281749
rdar://138184652

Reviewed by Chris Fleizach.

Walking the DOM rather than the render tree allows many simplifications to our code. Before this
patch, we had several display:contents special cases scattered throughout the code that are no longer
needed:

  1. AccessibilityRenderObject::addNodeOnlyChildren() added display:contents children to the right
     place in an object's render-tree children retroactively. This function is now unnecessary —
     we get this property for free by walking the DOM to build the AX tree.

  2. AccessibilityObject::insertChild had a special case to prevent double-insertion of display:contents children.

  3. AccessibilityRenderObject::addCanvasChildren() only existed because canvas descendants are not rendered,
     and thus not picked up by a render tree walk. This function is now removed.

  4. Numerous other more minor checks throughout the code that only existed because we had to deal with
     anonymous renderers are removed.

The flip side of this is that we don't get "interesting" anonymous renderers for free anymore, e.g.
list markers, CSS ::before and CSS ::after, mfenced element `open`, `close`, `separators` anonymous
renderers. This is still a great tradeoff, as there are far more uninteresting anonymous renderers
than there are interesting ones.

I tried to apply this change to USE(ATSPI) ports, but there are GTK tests that expect the presence of certain
anonymous renderers that I'm not sure we can outright remove (e.g. those in accessibility/gtk/replaced-objects-in-anonymous-blocks.html).
I filed a follow-up bug (https://bugs.webkit.org/show_bug.cgi?id=282117) to track this, more details are there.

Another important aspect of this change is the addition of AXObjectCache::onStyleChange. We have accessibility
properties cached based on element styles that never get updated, and the addition of this function gives us the
framework for starting to fix those.

* LayoutTests/accessibility/mac/ruby-hierarchy-roles-expected.txt:
* LayoutTests/accessibility/mac/ruby-hierarchy-roles.html:
Remove an else block from this test that only existed due to a useless
anonymous renderer child.
* Source/WebCore/accessibility/AXCoreObject.h:
* Source/WebCore/accessibility/AXLogger.cpp:
(WebCore::operator<<):
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::createObjectFromRenderer):
(WebCore::AXObjectCache::getOrCreate):
(WebCore::AXObjectCache::handleChildrenChanged):
(WebCore::AXObjectCache::onStyleChange):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityMathMLElement.cpp:
(WebCore::AccessibilityMathMLElement::addChildren):
* Source/WebCore/accessibility/AccessibilityMathMLElement.h:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::parentObject const):
(WebCore::AccessibilityNodeObject::addChildren):
(WebCore::shouldUseAccessibilityObjectInnerText):
(WebCore::AccessibilityNodeObject::parentObjectIfExists const): Deleted.
* Source/WebCore/accessibility/AccessibilityNodeObject.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::insertChild):
* Source/WebCore/accessibility/AccessibilityObject.h:
(WebCore::AccessibilityObject::parentInCoreTree const):
(WebCore::AccessibilityObject::parentObjectIfExists const): Deleted.
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::parentObject const):
(WebCore::AccessibilityRenderObject::textUnderElement const):
(WebCore::AccessibilityRenderObject::shouldGetTextFromNode const):
(WebCore::AccessibilityRenderObject::boundingBoxRect const):
(WebCore::AccessibilityRenderObject::determineAccessibilityRole):
(WebCore::AccessibilityRenderObject::markerRenderer const):
(WebCore::AccessibilityRenderObject::addListItemMarker):
(WebCore::AccessibilityRenderObject::addChildren):
(WebCore::AccessibilityRenderObject::parentObjectIfExists const): Deleted.
(WebCore::AccessibilityRenderObject::addCanvasChildren): Deleted.
(WebCore::AccessibilityRenderObject::addNodeOnlyChildren): Deleted.
* Source/WebCore/accessibility/AccessibilityRenderObject.h:
* Source/WebCore/accessibility/AccessibilityScrollView.h:
* Source/WebCore/accessibility/AccessibilityTable.cpp:
(WebCore::AccessibilityTable::addChildren):
* Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
(-[WebAccessibilityObjectWrapper detailParentForSummaryObject:]):
(-[WebAccessibilityObjectWrapper accessibilitySupportsARIAExpanded]):
(-[WebAccessibilityObjectWrapper accessibilityIsExpanded]):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::updateChildren):
* Source/WebCore/style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::resolveComposedTree):

Canonical link: https://commits.webkit.org/285807@main

59aa64c

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@twilco twilco self-assigned this Oct 18, 2024
@twilco twilco added the Accessibility For bugs related to accessibility. label Oct 18, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 18, 2024
@twilco twilco force-pushed the eng/AX-Default-to-walking-the-DOM-rather-than-the-render-tree-when-building-the-accessibility-tree branch from cc858cf to 46aa769 Compare October 18, 2024 17:06
@twilco twilco requested a review from a team as a code owner October 18, 2024 17:06
@twilco twilco force-pushed the eng/AX-Default-to-walking-the-DOM-rather-than-the-render-tree-when-building-the-accessibility-tree branch from 46aa769 to 75fcf0e Compare October 25, 2024 01:56
@twilco twilco force-pushed the eng/AX-Default-to-walking-the-DOM-rather-than-the-render-tree-when-building-the-accessibility-tree branch from 75fcf0e to 1fa6d88 Compare October 25, 2024 01:57
@twilco twilco force-pushed the eng/AX-Default-to-walking-the-DOM-rather-than-the-render-tree-when-building-the-accessibility-tree branch from 1fa6d88 to 59aa64c Compare October 26, 2024 00:01
@@ -18,7 +18,7 @@

if (window.accessibilityController) {
var listItem1 = accessibilityController.accessibleElementById("item1");
var p1 = listItem1.childAtIndex(0);
var p1 = listItem1.childAtIndex(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this PR, we sometimes used to insert list markers as not the first child of list items, which is wrong. I fixed this bug as a side effect of this change, and thus the <p> is at index 1 (as it should be) rather than index 0.

Comment on lines -44 to -49
else {
axRuby = axRuby.childAtIndex(0);
logId(axRuby);
output += expect("axRuby.role", "expectedRubyRole");
output += expect("axRuby.subrole", "expectedRubyInlineSubrole");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch only existed because we had to deal with anonymous renderers.

@@ -856,7 +874,8 @@ AccessibilityObject* AXObjectCache::getOrCreate(Node& node, IsPartOfRelation isP
if (auto* renderer = node.renderer())
return getOrCreate(*renderer);

if (!node.parentElement())
RefPtr composedParent = node.parentElementInComposedTree();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this a reference here instead of dereferencing later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, unfortunately, since this can be null. We could turn it into a Ref after null-checking, but there's not much difference between a Ref and a RefPtr that has been checked for null. Both hold a strong reference count and thus guarantee the pointed to object is valid for as long as the scope. No ergonomic difference either — both require usage of -> to get at the underlying value.

Copy link
Contributor

@fleizach fleizach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only minor comments

@twilco twilco added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Oct 28, 2024
…ing the accessibility tree

https://bugs.webkit.org/show_bug.cgi?id=281749
rdar://138184652

Reviewed by Chris Fleizach.

Walking the DOM rather than the render tree allows many simplifications to our code. Before this
patch, we had several display:contents special cases scattered throughout the code that are no longer
needed:

  1. AccessibilityRenderObject::addNodeOnlyChildren() added display:contents children to the right
     place in an object's render-tree children retroactively. This function is now unnecessary —
     we get this property for free by walking the DOM to build the AX tree.

  2. AccessibilityObject::insertChild had a special case to prevent double-insertion of display:contents children.

  3. AccessibilityRenderObject::addCanvasChildren() only existed because canvas descendants are not rendered,
     and thus not picked up by a render tree walk. This function is now removed.

  4. Numerous other more minor checks throughout the code that only existed because we had to deal with
     anonymous renderers are removed.

The flip side of this is that we don't get "interesting" anonymous renderers for free anymore, e.g.
list markers, CSS ::before and CSS ::after, mfenced element `open`, `close`, `separators` anonymous
renderers. This is still a great tradeoff, as there are far more uninteresting anonymous renderers
than there are interesting ones.

I tried to apply this change to USE(ATSPI) ports, but there are GTK tests that expect the presence of certain
anonymous renderers that I'm not sure we can outright remove (e.g. those in accessibility/gtk/replaced-objects-in-anonymous-blocks.html).
I filed a follow-up bug (https://bugs.webkit.org/show_bug.cgi?id=282117) to track this, more details are there.

Another important aspect of this change is the addition of AXObjectCache::onStyleChange. We have accessibility
properties cached based on element styles that never get updated, and the addition of this function gives us the
framework for starting to fix those.

* LayoutTests/accessibility/mac/ruby-hierarchy-roles-expected.txt:
* LayoutTests/accessibility/mac/ruby-hierarchy-roles.html:
Remove an else block from this test that only existed due to a useless
anonymous renderer child.
* Source/WebCore/accessibility/AXCoreObject.h:
* Source/WebCore/accessibility/AXLogger.cpp:
(WebCore::operator<<):
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::createObjectFromRenderer):
(WebCore::AXObjectCache::getOrCreate):
(WebCore::AXObjectCache::handleChildrenChanged):
(WebCore::AXObjectCache::onStyleChange):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityMathMLElement.cpp:
(WebCore::AccessibilityMathMLElement::addChildren):
* Source/WebCore/accessibility/AccessibilityMathMLElement.h:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::parentObject const):
(WebCore::AccessibilityNodeObject::addChildren):
(WebCore::shouldUseAccessibilityObjectInnerText):
(WebCore::AccessibilityNodeObject::parentObjectIfExists const): Deleted.
* Source/WebCore/accessibility/AccessibilityNodeObject.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::insertChild):
* Source/WebCore/accessibility/AccessibilityObject.h:
(WebCore::AccessibilityObject::parentInCoreTree const):
(WebCore::AccessibilityObject::parentObjectIfExists const): Deleted.
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::parentObject const):
(WebCore::AccessibilityRenderObject::textUnderElement const):
(WebCore::AccessibilityRenderObject::shouldGetTextFromNode const):
(WebCore::AccessibilityRenderObject::boundingBoxRect const):
(WebCore::AccessibilityRenderObject::determineAccessibilityRole):
(WebCore::AccessibilityRenderObject::markerRenderer const):
(WebCore::AccessibilityRenderObject::addListItemMarker):
(WebCore::AccessibilityRenderObject::addChildren):
(WebCore::AccessibilityRenderObject::parentObjectIfExists const): Deleted.
(WebCore::AccessibilityRenderObject::addCanvasChildren): Deleted.
(WebCore::AccessibilityRenderObject::addNodeOnlyChildren): Deleted.
* Source/WebCore/accessibility/AccessibilityRenderObject.h:
* Source/WebCore/accessibility/AccessibilityScrollView.h:
* Source/WebCore/accessibility/AccessibilityTable.cpp:
(WebCore::AccessibilityTable::addChildren):
* Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
(-[WebAccessibilityObjectWrapper detailParentForSummaryObject:]):
(-[WebAccessibilityObjectWrapper accessibilitySupportsARIAExpanded]):
(-[WebAccessibilityObjectWrapper accessibilityIsExpanded]):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::updateChildren):
* Source/WebCore/style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::resolveComposedTree):

Canonical link: https://commits.webkit.org/285807@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/AX-Default-to-walking-the-DOM-rather-than-the-render-tree-when-building-the-accessibility-tree branch from 59aa64c to 6a44b38 Compare October 29, 2024 02:25
@webkit-commit-queue
Copy link
Collaborator

Committed 285807@main (6a44b38): https://commits.webkit.org/285807@main

Reviewed commits have been landed. Closing PR #35430 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 6a44b38 into WebKit:main Oct 29, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility For bugs related to accessibility.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants