This will allow us to serve geometry to AX clients off the main-thread, thereby increasing responsiveness.
<rdar://problem/108756434>
Created attachment 466161 [details] Patch
Comment on attachment 466161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466161&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:842 > + scheduleAccessibiltyObjectRegionsUpdate(); scheduleAccessibiltyObjectRegionsUpdate > (missing an i) scheduleAccessibilityObjectRegionsUpdate > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:180 > + setRelativeFrame(IntRect()); is this the default for relative frame? aka) if we didn't do this, wouldn't the result be the same? > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1098 > + } else if (roleValue() == AccessibilityRole::Column || roleValue() == AccessibilityRole::TableHeaderContainer) should we combine these if conditions into the first if. it seems that the code block is the same in both of these blocks (more or less) or should we set m_getsGeometryFromChildren = tru when it matches these roles or hasExposed... > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1112 > + if (child) can we really not have an object here? what is stored in the array if the element is null? > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1113 > + computedRect.unite(child->relativeFrame()); also we may be able to write this in one line computedRect.unite(child ? child->relativeFrame() : computedRect);
Created attachment 466190 [details] Patch
(In reply to chris fleizach from comment #3) > Comment on attachment 466161 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466161&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:842 > > + scheduleAccessibiltyObjectRegionsUpdate(); > > scheduleAccessibiltyObjectRegionsUpdate > > > (missing an i) > > scheduleAccessibilityObjectRegionsUpdate Fixed this typo and rebased the patch to pick up https://bugs.webkit.org/show_bug.cgi?id=256218 (AX: Allow 1px difference in results for mac/bounds-for-range.html), will review your other comments more tomorrow.
Created attachment 466191 [details] Patch
(In reply to Tyler Wilcock from comment #6) > Created attachment 466191 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp I think this addition belongs to the AXIsoaltedTree class better than to the AXObjectCache. It could even be its own separate class and have the AXIsolatedTree to instantiate it. + // Schedule a paint to cache the rects for the objects in this new isolated tree + scheduleAccessibilityObjectRegionsUpdate(); Shouldn't this be done in AXIsolatedTree::create? +void AXObjectCache::onPaint(const RenderObject& renderer, IntRect&& paintRect) +{ + if (!shouldHandlePaint()) + return; + + auto axID = m_renderObjectMapping.get(const_cast<RenderObject*>(&renderer)); + onPaint(WTFMove(axID), WTFMove(paintRect)); +} Do you need to move an AXID? Don't do it any where else, I think it is not needed because it is equivalent to a built-in numeric type. +void AXObjectCache::onPaint(const Widget& widget, IntRect&& paintRect) +{ + if (!shouldHandlePaint()) + return; + + auto axID = m_widgetObjectMapping.get(const_cast<Widget*>(&widget)); + onPaint(WTFMove(axID), WTFMove(paintRect)); +} You can probably fuse these two methods into one using a switchOn with a variant like DOMObjectVariant. +void AXObjectCache::scheduleAccessibilityObjectRegionsUpdate() Do we need Accessibility as part of this method name? ' +{ + if (m_updateObjectRectsTimer.isActive() || !document().isTopDocument()) Why we ned the check for isTopDocument(), AXObjectCaches are created only for top documents, or they should. +void AXObjectCache::willUpdateAccessibilityObjectRegions() Accessibility? AXObjectCahce is already all about accessibility. At a first glance, it is confusing that all what willUpdateAccessibilityObjectRegions does is to stop the timer. willUpdate suggests that something is about to happen, not that it is stopping. --- a/Source/WebCore/accessibility/AXObjectCache.h +++ b/Source/WebCore/accessibility/AXObjectCache.h + std::optional<IntRect> paintRectByID(const AXID&); Do we need to pass AXIDs by ref? + void onPaint(AXID&&, IntRect&&); AXID&& ? --- a/Source/WebCore/accessibility/AccessibilityScrollView.cpp +++ b/Source/WebCore/accessibility/AccessibilityScrollView.cpp +LayoutRect AccessibilityScrollView::elementRect() const +{ + auto* scrollView = currentScrollView(); + if (!scrollView) + return LayoutRect(); return { }; --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp + if (std::optional frame = axObjectCache()->paintRectByID(object.objectID())) { + // Get the frame computed by paint. + setRelativeFrame(WTFMove(*frame)); This comment seems redundant here, maybe move it above the if statement and say what the whole if else if... is doing. + } else if (object.isMenuListPopup()) { + // AccessibilityMenuListPopup's elementRect is hardcoded to return an empty rect, so preserve that behavior. + setRelativeFrame(IntRect()); IntRect() -> { } +bool AXIsolatedObject::hasExposedTableAncestor() const +{ + return Accessibility::findAncestor<AXIsolatedObject>(*this, false, [] (const AXIsolatedObject& object) { + return object.isTable() && object.isExposable(); + }); +} FloatRect AXIsolatedObject::relativeFrame() const { ... + return Accessibility::retrieveValueFromMainThread<FloatRect>([&, this] () -> FloatRect { don't need to capture &. +FloatRect AXIsolatedObject::relativeFrameFromChildren() const +{ + FloatRect computedRect; just rect would suffice. + for (const auto& child : const_cast<AXIsolatedObject*>(this)->children()) { Do we want to update the children here? or we should children(false). + if (child) + computedRect.unite(child->relativeFrame()); Is it needed to check child for null? --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h + bool getsGeometryFromChildren() { return m_getsGeometryFromChildren; } not private? + bool hasExposedTableAncestor() const; Why we need this? If so, maybe we can have a common impl for both isolated and live objects? + // std::nullopt means a frame has not yet been cached for this object. + std::optional<IntRect> m_relativeFrame; Can we have this in the property map instead? --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +void AXIsolatedTree::updateFrame(AXID&& axID, IntRect&& newFrame) Can we update this just as any oother property in updateNodeProperty()? --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h + void updateFrame(AXID&&, IntRect&&); Do we need a separate method or we can use updateNodeProeprty? --- a/Source/WebCore/page/Page.cpp +++ b/Source/WebCore/page/Page.cpp +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + m_renderingUpdateRemainingSteps.last().remove(RenderingUpdateStep::AccessibilityRegionUpdate); + if (shouldUpdateAccessibilityRegions()) { + m_lastAccessibilityObjectRegionsUpdate = m_lastRenderingUpdateTimestamp; + forEachDocument([] (Document& document) { + document.updateAccessibilityObjectRegions(); Mayebe here is where we need to check whether it is the top document? --- a/Source/WebCore/rendering/AccessibilityRegionContext.cpp +++ b/Source/WebCore/rendering/AccessibilityRegionContext.cpp +#include "AccessibilityScrollView.h" + // AccessibilityScrollView::elementRectFromScrollView *must* remain cheap to call, because this method can be called a lot. + auto relativeFrame = enclosingIntRect(AccessibilityScrollView::elementRectFromScrollView(*frameView)); If it is used here, I don't think that method should be in AccessibilityScrollView at all. This shouldn't need to know about AccessibilityScrollView.
Created attachment 466204 [details] Patch
(In reply to Andres Gonzalez from comment #7) > (In reply to Tyler Wilcock from comment #6) > > Created attachment 466191 [details] > > Patch > > --- a/Source/WebCore/accessibility/AXObjectCache.cpp > +++ b/Source/WebCore/accessibility/AXObjectCache.cpp > > I think this addition belongs to the AXIsoaltedTree class better than to the > AXObjectCache. It could even be its own separate class and have the > AXIsolatedTree to instantiate it. It's true that as of this patch all of this painting is only ever driven if isolated tree mode is enabled. But that may not be the case forever, because getting geometry this way is both significantly faster than the existing mechanism, and generally more correct (with few insignificant exceptions). I have at least one bug assigned to me where WebKit is hung in an ITM-disabled config with 99% of the samples computing bounding box rects (rdar://107397016). These paint rects are tighter around buttons than our current frame computation mechanism. And before this patch, frames are totally broken on Twitter with ITM on or off (every element looks like it has infinite height), and after this patch everything looks correct -- it's a big improvement. So long story short, I'm not sure that we'll want this to forever be ITM only. That being said, I want to consider your idea of abstracting this logic into a separate class more deeply tomorrow, regardless of whether that class is owned by AXObjectCache or AXIsolatedTree. I'll do some experimenting. > + // Schedule a paint to cache the rects for the objects in this new > isolated tree > + scheduleAccessibilityObjectRegionsUpdate(); > > Shouldn't this be done in AXIsolatedTree::create? I think the answer to this depends on what owns this AXGeometryProvider / AXGeometryUpdater / whatever class. Will consider this as I experiment. > Do you need to move an AXID? Don't do it any where else, I think it is not > needed because it is equivalent to a built-in numeric type. Fixed this and the other AXID comments. > +void AXObjectCache::onPaint(const Widget& widget, IntRect&& paintRect) > +{ > + if (!shouldHandlePaint()) > + return; > + > + auto axID = m_widgetObjectMapping.get(const_cast<Widget*>(&widget)); > + onPaint(WTFMove(axID), WTFMove(paintRect)); > +} > > You can probably fuse these two methods into one using a switchOn with a > variant like DOMObjectVariant. I tried this, but it ended up being more lines of code, and more clunky since std::variant cannot (easily) hold references. I think this suggestion would make sense with three variants, but with the two we have I think leaving as-is is better. > +void AXObjectCache::scheduleAccessibilityObjectRegionsUpdate() > > Do we need Accessibility as part of this method name? Fixed. > + if (m_updateObjectRectsTimer.isActive() || !document().isTopDocument()) > > Why we ned the check for isTopDocument(), AXObjectCaches are created only > for top documents, or they should. Good point! I think we can just remove the top document check. > +void AXObjectCache::willUpdateAccessibilityObjectRegions() > > Accessibility? AXObjectCahce is already all about accessibility. > At a first glance, it is confusing that all what > willUpdateAccessibilityObjectRegions does is to stop the timer. willUpdate > suggests that something is about to happen, not that it is stopping. Well, "something is about to happen" is accurate, and the action we're taking based on that is stopping the timer representing a queued paint update (because we are about to perform one, so the queued one is not neccessary). Removed "Accessibility" from the method name. > + for (const auto& child : > const_cast<AXIsolatedObject*>(this)->children()) { > > Do we want to update the children here? or we should children(false). I think we do want to update children so we're definitely returning the correct answer. This should be a pretty rare codepath, so I think it will be OK performance-wise. > + if (child) > + computedRect.unite(child->relativeFrame()); > > Is it needed to check child for null? No, removed the null check. > + bool hasExposedTableAncestor() const; > > Why we need this? If so, maybe we can have a common impl for both isolated > and live objects? We need it to return the right relative frame of AccessibilityRole::Column and AccessibilityRole::TableHeaderContainer objects (see the elementRect implementations for the associated classes). Added a new AXCoreObject::exposedTableAncestor() and also made use of it in the iOS wrapper where similar logic was in place. > + // std::nullopt means a frame has not yet been cached for this object. > + std::optional<IntRect> m_relativeFrame; > > Can we have this in the property map instead? Fixed. > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > +void AXIsolatedTree::updateFrame(AXID&& axID, IntRect&& newFrame) > > Can we update this just as any oother property in updateNodeProperty()? I ended up keeping AXIsolatedTree::updateFrame because we have the data we need at the time of the onPaint notification, so I don't think it makes sense to ping-pong from AXObjectCache::onPaint where we have the data for the update, to AXIsolatedTree::updateNodeProperty, then go back to the cache for the data we had before a few callframes ago. This story would be different if the paint information lived in AXIsolatedTree like you suggest, but we may or may not want to do that per my other comment. > + // AccessibilityScrollView::elementRectFromScrollView *must* remain > cheap to call, because this method can be called a lot. > + auto relativeFrame = > enclosingIntRect(AccessibilityScrollView:: > elementRectFromScrollView(*frameView)); > > If it is used here, I don't think that method should be in > AccessibilityScrollView at all. This shouldn't need to know about > AccessibilityScrollView. Fixed this by moving the necessary logic into ScrollView. Any other comment I didn't quote is addressed. Thanks for the review!
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:180 > > + setRelativeFrame(IntRect()); > > is this the default for relative frame? aka) if we didn't do this, wouldn't > the result be the same? It would not be the same. With this, we cache a rect of {(0, 0), (0, 0)} on the AX thread. Without doing this, optionalAttributeValue would return std::nullopt (meaning the value is not cached), causing a hit to the main-thread (which would return the same empty rect we could've just cached up-front). > > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1098 > > + } else if (roleValue() == AccessibilityRole::Column || roleValue() == AccessibilityRole::TableHeaderContainer) > > should we combine these if conditions into the first if. it seems that the > code block is the same in both of these blocks (more or less) > > or should we set m_getsGeometryFromChildren = tru when it matches these > roles or hasExposed... I don't think we can do either since the behavior is subtly different. For m_getsGeometryFromChildren, if we compute an empty frame (because there are no children, or those children have empty frames), we have to hit the main-thread for the reason described in the comment. But for Role::Column and Role::TableHeaderContainer objects, returning an empty rect when there are no children is intentional, and should not cause a main-thread hit. These two types of objects intentionally have no children if they are not within an exposed table. So special-casing them this way is the right thing both in preserving existing behavior and preventing main-thread hits. > > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1112 > > + if (child) > > can we really not have an object here? what is stored in the array if the > element is null? Removed the null check.
(In reply to Tyler Wilcock from comment #9) > (In reply to Andres Gonzalez from comment #7) > > (In reply to Tyler Wilcock from comment #6) > > > Created attachment 466191 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/AXObjectCache.cpp > > +++ b/Source/WebCore/accessibility/AXObjectCache.cpp > > > > I think this addition belongs to the AXIsoaltedTree class better than to the > > AXObjectCache. It could even be its own separate class and have the > > AXIsolatedTree to instantiate it. > It's true that as of this patch all of this painting is only ever driven if > isolated tree mode is enabled. But that may not be the case forever, because > getting geometry this way is both significantly faster than the existing > mechanism, and generally more correct (with few insignificant exceptions). I > have at least one bug assigned to me where WebKit is hung in an ITM-disabled > config with 99% of the samples computing bounding box rects > (rdar://107397016). These paint rects are tighter around buttons than our > current frame computation mechanism. And before this patch, frames are > totally broken on Twitter with ITM on or off (every element looks like it > has infinite height), and after this patch everything looks correct -- it's > a big improvement. > > So long story short, I'm not sure that we'll want this to forever be ITM > only. That being said, I want to consider your idea of abstracting this > logic into a separate class more deeply tomorrow, regardless of whether that > class is owned by AXObjectCache or AXIsolatedTree. I'll do some > experimenting. > > > + // Schedule a paint to cache the rects for the objects in this new > > isolated tree > > + scheduleAccessibilityObjectRegionsUpdate(); > > > > Shouldn't this be done in AXIsolatedTree::create? > I think the answer to this depends on what owns this AXGeometryProvider / > AXGeometryUpdater / whatever class. Will consider this as I experiment. > > > Do you need to move an AXID? Don't do it any where else, I think it is not > > needed because it is equivalent to a built-in numeric type. > Fixed this and the other AXID comments. > > > +void AXObjectCache::onPaint(const Widget& widget, IntRect&& paintRect) > > +{ > > + if (!shouldHandlePaint()) > > + return; > > + > > + auto axID = m_widgetObjectMapping.get(const_cast<Widget*>(&widget)); > > + onPaint(WTFMove(axID), WTFMove(paintRect)); > > +} > > > > You can probably fuse these two methods into one using a switchOn with a > > variant like DOMObjectVariant. > I tried this, but it ended up being more lines of code, and more clunky > since std::variant cannot (easily) hold references. I think this suggestion > would make sense with three variants, but with the two we have I think > leaving as-is is better. > > > +void AXObjectCache::scheduleAccessibilityObjectRegionsUpdate() > > > > Do we need Accessibility as part of this method name? > Fixed. > > > + if (m_updateObjectRectsTimer.isActive() || !document().isTopDocument()) > > > > Why we ned the check for isTopDocument(), AXObjectCaches are created only > > for top documents, or they should. > Good point! I think we can just remove the top document check. > > > +void AXObjectCache::willUpdateAccessibilityObjectRegions() > > > > Accessibility? AXObjectCahce is already all about accessibility. > > At a first glance, it is confusing that all what > > willUpdateAccessibilityObjectRegions does is to stop the timer. willUpdate > > suggests that something is about to happen, not that it is stopping. > Well, "something is about to happen" is accurate, and the action we're > taking based on that is stopping the timer representing a queued paint > update (because we are about to perform one, so the queued one is not > neccessary). Removed "Accessibility" from the method name. A comment here would be helpful for who is trying to understand the mechanism. > > > + for (const auto& child : > > const_cast<AXIsolatedObject*>(this)->children()) { > > > > Do we want to update the children here? or we should children(false). > I think we do want to update children so we're definitely returning the > correct answer. This should be a pretty rare codepath, so I think it will be > OK performance-wise. > > > + if (child) > > + computedRect.unite(child->relativeFrame()); > > > > Is it needed to check child for null? > No, removed the null check. > > > + bool hasExposedTableAncestor() const; > > > > Why we need this? If so, maybe we can have a common impl for both isolated > > and live objects? > We need it to return the right relative frame of AccessibilityRole::Column > and AccessibilityRole::TableHeaderContainer objects (see the elementRect > implementations for the associated classes). Added a new > AXCoreObject::exposedTableAncestor() and also made use of it in the iOS > wrapper where similar logic was in place. > > > + // std::nullopt means a frame has not yet been cached for this object. > > + std::optional<IntRect> m_relativeFrame; > > > > Can we have this in the property map instead? > Fixed. > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > > > > +void AXIsolatedTree::updateFrame(AXID&& axID, IntRect&& newFrame) > > > > Can we update this just as any oother property in updateNodeProperty()? > I ended up keeping AXIsolatedTree::updateFrame because we have the data we > need at the time of the onPaint notification, so I don't think it makes > sense to ping-pong from AXObjectCache::onPaint where we have the data for > the update, to AXIsolatedTree::updateNodeProperty, then go back to the cache > for the data we had before a few callframes ago. This story would be > different if the paint information lived in AXIsolatedTree like you suggest, > but we may or may not want to do that per my other comment. > > > + // AccessibilityScrollView::elementRectFromScrollView *must* remain > > cheap to call, because this method can be called a lot. > > + auto relativeFrame = > > enclosingIntRect(AccessibilityScrollView:: > > elementRectFromScrollView(*frameView)); > > > > If it is used here, I don't think that method should be in > > AccessibilityScrollView at all. This shouldn't need to know about > > AccessibilityScrollView. > Fixed this by moving the necessary logic into ScrollView. > > Any other comment I didn't quote is addressed. Thanks for the review! No need to R? again if you just add that comment, already R+. Thanks.
Created attachment 466219 [details] Patch
(In reply to Tyler Wilcock from comment #12) > Created attachment 466219 [details] > Patch +void AXGeometryManager::onPaint(AXID axID, IntRect&& paintRect) +{ + // We shouldn't call this method on a geometry manager that has no page ID. + ASSERT(m_cache->pageID()); +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) + ASSERT(AXObjectCache::isIsolatedTreeEnabled()); +#endif You already have a #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) for the entire file. +void AXGeometryManager::scheduleObjectRegionsUpdate() +{ + if (m_updateObjectRegionsTimer.isActive()) + return; + m_updateObjectRegionsTimer.startOneShot(1_s); +} Two lines? if (!m_updateObjectRegionsTimer.isActive()) m_updateObjectRegionsTimer.startOneShot(1_s); +void AXGeometryManager::willUpdateObjectRegions() +{ + if (m_updateObjectRegionsTimer.isActive()) + m_updateObjectRegionsTimer.stop(); +} I would add a comment like: The RenderTree (or whatever) is about to repaint the accessibility object region, so cancel any pending update and wait for the new rendition. +class AXObjectCache : public CanMakeWeakPtr<AXObjectCache>, public CanMakeCheckedPtr , public AXTreeStore<AXObjectCache> { WTF_MAKE_NONCOPYABLE(AXObjectCache); WTF_MAKE_FAST_ALLOCATED; + friend class AXGeometryManager; why needs to be a friend? + void updateObjectRegionsTimerFired(); Is this a leftover in AXObjectCache.h?
Created attachment 466227 [details] Patch
Created attachment 466230 [details] Patch
(In reply to Andres Gonzalez from comment #13) Addressed all comments, thanks!
Created attachment 466232 [details] Patch
Created attachment 466249 [details] Patch
Created attachment 466250 [details] Patch
Created attachment 466252 [details] Patch
Committed 263771@main (8884fab24279): <https://commits.webkit.org/263771@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466252 [details].
Re-opening for pull request https://github.com/WebKit/WebKit/pull/13552
Committed 263779@main (bd9cb7bd2577): <https://commits.webkit.org/263779@main> Reviewed commits have been landed. Closing PR #13552 and removing active labels.