Eager spell-checking can cause performance issues.
<rdar://problem/109191055>
Created attachment 466319 [details] Patch
Created attachment 466320 [details] Patch
Comment on attachment 466320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466320&action=review > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:2105 > + if (didSpellCheck) { we should probably check that this is [didSpellCheck boolValue] ? > LayoutTests/accessibility/mac/attributed-string/attributed-string-does-not-includes-misspelled-for-non-editable-expected.txt:10 > +PASS attrString.indexOf('AXDidSpellCheck = 0') != -1 is true do we still want to have a test that does perform spell checking?
(In reply to chris fleizach from comment #4) > Comment on attachment 466320 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466320&action=review > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:2105 > > + if (didSpellCheck) { > > we should probably check that this is [didSpellCheck boolValue] ? There are two steps here: 1. Does objectForKey:@"AXDidSpellCheck" return non-nil, indicating that attribute is present? 2. If non-nil, what is the boolValue? The `if (didSpellCheck)` you highlighted performs the first step. If that evaluates to true, it means the property is set, and the debug output in the next line does the boolValue. Checking BOOL misspelled in the lines above follows a similar pattern. > > LayoutTests/accessibility/mac/attributed-string/attributed-string-does-not-includes-misspelled-for-non-editable-expected.txt:10 > > +PASS attrString.indexOf('AXDidSpellCheck = 0') != -1 is true > > do we still want to have a test that does perform spell checking? I think probably not, because it means we'd have to keep eager spell-checking codepaths around simply for the sake of testing them even though we won't actually behave that way when queried by AX clients. I'm open to discussion though if you think there's a good reason to keep such tests around. We could always bring them back if we decide we want to eagerly spell-check sometimes.
Created attachment 466322 [details] Patch
(In reply to Tyler Wilcock from comment #5) > (In reply to chris fleizach from comment #4) > > Comment on attachment 466320 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=466320&action=review > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:2105 > > > + if (didSpellCheck) { > > > > we should probably check that this is [didSpellCheck boolValue] ? > There are two steps here: > > 1. Does objectForKey:@"AXDidSpellCheck" return non-nil, indicating that > attribute is present? > 2. If non-nil, what is the boolValue? > > The `if (didSpellCheck)` you highlighted performs the first step. If that > evaluates to true, it means the property is set, and the debug output in the > next line does the boolValue. > > Checking BOOL misspelled in the lines above follows a similar pattern. > > > > LayoutTests/accessibility/mac/attributed-string/attributed-string-does-not-includes-misspelled-for-non-editable-expected.txt:10 > > > +PASS attrString.indexOf('AXDidSpellCheck = 0') != -1 is true > > > > do we still want to have a test that does perform spell checking? > I think probably not, because it means we'd have to keep eager > spell-checking codepaths around simply for the sake of testing them even > though we won't actually behave that way when queried by AX clients. I'm > open to discussion though if you think there's a good reason to keep such > tests around. We could always bring them back if we decide we want to > eagerly spell-check sometimes. We have other clients besides VO though and they won't likely update to handle this so I feel we need to support both paths. We only know this will work in VO
(In reply to chris fleizach from comment #7) > (In reply to Tyler Wilcock from comment #5) > > (In reply to chris fleizach from comment #4) > > > Comment on attachment 466320 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=466320&action=review > > > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:2105 > > > > + if (didSpellCheck) { > > > > > > we should probably check that this is [didSpellCheck boolValue] ? > > There are two steps here: > > > > 1. Does objectForKey:@"AXDidSpellCheck" return non-nil, indicating that > > attribute is present? > > 2. If non-nil, what is the boolValue? > > > > The `if (didSpellCheck)` you highlighted performs the first step. If that > > evaluates to true, it means the property is set, and the debug output in the > > next line does the boolValue. > > > > Checking BOOL misspelled in the lines above follows a similar pattern. > > > > > > LayoutTests/accessibility/mac/attributed-string/attributed-string-does-not-includes-misspelled-for-non-editable-expected.txt:10 > > > > +PASS attrString.indexOf('AXDidSpellCheck = 0') != -1 is true > > > > > > do we still want to have a test that does perform spell checking? > > I think probably not, because it means we'd have to keep eager > > spell-checking codepaths around simply for the sake of testing them even > > though we won't actually behave that way when queried by AX clients. I'm > > open to discussion though if you think there's a good reason to keep such > > tests around. We could always bring them back if we decide we want to > > eagerly spell-check sometimes. > > > > We have other clients besides VO though and they won't likely update to > handle this so I feel we need to support both paths. We only know this will > work in VO Here is a proposal. The Mac API already has to entry points: AXAttributedStringForTextMarkerRangeAttribute and AXAttributedStringForTextMarkerRangeWithOptionsAttribute. In ITM we are always doing eager caching of spelling and removing it if the client doesn't want it through the latter API. What we need to do instead is: 1. Always cache without spelling. 2. Only do the spelling if requested through AXAttributedStringForTextMarkerRangeWithOptionsAttribute. In this case, hit the main thread and be slow. 3. Make sure that VO never uses AXAttributedStringForTextMarkerRangeWithOptionsAttribute to request for spelling. Here is a proposal: The Mac API already has to entry points: AXAttributedStringForTextMarkerRangeAttribute and AXAttributedStringForTextMarkerRangeWithOptionsAttribute. In ITM we were always doing eager caching of the spelling.
Created attachment 466365 [details] Patch
Created attachment 466366 [details] Patch
Comment on attachment 466365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466365&action=review > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:658 > +bool AXObjectCache::shouldSpellCheckEagerly() this feels like it should be the default mode.. aka it should just be called shouldSpellCheck() with the assumption that means spell checking will happen the opposite would then be "deferSpellCheckingToClient()"
Created attachment 466367 [details] Patch
(In reply to chris fleizach from comment #11) > Comment on attachment 466365 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466365&action=review > > > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:658 > > +bool AXObjectCache::shouldSpellCheckEagerly() > > this feels like it should be the default mode.. aka it should just be called > > shouldSpellCheck() > > with the assumption that means spell checking will happen > > the opposite would then be > > "deferSpellCheckingToClient()" You're right, that naming is better. Updated in the latest patch.
Created attachment 466371 [details] Patch
Committed 264188@main (017e7fd5e52a): <https://commits.webkit.org/264188@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466371 [details].