Changing focus can be expensive on pages with complicated style. Waiting to commit focus until the user has settled on an element would be a performance improvement.
<rdar://problem/117217888>
Created attachment 468277 [details] Patch
Comment on attachment 468277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468277&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2676 > +#if PLATFORM(MAC) feel like this is appropriate for COCOA and get iOS as well
(In reply to chris fleizach from comment #3) > Comment on attachment 468277 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=468277&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2676 > > +#if PLATFORM(MAC) > > feel like this is appropriate for COCOA and get iOS as well Hmm, yeah I limited it to macOS because my thought process was that iOS VoiceOver swiping is much less likely to trigger multiple-focus events within the (current) 200ms debounce threshold. But now that I type this, explore-by-touch interaction would probably benefit from this change. My only hesitation is that we probably want to limit this behavior to certain ATs, as presumably immediate focus syncing is important for some, like Full Keyboard Access. I guess we could use: https://developer.apple.com/documentation/uikit/1615187-uiaccessibilityisvoiceoverrunnin?language=objc To determine if VoiceOver is running, but I don't see an equivalent for FKA, and I know it's common for users to combine these. Should we debounce anyways? What do you think?
Comment on attachment 468277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468277&action=review >>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2676 >>> +#if PLATFORM(MAC) >> >> feel like this is appropriate for COCOA and get iOS as well > > Hmm, yeah I limited it to macOS because my thought process was that iOS VoiceOver swiping is much less likely to trigger multiple-focus events within the (current) 200ms debounce threshold. But now that I type this, explore-by-touch interaction would probably benefit from this change. > > My only hesitation is that we probably want to limit this behavior to certain ATs, as presumably immediate focus syncing is important for some, like Full Keyboard Access. I guess we could use: > > https://developer.apple.com/documentation/uikit/1615187-uiaccessibilityisvoiceoverrunnin?language=objc > > To determine if VoiceOver is running, but I don't see an equivalent for FKA, and I know it's common for users to combine these. Should we debounce anyways? What do you think? I think we can check clientType() here?
Created attachment 468292 [details] Patch
Created attachment 468293 [details] Patch
(In reply to Tyler Wilcock from comment #7) > Created attachment 468293 [details] > Patch diff --git a/Source/WebCore/accessibility/AXCoreObject.h b/Source/WebCore/accessibility/AXCoreObject.h index 991b3a086cbf..7b86b5bc92aa 100644 --- a/Source/WebCore/accessibility/AXCoreObject.h +++ b/Source/WebCore/accessibility/AXCoreObject.h @@ -1221,7 +1221,8 @@ public: virtual unsigned hierarchicalLevel() const = 0; virtual bool isInlineText() const = 0; - virtual void setFocused(bool) = 0; + enum class ShouldDebounce : bool { No, Yes }; + virtual void setFocused(bool, ShouldDebounce = ShouldDebounce::No) = 0; virtual void setSelectedText(const String&) = 0; virtual void setSelectedTextRange(CharacterRange&&) = 0; virtual bool setValue(const String&) = 0; @@ -1440,6 +1441,8 @@ public: virtual Vector<RetainPtr<id>> modelElementChildren() = 0; #endif + virtual bool forceFocusDebouncing() const = 0; AG: why we need this in the AXCoreObject interface? This is not an AX object property, but a focus handling mode. Why do we need to force vs. not forcing this mode? + protected: AXCoreObject() = default; explicit AXCoreObject(AXID axID) diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp index 90148f101879..ba2bd607f9a1 100644 --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp @@ -124,6 +124,7 @@ #include <utility> #include <wtf/DataLog.h> #include <wtf/NeverDestroyed.h> +#include <wtf/Scope.h> #include <wtf/SetForScope.h> #include <wtf/text/AtomString.h> @@ -207,6 +208,7 @@ void AccessibilityReplacedText::postTextStateChangeNotification(AXObjectCache* c bool AXObjectCache::gAccessibilityEnabled = false; bool AXObjectCache::gAccessibilityEnhancedUserInterfaceEnabled = false; bool AXObjectCache::gForceDeferredSpellChecking = false; +bool AXObjectCache::gForceFocusDebouncing = false; void AXObjectCache::enableAccessibility() { @@ -224,6 +226,11 @@ void AXObjectCache::setForceDeferredSpellChecking(bool shouldForce) gForceDeferredSpellChecking = shouldForce; } +void AXObjectCache::setForceFocusDebouncing(bool shouldForce) +{ + gForceFocusDebouncing = shouldForce; +} + void AXObjectCache::setEnhancedUserInterfaceAccessibility(bool flag) { gAccessibilityEnhancedUserInterfaceEnabled = flag; @@ -245,6 +252,9 @@ AXObjectCache::AXObjectCache(Document& document) , m_buildIsolatedTreeTimer(*this, &AXObjectCache::buildIsolatedTree) , m_geometryManager(AXGeometryManager::create(*this)) #endif +#if PLATFORM(COCOA) + , m_focusDebounceTimer(*this, &AXObjectCache::focusDebounceTimerFired, platformFocusDebounceInterval()) +#endif { AXTRACE(makeString("AXObjectCache::AXObjectCache 0x"_s, hex(reinterpret_cast<uintptr_t>(this)))); #ifndef NDEBUG @@ -274,6 +284,9 @@ AXObjectCache::~AXObjectCache() m_notificationPostTimer.stop(); m_liveRegionChangedPostTimer.stop(); m_performCacheUpdateTimer.stop(); +#if PLATFORM(COCOA) + m_focusDebounceTimer.stop(); +#endif for (const auto& object : m_objects.values()) object->detach(AccessibilityDetachmentType::CacheDestroyed); @@ -515,6 +528,37 @@ AccessibilityObject* AXObjectCache::focusedObjectForNode(Node* focusedNode) return focus; } +#if PLATFORM(COCOA) +void AXObjectCache::requestFocus(AccessibilityNodeObject* newFocus, Document& document) +{ + m_focusDebounceTimer.restart(); + m_pendingFocus = PendingFocus(newFocus, document); +} + +void AXObjectCache::focusDebounceTimerFired() +{ + m_focusDebounceTimer.stop(); + auto clearPendingFocus = makeScopeExit([&] { + m_pendingFocus = std::nullopt; + }); + + if (!m_pendingFocus || !m_pendingFocus->document) + return; + + if (m_pendingFocus->document->focusedElement() != m_pendingFocus->oldFocus) { + // The focused element has changed since we queued our pending focus. Since that + // focus came later in time, we shouldn't overwrite it. + return; + } + + if (!m_pendingFocus->candidate) { + m_pendingFocus->document->setFocusedElement(nullptr); + return; + } + m_pendingFocus->candidate->setFocused(true); +} +#endif // PLATFORM(COCOA) + #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) void AXObjectCache::setIsolatedTreeFocusedObject(AccessibilityObject* focus) { diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h index 498e7a09fe2e..ffc273fb049b 100644 --- a/Source/WebCore/accessibility/AXObjectCache.h +++ b/Source/WebCore/accessibility/AXObjectCache.h @@ -48,6 +48,7 @@ class TextStream; namespace WebCore { +class AccessibilityNodeObject; class AccessibilityTable; class AccessibilityTableCell; class Document; @@ -256,6 +257,13 @@ public: WEBCORE_EXPORT static void disableAccessibility(); static bool forceDeferredSpellChecking() { return gForceDeferredSpellChecking; } WEBCORE_EXPORT static void setForceDeferredSpellChecking(bool); + static bool forceFocusDebouncing() { return gForceFocusDebouncing; } + WEBCORE_EXPORT static void setForceFocusDebouncing(bool); + +#if PLATFORM(COCOA) + void requestFocus(AccessibilityNodeObject*, Document&); +#endif + #if PLATFORM(MAC) static bool shouldSpellCheck(); #else @@ -567,7 +575,10 @@ private: void liveRegionChangedNotificationPostTimerFired(); void focusCurrentModal(); - +#if PLATFORM(COCOA) + void focusDebounceTimerFired(); + Seconds platformFocusDebounceInterval() const; +#endif void performCacheUpdateTimerFired(); void postTextStateChangeNotification(AccessibilityObject*, const AXTextStateChangeIntent&, const VisibleSelection&); @@ -647,10 +658,12 @@ private: WEBCORE_EXPORT static bool gAccessibilityEnabled; WEBCORE_EXPORT static bool gAccessibilityEnhancedUserInterfaceEnabled; static bool gForceDeferredSpellChecking; + static bool gForceFocusDebouncing; #else static constexpr bool gAccessibilityEnabled { false }; static constexpr bool gAccessibilityEnhancedUserInterfaceEnabled { false }; static constexpr bool gForceDeferredSpellChecking { false }; + static constexpr bool gForceFocusDebouncing { false }; #endif HashSet<AXID> m_idsInUse; @@ -706,6 +719,27 @@ private: bool m_relationsNeedUpdate { true }; HashSet<AXID> m_relationTargets; +#if PLATFORM(COCOA) + struct PendingFocus { + // nullptr if the document's focus should become null. + RefPtr<AccessibilityObject> candidate; + // Document to focus into. + WeakPtr<Document, WeakPtrImplWithEventTargetData> document; + // The focused element of |document| when this struct was created. + // Do not de-reference, only use for pointer comparison. + Element* oldFocus; + + PendingFocus(AccessibilityObject* focusCandidate, Document& focusDocument) + : candidate(focusCandidate) + , document(focusDocument) + , oldFocus(focusDocument.focusedElement()) + { } + }; + // std::nullopt if there is no pending focus. + std::optional<PendingFocus> m_pendingFocus { std::nullopt }; + DeferrableOneShotTimer m_focusDebounceTimer; +#endif // PLATFORM(COCOA) + #if USE(ATSPI) ListHashSet<RefPtr<AXCoreObject>> m_deferredParentChangedList; #endif @@ -749,6 +783,7 @@ inline AccessibilityObject* AXObjectCache::rootObjectForFrame(LocalFrame*) { ret inline AccessibilityObject* AXObjectCache::focusedObjectForPage(const Page*) { return nullptr; } inline void AXObjectCache::enableAccessibility() { } inline void AXObjectCache::setForceDeferredSpellChecking(bool) { } +inline void AXObjectCache::setForceFocusDebouncing(bool) { } inline void AXObjectCache::disableAccessibility() { } inline void AXObjectCache::setEnhancedUserInterfaceAccessibility(bool) { } inline bool nodeHasRole(Node*, StringView) { return false; } diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp index 09bf0e3ca2a3..7dfc5a4ca26b 100644 --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp @@ -2671,8 +2671,21 @@ bool AccessibilityNodeObject::isFocused() const return frame ? frame->selection().isFocusedAndActive() : false; } -void AccessibilityNodeObject::setFocused(bool on) +void AccessibilityNodeObject::setFocused(bool on, ShouldDebounce shouldDebounce) { +#if PLATFORM(COCOA) + if (shouldDebounce == ShouldDebounce::Yes) { + if (auto* cache = axObjectCache()) { + if (auto* document = this->document()) { + cache->requestFocus(on ? this : nullptr, *document); + return; + } + } + } +#else + UNUSED_PARAM(shouldDebounce); +#endif // PLATFORM(COCOA) + // Call the base class setFocused to ensure the view is focused and active. AccessibilityObject::setFocused(on); diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.h b/Source/WebCore/accessibility/AccessibilityNodeObject.h index 08da84fbf852..34597e3916f6 100644 --- a/Source/WebCore/accessibility/AccessibilityNodeObject.h +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.h @@ -85,7 +85,7 @@ public: Document* document() const override; LocalFrameView* documentFrameView() const override; - void setFocused(bool) override; + void setFocused(bool, ShouldDebounce = ShouldDebounce::No) final; bool isFocused() const override; bool canSetFocusAttribute() const override; unsigned headingLevel() const override; diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp index 76cc532133df..0a92b103c558 100644 --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp @@ -3187,7 +3187,7 @@ void AccessibilityObject::setSelectedRows(AccessibilityChildrenVector&& selected } } -void AccessibilityObject::setFocused(bool focus) +void AccessibilityObject::setFocused(bool focus, ShouldDebounce) { if (focus) { // Ensure that the view is focused and active, otherwise, any attempt to set focus to an object inside it will fail. @@ -4390,6 +4390,12 @@ String AccessibilityObject::outerHTML() const return element ? element->outerHTML() : String(); } +bool AccessibilityObject::forceFocusDebouncing() const +{ + auto* cache = axObjectCache(); + return cache && UNLIKELY(cache->forceFocusDebouncing()); +} + namespace Accessibility { #if !PLATFORM(MAC) && !USE(ATSPI) diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h index 969ca602b902..03cc7c51fca8 100644 --- a/Source/WebCore/accessibility/AccessibilityObject.h +++ b/Source/WebCore/accessibility/AccessibilityObject.h @@ -480,7 +480,7 @@ public: // Ensures that the view is focused and active before attempting to set focus to an AccessibilityObject. // Subclasses that override setFocused should call this base implementation first. - void setFocused(bool) override; + void setFocused(bool, ShouldDebounce = ShouldDebounce::No) override; void setSelectedText(const String&) override { } void setSelectedTextRange(CharacterRange&&) override { } @@ -778,6 +778,8 @@ public: void setLastPresentedTextPrediction(Node&, CompositionState, const String&, size_t, bool); #endif // PLATFORM(IOS_FAMILY) + bool forceFocusDebouncing() const final; + protected: AccessibilityObject() = default; diff --git a/Source/WebCore/accessibility/AccessibilityScrollView.cpp b/Source/WebCore/accessibility/AccessibilityScrollView.cpp index 6bc841ebb067..7a01a4ef295c 100644 --- a/Source/WebCore/accessibility/AccessibilityScrollView.cpp +++ b/Source/WebCore/accessibility/AccessibilityScrollView.cpp @@ -111,13 +111,13 @@ bool AccessibilityScrollView::isFocused() const return webArea && webArea->isFocused(); } -void AccessibilityScrollView::setFocused(bool focused) +void AccessibilityScrollView::setFocused(bool focused, ShouldDebounce shouldDebounce) { // Call the base class setFocused to ensure the view is focused and active. AccessibilityObject::setFocused(focused); if (AccessibilityObject* webArea = webAreaObject()) - webArea->setFocused(focused); + webArea->setFocused(focused, shouldDebounce); } void AccessibilityScrollView::updateChildrenIfNecessary() diff --git a/Source/WebCore/accessibility/AccessibilityScrollView.h b/Source/WebCore/accessibility/AccessibilityScrollView.h index 40a8f13f36ab..4cd05b6938be 100644 --- a/Source/WebCore/accessibility/AccessibilityScrollView.h +++ b/Source/WebCore/accessibility/AccessibilityScrollView.h @@ -66,7 +66,7 @@ private: void updateChildrenIfNecessary() override; void setNeedsToUpdateChildren() override { m_childrenDirty = true; } void updateScrollbars(); - void setFocused(bool) override; + void setFocused(bool, ShouldDebounce = ShouldDebounce::No) final; bool canSetFocusAttribute() const override; bool isFocused() const override; diff --git a/Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm b/Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm index 75a9c5068944..d68a3040050f 100644 --- a/Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm +++ b/Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm @@ -156,6 +156,11 @@ void AXObjectCache::platformPerformDeferredCacheUpdate() { } +Seconds AXObjectCache::platformFocusDebounceInterval() const +{ + return 125_ms; +} + } #endif // ENABLE(ACCESSIBILITY) && PLATFORM(IOS_FAMILY) diff --git a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm index 4a915d156f95..c65a36bf89cb 100644 --- a/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm +++ b/Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm @@ -60,8 +60,13 @@ #import "WebCoreThread.h" #import "VisibleUnits.h" #import <CoreText/CoreText.h> +#import <wtf/SoftLinking.h> #import <wtf/cocoa/VectorCocoa.h> +SOFT_LINK(AXRuntime, AXRequestingClient, CFIndex, (void), ()) +// Defined in the AXRuntime private framework. +static constexpr CFIndex AXRequestingClientTypeVoiceOver = 3; + @interface NSObject (AccessibilityPrivate) - (void)_accessibilityUnregister; - (NSString *)accessibilityLabel; @@ -2149,8 +2154,10 @@ - (void)accessibilityIncreaseSelection:(TextGranularity)granularity - (void)_accessibilitySetFocus:(BOOL)focus { - if (auto* backingObject = self.axBackingObject) - backingObject->setFocused(focus); + if (auto* backingObject = self.axBackingObject) { + auto shouldDebounce = AXRequestingClient() == AXRequestingClientTypeVoiceOver || UNLIKELY(backingObject->forceFocusDebouncing()) ? AXCoreObject::ShouldDebounce::Yes : AXCoreObject::ShouldDebounce::No; + backingObject->setFocused(focus, shouldDebounce); + } } - (BOOL)_accessibilityIsFocusedForTesting diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp index 88c5e69a081a..7116e77837e5 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp @@ -707,10 +707,10 @@ void AXIsolatedObject::setSelectedRows(AccessibilityChildrenVector&& selectedRow }); } -void AXIsolatedObject::setFocused(bool value) +void AXIsolatedObject::setFocused(bool value, ShouldDebounce shouldDebounce) { - performFunctionOnMainThread([value] (auto* object) { - object->setFocused(value); + performFunctionOnMainThread([=] (auto* object) { + object->setFocused(value, shouldDebounce); }); } @@ -1905,6 +1905,16 @@ AXCoreObject::AccessibilityChildrenVector AXIsolatedObject::rowHeaders() return headers; } +bool AXIsolatedObject::forceFocusDebouncing() const +{ + if (!isMainThread()) { + ASSERT_NOT_REACHED(); + return false; + } + auto* cache = axObjectCache(); + return cache && UNLIKELY(cache->forceFocusDebouncing()); +} AG: How can this be called on the main thread? + #if !PLATFORM(MAC) IntPoint AXIsolatedObject::clickPoint() { diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h index 6a698d8ae15c..0e9cd3506b88 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h @@ -403,7 +403,7 @@ private: void setValueIgnoringResult(float) final; void setSelected(bool) final; void setSelectedRows(AccessibilityChildrenVector&&) final; - void setFocused(bool) final; + void setFocused(bool, ShouldDebounce = ShouldDebounce::No) final; void setSelectedText(const String&) final; void setSelectedTextRange(CharacterRange&&) final; bool setValue(const String&) final; @@ -528,6 +528,8 @@ private: String innerHTML() const final; String outerHTML() const final; + bool forceFocusDebouncing() const final; + // FIXME: Make this a ThreadSafeWeakPtr<AXIsolatedTree>. RefPtr<AXIsolatedTree> m_cachedTree; AXID m_parentID; diff --git a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm index c79ef7651672..0a10b2ba4bc1 100644 --- a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm +++ b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm @@ -36,6 +36,7 @@ #import "WebAccessibilityObjectWrapperMac.h" #import <pal/spi/cocoa/NSAccessibilitySPI.h> #import <pal/spi/mac/HIServicesSPI.h> +#import <wtf/Scope.h> #import <wtf/cocoa/TypeCastsCocoa.h> #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) @@ -742,6 +743,11 @@ bool AXObjectCache::shouldSpellCheck() #endif } +Seconds AXObjectCache::platformFocusDebounceInterval() const +{ + return 200_ms; +} + // TextMarker and TextMarkerRange funcstions. // FIXME: TextMarker and TextMarkerRange should become classes wrapping the system objects. diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm index cd3890a50a68..427330497345 100644 --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm @@ -75,6 +75,7 @@ #import "VisibleUnits.h" #import "WebCoreFrameView.h" #import <pal/spi/cocoa/NSAccessibilitySPI.h> +#import <pal/spi/mac/HIServicesSPI.h> #import <wtf/cocoa/TypeCastsCocoa.h> #import <wtf/cocoa/VectorCocoa.h> @@ -2637,8 +2638,8 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END #if PLATFORM(MAC) // In case anything we do by changing values causes an alert or other modal // behaviors, we need to return now, so that VoiceOver doesn't hang indefinitely. - callOnMainThread([value = retainPtr(value), attributeName = retainPtr(attributeName), protectedSelf = retainPtr(self)] { - [protectedSelf _accessibilitySetValue:value.get() forAttribute:attributeName.get()]; + callOnMainThread([value = retainPtr(value), attributeName = retainPtr(attributeName), protectedSelf = retainPtr(self), client = _AXGetClientForCurrentRequestUntrusted()] { + [protectedSelf _accessibilitySetValue:value.get() forAttribute:attributeName.get() fromClient:client]; }); #else // dispatch_async on earlier versions can cause focus not to track. @@ -2648,6 +2649,15 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END - (void)_accessibilitySetValue:(id)value forAttribute:(NSString *)attributeName { + [self _accessibilitySetValue:value forAttribute:attributeName fromClient:_AXGetClientForCurrentRequestUntrusted()]; +} + +- (void)_accessibilitySetValue:(id)value forAttribute:(NSString *)attributeName fromClient:(AXClientType)client +{ +#if PLATFORM(MAC) AG: I think this file is compiled only for PLATFORM(MAC), thus not needed. + ASSERT(isMainThread()); +#endif + AXTRACE(makeString("WebAccessibilityObjectWrapper _accessibilitySetValue: forAttribute:", String(attributeName))); AG: We have the tacit convention of having the AXTRACE as the first line in a function. RefPtr<AXCoreObject> backingObject = self.updateObjectBackingStore; @@ -2681,8 +2691,12 @@ - (void)_accessibilitySetValue:(id)value forAttribute:(NSString *)attributeName if (RefPtr<AXCoreObject> backingObject = protectedSelf.get().axBackingObject) backingObject->setSelectedVisiblePositionRange(AXTextMarkerRange { textMarkerRange.get() }); }); - } else if ([attributeName isEqualToString: NSAccessibilityFocusedAttribute]) { - backingObject->setFocused([number boolValue]); + } else if ([attributeName isEqualToString:NSAccessibilityFocusedAttribute]) { + // Because each focus change causes style recomputation which can be expensive, debounce it for certain clients. + // Right now this is only VoiceOver. VoiceOver is one of the most performance-sensitive clients, + // and also depends on immediately-synced DOM focus less than other clients (e.g. Full Keyboard Access). + auto shouldDebounce = client == kAXClientTypeVoiceOver || UNLIKELY(backingObject->forceFocusDebouncing()) ? AXCoreObject::ShouldDebounce::Yes : AXCoreObject::ShouldDebounce::No; + backingObject->setFocused([number boolValue], shouldDebounce); } else if ([attributeName isEqualToString: NSAccessibilityValueAttribute]) { if (number && backingObject->canSetNumericValue()) backingObject->setValueIgnoringResult([number floatValue]); AG: Do we want to de-bounce other methods down the road? E.G., make visible? AG: I don't think the de-bouncing vs. not de-bouncing belongs to the AX object. We can keep it in the axObjectCache, since it is already a class that "wears many hats" already. Can we accomplish the same by having the wrapper code accessing the cache directly?
Created attachment 468325 [details] Patch
> - } else if ([attributeName isEqualToString: NSAccessibilityFocusedAttribute]) { > - backingObject->setFocused([number boolValue]); > + } else if ([attributeName isEqualToString:NSAccessibilityFocusedAttribute]) { > + // Because each focus change causes style recomputation which can be expensive, debounce it for certain clients. > + // Right now this is only VoiceOver. VoiceOver is one of the most performance-sensitive clients, > + // and also depends on immediately-synced DOM focus less than other clients (e.g. Full Keyboard Access). > + auto shouldDebounce = client == kAXClientTypeVoiceOver || UNLIKELY(backingObject->forceFocusDebouncing()) ? AXCoreObject::ShouldDebounce::Yes : AXCoreObject::ShouldDebounce::No; > + backingObject->setFocused([number boolValue], shouldDebounce); > } else if ([attributeName isEqualToString: NSAccessibilityValueAttribute]) { > if (number && backingObject->canSetNumericValue()) > backingObject->setValueIgnoringResult([number floatValue]); > AG: Do we want to de-bounce other methods down the road? E.G., make visible? TW: Yes, I think scroll-to-make-visible should also be debounced to help avoid the scroll jumping problem we have. But there are caveats with that one that we'll need to investigate (e.g. will we cause bugs by not immediately scrolling new content into view?) Let's explore that in the future and make the solution in this patch general purpose if need be. All other comments addressed. Please re-review when you get a moment (I added a comment to the ASSERT(isMainThread()) that relates to a FIXME you added a while ago). Thanks!
Ah, unless you mean to say that I also shouldn't include ShouldDebounce in the setFocused method signature, and instead allow the cache to handle it entirely? I can try that.
Created attachment 468345 [details] Patch
(In reply to Tyler Wilcock from comment #12) > Created attachment 468345 [details] > Patch A few questions/comments to help me understand some of the details: Why we don't always debounce? The name AXObjectCache::requestFocus is rather confusing, I thought originally it was to request the focused object. I would call it setFocus or setFocusedObject. What is missing in the following simpler algorithm: 1. wrapper calls setFocusedObject. 2. setFocusedObject stores the object and document, and restarts the timer. It overwrites whatever was stored before. 3. when the timer fires, you know there has been no new calls to setFocusedObject for the platform interval. So go ahead then and set focus for real. In 2., setFocusedObject should deal with the client type, so we don't have to do it in each platform. Although I'm also pondering if we really need to distinguish between clients or we can do the debouncing for all clients. The forced debouncing flag is for the TestRunner, right? I think you can avoid that checking for the TestRunner client type if we indeed want to distinguish between clients.
Thanks for doing another round of review! Unfortunately after kicking the tires on this patch some more, I've noticed it causes jumpy VoiceOver navigation that we really don't want, so will need to understand why that's happening before we can move forward. I'll still reply to your comments just to help explain my thinking on some of these things. > A few questions/comments to help me understand some of the details: > Why we don't always debounce? > The name AXObjectCache::requestFocus is rather confusing, I thought > originally it was to request the focused object. I would call it setFocus or > setFocusedObject. I chose "requestFocus" because "setFocus" implies immediacy, whereas "requestFocus" helps know at the callsite that the focus may not be immediate. > What is missing in the following simpler algorithm: > 1. wrapper calls setFocusedObject. > 2. setFocusedObject stores the object and document, and restarts the timer. > It overwrites whatever was stored before. > 3. when the timer fires, you know there has been no new calls to > setFocusedObject for the platform interval. So go ahead then and set focus > for real. That's how it's implemented now, no? > In 2., setFocusedObject should deal with the client type, so we don't have > to do it in each platform. How you determine the client differs per-platform, so we'll need per-platform logic somewhere (in the wrappers in the latest iteration). It would probably work to move the client checks to some new AXObjectCache::platformDebouncesFocus function or something, and then call that from requestFocus / setFocus. > Although I'm also pondering if we really need to > distinguish between clients or we can do the debouncing for all clients. The delay in focus movement is pretty noticeable for sighted users, which is why it's a better tradeoff for VoiceOver, but might not be worth it for other ATs like Full Keyboard Access which is especially dependent on snappy keyboard focus.