Bug 219398 – [iOS] Support image extraction interactions for accessibility

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

<rdar://problem/70744914>

[Wenson Hsieh]

Created attachment 415142 [details]
Patch

[Wenson Hsieh]

Comment on attachment 415142 [details]
Patch

The previous mac-debug build failure appears to due to build artifacts from #219371. Removing cq-

[Wenson Hsieh]

Created attachment 415168 [details]
Patch

[Devin Rousso]

Comment on attachment 415168 [details]
Patch

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

r=me

> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:87
> +    if (other.includeImageData && !includeImageData && !includeSnapshot)

I'd add a comment (like what's in the ChangeLog) here explaining why the `!includeSnapshot` is here too.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1066
> +    [self _tearDownImageExtraction];

Should this go above `_hasSetUpInteractions = NO;`?

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.h:40
> +- (instancetype)initWithDelegate:(UIView <WKImageExtractionGestureRecognizerDelegate> *)delegate;

Should we be more specific like `initWithImageExtractionGestureDelegate`?  That appears to be what some other `WK*GestureRecognizer` do.

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:32
> +    __weak UIView <WKImageExtractionGestureRecognizerDelegate> *_imageExtractionDelegate;

`WeakObjCPtr`?

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:43
> +    self.allowableMovement = 1;

Why not `0`?  Or is this to allow some minuscule amount of movement that's possibly expected because of how large a finger is vs a point unit?

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:51
> +    [super setState:state];

Style: can you `self.state = state;` instead (like how you use the "get" on :53)?

[Wenson Hsieh]

Comment on attachment 415168 [details]
Patch

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

>> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:87
>> +    if (other.includeImageData && !includeImageData && !includeSnapshot)
> 
> I'd add a comment (like what's in the ChangeLog) here explaining why the `!includeSnapshot` is here too.

Sounds good — added!

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1066
>> +    [self _tearDownImageExtraction];
> 
> Should this go above `_hasSetUpInteractions = NO;`?

Good catch — it probably should, along with some of the other stuff here (but we can clean that up separately).

>> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.h:40
>> +- (instancetype)initWithDelegate:(UIView <WKImageExtractionGestureRecognizerDelegate> *)delegate;
> 
> Should we be more specific like `initWithImageExtractionGestureDelegate`?  That appears to be what some other `WK*GestureRecognizer` do.

Sounds good — changed to -initWithImageExtractionGestureDelegate:

>> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:32
>> +    __weak UIView <WKImageExtractionGestureRecognizerDelegate> *_imageExtractionDelegate;
> 
> `WeakObjCPtr`?

Hm..I think we actually prefer __weak for members (perhaps thorton can comment).

>> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:43
>> +    self.allowableMovement = 1;
> 
> Why not `0`?  Or is this to allow some minuscule amount of movement that's possibly expected because of how large a finger is vs a point unit?

That's true — 0 should have essentially the same effect. I'll change it to 0.

>> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:51
>> +    [super setState:state];
> 
> Style: can you `self.state = state;` instead (like how you use the "get" on :53)?

Err…I think this would result in ∞.

It does, however, look like `super.state = state;` is a thing (which I hadn't known before)! I'll change it to that.

[Devin Rousso]

Comment on attachment 415168 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:51
>>> +    [super setState:state];
>> 
>> Style: can you `self.state = state;` instead (like how you use the "get" on :53)?
> 
> Err…I think this would result in ∞.
> 
> It does, however, look like `super.state = state;` is a thing (which I hadn't known before)! I'll change it to that.

OH 🤦‍♂️ right yes this is inside the `setState` function sorry ignore me

[Wenson Hsieh]

Created attachment 415176 [details]
Address comments

[EWS]

Committed r270332: <https://trac.webkit.org/changeset/270332>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415176 [details].