Bug 267957 – AX: should be able to compute AXPosition using the isolated tree

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

Right now WebKit can handle AXRelativeFrame using the isolated tree, but AXPosition hits the main thread. VoiceOver sometimes queries AXPosition, so it'd be nice to make it fast.

The relative frame that we're caching in the isolated tree is already in the correct screen coordinates, all that's necessary is to cache the screen relative position of the root, and then add them together.

I propose updating the cached root position when AXGeometryManager updates object regions.

[Radar WebKit Bug Importer]

<rdar://problem/121472048>

[Dominic Mazzoni]

Created attachment 469518 [details]
Patch

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #2)
> Created attachment 469518 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXGeometryManager.cpp b/Source/WebCore/accessibility/AXGeometryManager.cpp
index 02c21ce87708..96e9da29c883 100644
--- a/Source/WebCore/accessibility/AXGeometryManager.cpp
+++ b/Source/WebCore/accessibility/AXGeometryManager.cpp
@@ -112,6 +112,12 @@ void AXGeometryManager::willUpdateObjectRegions()
 {
     if (m_updateObjectRegionsTimer.isActive())
         m_updateObjectRegionsTimer.stop();
+
+    if (!m_cache)
+        return;
+
+    if (RefPtr tree = AXIsolatedTree::treeForPageID(*m_cache->pageID()))

AG: use the overload of treeForPageID that takes an optional so you can write treeForPageID(m_cache->pageID()), otherwise you would have to check whether m_cache->pageID() returns nullopt.

+        tree->updateRootScreenRelativePosition();
 }

 void AXGeometryManager::scheduleRenderingUpdate()
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 013052e480db..9048f367cf22 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -1211,6 +1211,13 @@ FloatPoint AXIsolatedObject::screenRelativePosition() const
     if (auto point = optionalAttributeValue<FloatPoint>(AXPropertyName::ScreenRelativePosition))
         return *point;

+    auto rootPoint = tree()->rootNode()->optionalAttributeValue<FloatPoint>(AXPropertyName::ScreenRelativePosition);

AG: rootNode() could be null, shouldn't happen but it could.

+    if (rootPoint.has_value()) {
+        // Relative frames are top-left origin, but screen relative positions are bottom-left origin.
+        FloatRect rootRelativeFrame = tree()->rootNode()->relativeFrame();
+        return FloatPoint(rootPoint->x() + relativeFrame().location().x(), rootPoint->y() + (rootRelativeFrame.maxY() - relativeFrame().maxY()));
+    }

AG: you can write:
        return { rootPoint->x() + relativeFrame().location().x(), rootPoint->y() + (rootRelativeFrame.maxY() - relativeFrame().maxY()) };

+
     return Accessibility::retrieveValueFromMainThread<FloatPoint>([&, this] () -> FloatPoint {
         if (auto* axObject = associatedAXObject())
             return axObject->screenRelativePosition();
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 4a28591c3447..408e72d1ca8c 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -1012,6 +1012,22 @@ void AXIsolatedTree::updateFrame(AXID axID, IntRect&& newFrame)
     m_pendingPropertyChanges.append({ axID, WTFMove(propertyMap) });
 }

+void AXIsolatedTree::updateRootScreenRelativePosition()
+{
+    AXTRACE("AXIsolatedTree::updateRootScreenRelativePosition"_s);
+    ASSERT(isMainThread());
+
+    auto* rootAXObject = rootNode()->associatedAXObject();

AG: we tend to use axRoot for the live object and just root for the iso object in iso code. Same with axObject vs. object.

+    if (!rootAXObject)
+        return;
+
+    AXPropertyMap propertyMap;
+    FloatPoint screenRelativePosition = rootAXObject->screenRelativePosition();
+    propertyMap.set(AXPropertyName::ScreenRelativePosition, WTFMove(screenRelativePosition));

AG: you can do the above two lines in one as:
    propertyMap.set(AXPropertyName::ScreenRelativePosition, rootAXObject->screenRelativePosition());

+    Locker locker { m_changeLogLock };
+    m_pendingPropertyChanges.append({ rootAXObject->objectID(), WTFMove(propertyMap) });

AG: can we add this property to AXIsolatedTree::updateNodeProperties if it is not already there and call it here instead? As opposed to creating a property map, acquiring the Lock... which is what updateNodeProperties does.

+}
+
 void AXIsolatedTree::removeNode(const AccessibilityObject& axObject)
 {
     AXTRACE("AXIsolatedTree::removeNode"_s);

[Dominic Mazzoni]

Thanks for the great feedback! All should be addressed.

[Dominic Mazzoni]

Created attachment 469560 [details]
Patch

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #5)
> Created attachment 469560 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 013052e480db..6f312640b8ae 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -1211,6 +1211,14 @@ FloatPoint AXIsolatedObject::screenRelativePosition() const
     if (auto point = optionalAttributeValue<FloatPoint>(AXPropertyName::ScreenRelativePosition))
         return *point;

+    if (auto rootNode = tree()->rootNode()) {
+        if (auto rootPoint = rootNode->optionalAttributeValue<FloatPoint>(AXPropertyName::ScreenRelativePosition)) {
+            // Relative frames are top-left origin, but screen relative positions are bottom-left origin.
+            FloatRect rootRelativeFrame = tree()->rootNode()->relativeFrame();

AG: no need to do tree()->rootNode() again since you already have a local variable rootNode, i.e., should be:
            FloatRect rootRelativeFrame = rootNode->relativeFrame();

+            return { rootPoint->x() + relativeFrame().location().x(), rootPoint->y() + (rootRelativeFrame.maxY() - relativeFrame().maxY()) };

AG: should we avoid calling relativeFrame() twice here since it can be costly, in some cases hitting the main thread or walking the children?

+        }
+    }
+
     return Accessibility::retrieveValueFromMainThread<FloatPoint>([&, this] () -> FloatPoint {
         if (auto* axObject = associatedAXObject())
             return axObject->screenRelativePosition();
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 4a28591c3447..0adb19b9c46f 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -640,6 +640,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const AXProper
         case AXPropertyName::CellScope:
             propertyMap.set(AXPropertyName::CellScope, axObject.cellScope().isolatedCopy());
             break;
+        case AXPropertyName::ScreenRelativePosition:
+            propertyMap.set(AXPropertyName::ScreenRelativePosition, axObject.screenRelativePosition());
+            break;
         case AXPropertyName::SelectedTextRange:
             propertyMap.set(AXPropertyName::SelectedTextRange, axObject.selectedTextRange());
             break;
@@ -1012,6 +1015,15 @@ void AXIsolatedTree::updateFrame(AXID axID, IntRect&& newFrame)
     m_pendingPropertyChanges.append({ axID, WTFMove(propertyMap) });
 }

+void AXIsolatedTree::updateRootScreenRelativePosition()
+{
+    AXTRACE("AXIsolatedTree::updateRootScreenRelativePosition"_s);
+    ASSERT(isMainThread());
+
+    if (auto* axRoot = rootNode()->associatedAXObject())

AG: should we check rootNode() for null here?

+        updateNodeProperties(*axRoot, { AXPropertyName::ScreenRelativePosition });
+}
+
 void AXIsolatedTree::removeNode(const AccessibilityObject& axObject)
 {
     AXTRACE("AXIsolatedTree::removeNode"_s);

[Dominic Mazzoni]

Created attachment 469600 [details]
Patch

[Dominic Mazzoni]

Thanks. I believe the compiler is technically allowed to optimize the two calls to relativeFrame since it's a const function, but that's definitely not a guarantee, so I agree it's better to just put it in a local.

[Andres Gonzalez]

(In reply to Dominic Mazzoni from comment #7)
> Created attachment 469600 [details]
> Patch

+            FloatRect objRelativeFrame = relativeFrame();

AG: minor nit, WebKit coding guidelines don't like partial words, so I'd suggest:
FloatRect relativeFrame = this->relativeFrame();

[Dominic Mazzoni]

Created attachment 469603 [details]
Patch

[EWS]

Committed 273716@main (551f6e680f88): <https://commits.webkit.org/273716@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 469603 [details].