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.
<rdar://problem/121472048>
Created attachment 469518 [details] Patch
(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);
Thanks for the great feedback! All should be addressed.
Created attachment 469560 [details] Patch
(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);
Created attachment 469600 [details] Patch
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.
(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();
Created attachment 469603 [details] Patch
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].