Bug 222811 – [iOS] Implement additional accessibility support for image overlays

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

[Wenson Hsieh]

<rdar://problem/73203775>

[Wenson Hsieh]

Created attachment 422391 [details]
Patch

[Wenson Hsieh]

Created attachment 422392 [details]
Patch

[Wenson Hsieh]

Created attachment 422402 [details]
Fix non-internal iOS build

[Wenson Hsieh]

Thanks for the review!

(Looks like I need a slight adjustment to fix the !ENABLE(IMAGE_EXTRACTION) build)

[Devin Rousso]

Comment on attachment 422402 [details]
Fix non-internal iOS build

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

r=me as well :)

> Source/WebKit/ChangeLog:27
> +        There's no need to make the regular text interaction duck in favor of the image extraction interaction after

"duck"?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2737
> +        if (HTMLElement::isImageOverlayText(*innerNonSharedNode))

Can/Should we assume that this is not `nullptr`?  Should we have `HTMLElement::isImageOverlayText` take a `const Node*` instead?

[Wenson Hsieh]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 422402 [details]
> Fix non-internal iOS build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422402&action=review
> 
> r=me as well :)
> 
> > Source/WebKit/ChangeLog:27
> > +        There's no need to make the regular text interaction duck in favor of the image extraction interaction after
> 
> "duck"?

As in, "make one thing duck to avoid another thing", but perhaps that isn't so clear — I'll reword this sentence.

> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2737
> > +        if (HTMLElement::isImageOverlayText(*innerNonSharedNode))
> 
> Can/Should we assume that this is not `nullptr`?  Should we have
> `HTMLElement::isImageOverlayText` take a `const Node*` instead?

Since this is a hit test result, I'm a bit hesitant to assume that it must be non-null.

[Wenson Hsieh]

Created attachment 422407 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 422407 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9412
> +        block(NO);

Use an enum?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9944
> +    getConfigurationAndContinue(NO);

Use an enum?

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 422407 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422407&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9412
> > +        block(NO);
> 
> Use an enum?

Good call — added this enum, and changed the block to take it instead of BOOL:

enum class ProceedWithImageExtraction : bool {
    No,
    Yes
};

> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:9944
> > +    getConfigurationAndContinue(NO);
> 
> Use an enum?

[Wenson Hsieh]

Created attachment 422428 [details]
For EWS

[Wenson Hsieh]

Created attachment 422444 [details]
Fix non-internal iOS build

[Wenson Hsieh]

Created attachment 422445 [details]
Fix non-internal iOS build

[Wenson Hsieh]

Created attachment 422450 [details]
Patch

[EWS]

Committed r274031: <https://commits.webkit.org/r274031>

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