VoiceOver focus/navigation on extremely long textareas or contenteditable elements is very slow, due to spellchecking on the WebKit/Editor side. VoiceOver already handles this, so we can disable spellchecking when it is on.
<rdar://problem/117114220>
Created attachment 468259 [details] Patch
rdar://116521714
Comment on attachment 468259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468259&action=review > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:43 > + // Don't eagerly cache textarea/contenteditable values longer than 5000 characters as rangeForCharacterRange is very expensive. A FIXME might be good here. We should be able to handle text controls of any length, so this is a temporary workaround.
(In reply to Joshua Hoffman from comment #3) > rdar://116521714 From d5c2b77a0ea2978151763594c9888960cfc8cd94 Mon Sep 17 00:00:00 2001 From: Joshua Hoffman <jhoffman23@apple.com> Date: Tue, 17 Oct 2023 23:02:48 -0700 Subject: [PATCH] AX: Extremely long textareas/contenteditable fields are slow with Voiceover https://bugs.webkit.org/show_bug.cgi?id=263303 rdar://problem/117114220 Reviewed by NOBODY (OOPS!). Very long textareas/contenteditable fields can be slow with AT due to two expensive operations in WebKit: (1) Spellchecking long text & drawing misspellings. (2) Caching text using rangeForCharacterRange on the isolated tree. This patch addresses those two concerns by: AG: concerns -> causes ? (1) Not spellchecking on the WebKit side when Voiceover is the AX client. (2) Only caching textarea/contenteditable values with fewer than 5000 characters. There is also a new test included to verify that not caching long values does not affect behavior. * LayoutTests/accessibility/mac/large-text-area-expected.txt: Added. * LayoutTests/accessibility/mac/large-text-area.html: Added. * Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm: (WebCore::AXIsolatedObject::initializePlatformProperties): * Source/WebCore/editing/Editor.cpp: (WebCore::Editor::markMisspellingsAfterTypingToWord): (WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges): (WebCore::Editor::markMisspellingsAndBadGrammar): * Source/WebCore/editing/TextCheckingHelper.cpp: (WebCore::TextCheckingHelper::findFirstMisspelledWordOrUngrammaticalPhrase const): (WebCore::TextCheckingHelper::guessesForMisspelledWordOrUngrammaticalPhrase const): --- .../isolatedtree/mac/AXIsolatedObjectMac.mm | 31 +++--- Source/WebCore/editing/Editor.cpp | 9 ++ Source/WebCore/editing/TextCheckingHelper.cpp | 6 ++ .../mac/large-text-area-expected.txt | 46 ++++++++ .../accessibility/mac/large-text-area.html | 102 ++++++++++++++++++ 5 files changed, 180 insertions(+), 14 deletions(-) create mode 100644 LayoutTests/accessibility/mac/large-text-area-expected.txt create mode 100644 LayoutTests/accessibility/mac/large-text-area.html diff --git a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm b/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm index 3e7a017315f6..6df6d86c0bba 100644 --- a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm +++ b/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm @@ -40,22 +40,25 @@ void AXIsolatedObject::initializePlatformProperties(const Ref<const Accessibilit setProperty(AXPropertyName::SpeechHint, object->speechHintAttributeValue().isolatedCopy()); RetainPtr<NSAttributedString> attributedText; - if (object->hasAttributedText()) { - std::optional<SimpleRange> range; - if (isTextControl()) - range = rangeForCharacterRange({ 0, object->text().length() }); - else - range = object->simpleRange(); - if (range) { - if ((attributedText = object->attributedStringForRange(*range, SpellCheck::Yes))) - setProperty(AXPropertyName::AttributedText, attributedText); + // Don't eagerly cache textarea/contenteditable values longer than 5000 characters as rangeForCharacterRange is very expensive. + if (isTextControl() && object->text().length() < 5000) { + if (object->hasAttributedText()) { + std::optional<SimpleRange> range; + if (isTextControl()) AG: already in a isTextControl() block. + range = rangeForCharacterRange({ 0, object->text().length() }); + else + range = object->simpleRange(); + if (range) { + if ((attributedText = object->attributedStringForRange(*range, SpellCheck::Yes))) + setProperty(AXPropertyName::AttributedText, attributedText); + } } - } - // Cache the TextContent only if it is not empty and differs from the AttributedText. - if (auto text = object->textContent()) { - if (!attributedText || (text->length() && *text != String([attributedText string]))) - setProperty(AXPropertyName::TextContent, text->isolatedCopy()); + // Cache the TextContent only if it is not empty and differs from the AttributedText. + if (auto text = object->textContent()) { + if (!attributedText || (text->length() && *text != String([attributedText string]))) + setProperty(AXPropertyName::TextContent, text->isolatedCopy()); + } } AG: what about the case where object->hasAttributedText() but is not a text control? // Cache the StringValue only if it differs from the AttributedText. diff --git a/Source/WebCore/editing/Editor.cpp b/Source/WebCore/editing/Editor.cpp index 6f091192f401..c6af096c0a59 100644 --- a/Source/WebCore/editing/Editor.cpp +++ b/Source/WebCore/editing/Editor.cpp @@ -2682,6 +2682,9 @@ void Editor::markMisspellingsAfterTypingToWord(const VisiblePosition& wordStart, if (platformDrivenTextCheckerEnabled()) return; + if (!AXObjectCache::shouldSpellCheck()) + return; + #if PLATFORM(IOS_FAMILY) UNUSED_PARAM(selectionAfterTyping); UNUSED_PARAM(doReplacement); @@ -2890,6 +2893,9 @@ void Editor::markAllMisspellingsAndBadGrammarInRanges(OptionSet<TextCheckingType if (platformDrivenTextCheckerEnabled()) return; + if (!AXObjectCache::shouldSpellCheck()) + return; + ASSERT(unifiedTextCheckerEnabled()); // There shouldn't be pending autocorrection at this moment. @@ -3192,6 +3198,9 @@ void Editor::markMisspellingsAndBadGrammar(const VisibleSelection& spellingSelec if (platformDrivenTextCheckerEnabled()) return; + if (!AXObjectCache::shouldSpellCheck()) + return; + if (unifiedTextCheckerEnabled()) { if (!isContinuousSpellCheckingEnabled()) return; diff --git a/Source/WebCore/editing/TextCheckingHelper.cpp b/Source/WebCore/editing/TextCheckingHelper.cpp index 51cf9728b11e..7be4cec88889 100644 --- a/Source/WebCore/editing/TextCheckingHelper.cpp +++ b/Source/WebCore/editing/TextCheckingHelper.cpp @@ -292,6 +292,9 @@ auto TextCheckingHelper::findFirstMisspelledWordOrUngrammaticalPhrase(bool check if (platformDrivenTextCheckerEnabled()) return { }; + if (!AXObjectCache::shouldSpellCheck()) + return { }; + std::variant<MisspelledWord, UngrammaticalPhrase> firstFoundItem; GrammarDetail grammarDetail; @@ -502,6 +505,9 @@ TextCheckingGuesses TextCheckingHelper::guessesForMisspelledWordOrUngrammaticalP if (platformDrivenTextCheckerEnabled()) return { }; + if (!AXObjectCache::shouldSpellCheck()) + return { }; + AG: instead of doing this everywhere, can we add it to platformDrivenTextCheckerEnabled() with the appropriate guards for ENABLED(ACCESSIBILITY) && PLATFORM(MAC)?) if (m_range.collapsed()) return { }; AG: You are not adding this, but in a related question: bool AXObjectCache::shouldSpellCheck() { if (UNLIKELY(forceDeferredSpellChecking())) return false; AG: should be LIKELY if we are optimizing for VoiceOver. auto client = _AXGetClientForCurrentRequestUntrusted(); // The only AT that we know can handle deferred spellchecking is VoiceOver. if (client == kAXClientTypeVoiceOver) return false; AG: shouldn't it be if (client != kAXClientTypeVoiceOver) return true; #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) if (UNLIKELY(isTestAXClientType(client))) return true; AG: not needed, because above change. // ITM is currently only ever enabled for VoiceOver, so if it's enabled we can defer spell-checking. return !isIsolatedTreeEnabled(); #else return true; #endif } AG: @Tyler Wilcock may have some insight on that.
> AG: You are not adding this, but in a related question: > > bool AXObjectCache::shouldSpellCheck() > { > if (UNLIKELY(forceDeferredSpellChecking())) > return false; > > AG: should be LIKELY if we are optimizing for VoiceOver. forceDeferredSpellChecking() is only true for tests, so I think UNLIKELY is accurate here. > auto client = _AXGetClientForCurrentRequestUntrusted(); > // The only AT that we know can handle deferred spellchecking is > VoiceOver. > if (client == kAXClientTypeVoiceOver) > return false; > > AG: shouldn't it be > > if (client != kAXClientTypeVoiceOver) > return true; Your suggestion would introduce big performance regressions, because AXObjectCache::shouldSpellCheck() is called from places not directly downstream of a VoiceOver request, e.g. performDeferredCacheUpdate to cache attributed strings for ITM objects, during which we still don't want to eagerly spellcheck. Getting this logic right was really tricky. I think it's correct in all scenarios right now, and we should avoid changing it without reason. > #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) > if (UNLIKELY(isTestAXClientType(client))) > return true; > > AG: not needed, because above change. Removing this would likely cause test failures.
(In reply to Tyler Wilcock from comment #4) > Comment on attachment 468259 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=468259&action=review > > > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:43 > > + // Don't eagerly cache textarea/contenteditable values longer than 5000 characters as rangeForCharacterRange is very expensive. > > A FIXME might be good here. We should be able to handle text controls of any > length, so this is a temporary workaround. Yeah, that's a good call out. (In reply to Andres Gonzalez from comment #5) > > AG: already in a isTextControl() block. > > > - // Cache the TextContent only if it is not empty and differs from the > AttributedText. > - if (auto text = object->textContent()) { > - if (!attributedText || (text->length() && *text != > String([attributedText string]))) > - setProperty(AXPropertyName::TextContent, text->isolatedCopy()); > + // Cache the TextContent only if it is not empty and differs from > the AttributedText. > + if (auto text = object->textContent()) { > + if (!attributedText || (text->length() && *text != > String([attributedText string]))) > + setProperty(AXPropertyName::TextContent, > text->isolatedCopy()); > + } > } > > AG: what about the case where object->hasAttributedText() but is not a text > control? Both of these questions should be resolved by changing the condition to cache attributed text and text content to the following: if (!isTextControl() || object->text().length() < 5000) { This way, we cache these properties for all things that are (a) not text controls and (b) text controls with a character length less than 5000. This requires us to keep the second textControl check, but is much cleaner.
Created attachment 468276 [details] Patch
Created attachment 468283 [details] Patch
Comment on attachment 468283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468283&action=review > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:45 > + if (!isTextControl() || object->text().length() < 5000) { Are text controls with 4999 characters performant? I'm fine with cutting this off at 5000, just want to make sure we aren't setting the limit too high before we hit the performance cliff.
(In reply to Tyler Wilcock from comment #10) > Comment on attachment 468283 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=468283&action=review > > > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:45 > > + if (!isTextControl() || object->text().length() < 5000) { > > Are text controls with 4999 characters performant? I'm fine with cutting > this off at 5000, just want to make sure we aren't setting the limit too > high before we hit the performance cliff. There seems to be some variability, but let me go through another round of testing to confirm.
Created attachment 468309 [details] Patch
Comment on attachment 468309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468309&action=review > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:45 > + if (!isTextControl() || object->text().length() < 12500) { I think calling AccessibilityObject::text() here will copy the String, which is unfortunate because all we want to do is know its length. But interestingly, HTMLTextAreaElement itself seems to do roughly the same thing? String value() const final; unsigned textLength() const { return value().length(); } Since I'm guessing this copy hasn't shown up in your samples, maybe not worth worrying about. I do notice that we use object->text().length() twice -- maybe we can squash that down to one so we don't repeat this copy. unsigned textControlLength = isTextControl() ? object->text().length() : 0;
Also, did this turn out to be a viable option? > + if (!AXObjectCache::shouldSpellCheck()) > + return { }; > + > AG: instead of doing this everywhere, can we add it to > platformDrivenTextCheckerEnabled() with the appropriate guards for > ENABLED(ACCESSIBILITY) && PLATFORM(MAC)?)
(In reply to Tyler Wilcock from comment #13) > Comment on attachment 468309 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=468309&action=review > > > Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:45 > > + if (!isTextControl() || object->text().length() < 12500) { > > I think calling AccessibilityObject::text() here will copy the String, which > is unfortunate because all we want to do is know its length. > > But interestingly, HTMLTextAreaElement itself seems to do roughly the same > thing? > > String value() const final; > unsigned textLength() const { return value().length(); } > > Since I'm guessing this copy hasn't shown up in your samples, maybe not > worth worrying about. > > I do notice that we use object->text().length() twice -- maybe we can squash > that down to one so we don't repeat this copy. > > unsigned textControlLength = isTextControl() ? object->text().length() : 0; Yeah, I haven't seen this in samples but I like the idea of combining those two calls however! (In reply to Tyler Wilcock from comment #14) > Also, did this turn out to be a viable option? > > > + if (!AXObjectCache::shouldSpellCheck()) > > + return { }; > > + > > AG: instead of doing this everywhere, can we add it to > platformDrivenTextCheckerEnabled() with the appropriate guards for > ENABLED(ACCESSIBILITY) && PLATFORM(MAC)?) This does work—will include this in the latest patch.
Created attachment 468313 [details] Patch
Should've mentioned this in my last review (sorry!), but maybe AXObjectCache::shouldSpellCheck() should be changed to return `true` if accessibility is disabled to be double sure we don't affect non-AX use cases? bool AXObjectCache::shouldSpellCheck() { // This method can be called from non-accessibility contexts, so make sure to allow spell-checking if accessibility is disabled. if (!accessibilityEnabled()) return true; if (UNLIKELY(forceDeferredSpellChecking())) return false; ... } What do you think?
(In reply to Tyler Wilcock from comment #17) > Should've mentioned this in my last review (sorry!), but maybe > AXObjectCache::shouldSpellCheck() should be changed to return `true` if > accessibility is disabled to be double sure we don't affect non-AX use cases? > > bool AXObjectCache::shouldSpellCheck() > { > // This method can be called from non-accessibility contexts, so make > sure to allow spell-checking if accessibility is disabled. > if (!accessibilityEnabled()) > return true; > > if (UNLIKELY(forceDeferredSpellChecking())) > return false; > > ... > } > > What do you think? For sure! I like that for future-proofing.
Created attachment 468315 [details] Patch
Comment on attachment 468315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468315&action=review > Source/WebCore/editing/TextCheckingHelper.cpp:609 > bool platformDrivenTextCheckerEnabled() > { > +#if ENABLE(ACCESSIBILITY) && PLATFORM(MAC) > + if (!AXObjectCache::shouldSpellCheck()) > + return true; > +#endif I'm sorry for flip-flopping on this, but I sort of think moving this check into platformDrivenTextCheckerEnabled() was a mistake. It's nice that it allowed us to de-duplicate the repeated AXObjectCache::shouldSpellCheck() checks, but I don't think it makes sense based on the name of this function. How does the inverse of AXObjectCache::shouldSpellCheck() mean that "platform driven text checking is enabled"? I think it might be more correct to just repeat the check in all the places you had it before. Andres, could you please weigh in with your opinion here? What do you think?
Created attachment 468360 [details] Patch
(In reply to Joshua Hoffman from comment #21) > Created attachment 468360 [details] > Patch diff --git a/LayoutTests/accessibility/mac/large-text-area.html b/LayoutTests/accessibility/mac/large-text-area.html new file mode 100644 index 000000000000..d029bff5671e --- /dev/null +++ b/LayoutTests/accessibility/mac/large-text-area.html +<script> + +var result = "This tests that long textarea and contenteditable fields return the right strings.\n\n"; AG: We call this variable output, which is a good informal convention to maintain in case we want to re-work test output all at once. It can also be declare with let instead of var in this case. + +if (window.accessibilityController) { + result += `${accessibilityController.accessibleElementById("textarea").stringValue}\n`; + + result += `${accessibilityController.accessibleElementById("contenteditable").stringValue}\n`; AG: we should also log the length of the strings, since we are taking different paths depending on the lengths. We should also get the AttributedStrings to ensure we are not spellchecking. + + debug(result); +} + +</script> + +</body> +</html> +
Created attachment 468403 [details] Patch
Created attachment 468404 [details] Patch
Created attachment 468407 [details] Patch
Created attachment 468411 [details] Patch
Comment on attachment 468411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=468411&action=review > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:162 > + if ([clientString isEqualToString:@"VoiceOver"]) > + _AXSetClientIdentificationOverride(kAXClientTypeVoiceOver); Might be nice to compare this case-insensitively: [clientString caseInsensitiveCompare:@"voiceover"] == NSOrderedSame > LayoutTests/accessibility/mac/spellcheck-with-voiceover.html:14 > + Don't need a newline here (in my opinion). > LayoutTests/accessibility/mac/spellcheck-with-voiceover.html:29 > + Don't need a newline here (in my opinion). > LayoutTests/accessibility/mac/spellcheck-with-voiceover.html:31 > + Don't need a newline here (in my opinion).
Created attachment 468437 [details] Patch
Committed 270066@main (aa6898f2464d): <https://commits.webkit.org/270066@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 468437 [details].