Bug 176247 – AX: Provide a way for Accessibility to cache the selection while retrieving rects for speak selection

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

_accessibilityRetrieveRectsAtSelectionOffset:withText: is problematic for speak selection since it would use the current selection as the search range. The rects would be wrong when a speak session is active and the user tries to select some other text.

[Radar WebKit Bug Importer]

<rdar://problem/34217143>

[Nan Wang]

Created attachment 319665 [details]
patch

[Nan Wang]

Created attachment 323958 [details]
patch

merge with TOT

[Nan Wang]

Comment on attachment 323958 [details]
patch

Can someone take a look? Thanks!

[Ryosuke Niwa]

Comment on attachment 323958 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323958&action=review

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1893
> +void WebPage::cacheSelectionForAccessibility(bool shouldCache)

I don't think we should call this a "cache" since what we're doing here is to save the selection at the time of invocation.
A better name might be like storeSelectionForAccessibility or freezeSelectionForAccessibility.

There is an alternative approach which is to expose some sort of opaque object like WKWebViewSelection
and then add a new variant of getRectsForGranularityWithSelectionOffset which takes that opaque object
and computes the correct rect but that might be a bit of an overkill for this particular bug.

[Ryosuke Niwa]

By the way, feel free to email my webkit.org or apple.com email address if you need a code review.

[Nan Wang]

Comment on attachment 323958 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323958&action=review

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1893
>> +void WebPage::cacheSelectionForAccessibility(bool shouldCache)
> 
> I don't think we should call this a "cache" since what we're doing here is to save the selection at the time of invocation.
> A better name might be like storeSelectionForAccessibility or freezeSelectionForAccessibility.
> 
> There is an alternative approach which is to expose some sort of opaque object like WKWebViewSelection
> and then add a new variant of getRectsForGranularityWithSelectionOffset which takes that opaque object
> and computes the correct rect but that might be a bit of an overkill for this particular bug.

Thanks for the alternative approach! But for this particular bug I think it's more straightforward to go with the current approach and it's easier for the iOS assistive technology to adapt the changes.
I will rename them.

[Nan Wang]

(In reply to Ryosuke Niwa from comment #6)
> By the way, feel free to email my webkit.org or apple.com email address if
> you need a code review.

Sure! Thanks for the review.

[Nan Wang]

Created attachment 324241 [details]
patch

rename

[Ryosuke Niwa]

Comment on attachment 324241 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324241&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2313
> +- (void)_accessibilityStoreSelection:(BOOL)shouldStore
> +{
> +    _page->storeSelectionForAccessibility(shouldStore);
> +}

Maybe it's better to add _accessibilityClearSelection instead of making this method take a bool
It's kind of weird to say _accessibilityStoreSelection: FALSE.
It sounds as if we're going to not store selection in the future instead of the call directly clearing what's already been stored.

[Nan Wang]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 324241 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324241&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2313
> > +- (void)_accessibilityStoreSelection:(BOOL)shouldStore
> > +{
> > +    _page->storeSelectionForAccessibility(shouldStore);
> > +}
> 
> Maybe it's better to add _accessibilityClearSelection instead of making this
> method take a bool
> It's kind of weird to say _accessibilityStoreSelection: FALSE.
> It sounds as if we're going to not store selection in the future instead of
> the call directly clearing what's already been stored.

Ok I will make another patch to address this.

[Nan Wang]

Created attachment 324294 [details]
patch to land

update from review

[Nan Wang]

I want to make sure tests are still passing. 
Ryosuke, can you take a quick look again? Thanks.

[WebKit Commit Bot]

Comment on attachment 324294 [details]
patch to land

Clearing flags on attachment: 324294

Committed r223726: <https://trac.webkit.org/changeset/223726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.