_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.
<rdar://problem/34217143>
Created attachment 319665 [details] patch
Created attachment 323958 [details] patch merge with TOT
Comment on attachment 323958 [details] patch Can someone take a look? Thanks!
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.
By the way, feel free to email my webkit.org or apple.com email address if you need a code review.
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.
(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.
Created attachment 324241 [details] patch rename
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.
(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.
Created attachment 324294 [details] patch to land update from review
I want to make sure tests are still passing. Ryosuke, can you take a quick look again? Thanks.
Comment on attachment 324294 [details] patch to land Clearing flags on attachment: 324294 Committed r223726: <https://trac.webkit.org/changeset/223726>
All reviewed patches have been landed. Closing bug.