Bug 256629 – AX: Use AXDidSpellCheck to allow assistive technologies to lazily resolve spell-checking

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Eager spell-checking can cause performance issues.

[Radar WebKit Bug Importer]

<rdar://problem/109191055>

[Tyler Wilcock]

Created attachment 466319 [details]
Patch

[Tyler Wilcock]

Created attachment 466320 [details]
Patch

[chris fleizach]

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?

[Tyler Wilcock]

(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.

[Tyler Wilcock]

Created attachment 466322 [details]
Patch

[chris fleizach]

(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

[Andres Gonzalez]

(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.

[Tyler Wilcock]

Created attachment 466365 [details]
Patch

[Tyler Wilcock]

Created attachment 466366 [details]
Patch

[chris fleizach]

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()"

[Tyler Wilcock]

Created attachment 466367 [details]
Patch

[Tyler Wilcock]

(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.

[Tyler Wilcock]

Created attachment 466371 [details]
Patch

[EWS]

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].