Bug 224517

Summary:

[iOS] Make image extraction interactions work for elements inside links

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

Platform

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, hi, megan_gardner, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
For EWS	none
Add a new test	none
Add a new test	ews-feeder: commit-queue-

Description Wenson Hsieh 2021-04-13 16:26:23 PDT

Comment 1 Radar WebKit Bug Importer 2021-04-13 16:27:24 PDT

<rdar://problem/76616431>

Comment 2 Wenson Hsieh 2021-04-13 17:52:25 PDT

Created attachment 425936 [details]
For EWS

Comment 3 Wenson Hsieh 2021-04-14 07:33:38 PDT

Comment on attachment 425936 [details]
For EWS

(Pretty sure the `http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html` failure is unrelated)

Comment 4 Devin Rousso 2021-04-14 10:32:23 PDT

Comment on attachment 425936 [details]
For EWS

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2698
> +    info.imageElementContext = page.contextForElement(element);

Won't this be the exact same value as `elementContext` since `imagePositionInformation` is called with the same `Element` right before the `elementContext` is set?  If so, would just `isImage` be enough to indicate that the `elementContext` is for an image?

Comment 5 Wenson Hsieh 2021-04-14 10:36:49 PDT

Comment on attachment 425936 [details]
For EWS

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2698
>> +    info.imageElementContext = page.contextForElement(element);
> 
> Won't this be the exact same value as `elementContext` since `imagePositionInformation` is called with the same `Element` right before the `elementContext` is set?  If so, would just `isImage` be enough to indicate that the `elementContext` is for an image?

They are the same in the `elementPositionInformation` case, but not in the case where the clickable element is a link (<a>).

Comment 6 Devin Rousso 2021-04-14 10:52:11 PDT

Comment on attachment 425936 [details]
For EWS

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

r=me

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2698
>>> +    info.imageElementContext = page.contextForElement(element);
>> 
>> Won't this be the exact same value as `elementContext` since `imagePositionInformation` is called with the same `Element` right before the `elementContext` is set?  If so, would just `isImage` be enough to indicate that the `elementContext` is for an image?
> 
> They are the same in the `elementPositionInformation` case, but not in the case where the clickable element is a link (<a>).

Ah I see are you referring to the `downcast<HTMLImageElement>(*hitTestNode)` below?  That makes sense.

NIT: In that case, are we concerned with having duplicate data in the non-link scenario?  Should we only set the `imageElementContext` when we it's for a different `Element` than the `elementContext`?

Comment 7 Wenson Hsieh 2021-04-14 11:00:58 PDT

Comment on attachment 425936 [details]
For EWS

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

>>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2698
>>>> +    info.imageElementContext = page.contextForElement(element);
>>> 
>>> Won't this be the exact same value as `elementContext` since `imagePositionInformation` is called with the same `Element` right before the `elementContext` is set?  If so, would just `isImage` be enough to indicate that the `elementContext` is for an image?
>> 
>> They are the same in the `elementPositionInformation` case, but not in the case where the clickable element is a link (<a>).
> 
> Ah I see are you referring to the `downcast<HTMLImageElement>(*hitTestNode)` below?  That makes sense.
> 
> NIT: In that case, are we concerned with having duplicate data in the non-link scenario?  Should we only set the `imageElementContext` when we it's for a different `Element` than the `elementContext`?

Hm…that could be done, but I feel like that might make the logic that consults this element context more confusing, since it would need to check both the `elementContext` and `imageElementContext` (depending on the `isImage` flag). If it's alright, I think I would prefer to keep `imageElementContext` the way it is in this patch, to make things a bit simpler.

(I'll update the patch to add a test for the link clicking behavior, as we discussed on slack!)

Comment 8 Wenson Hsieh 2021-04-14 11:35:35 PDT Comment hidden (obsolete)

Created attachment 426023 [details]
Add a new test

Comment 9 Wenson Hsieh 2021-04-14 11:36:31 PDT

Created attachment 426024 [details]
Add a new test

Comment 10 EWS 2021-04-14 17:11:40 PDT

Committed r275980 (236532@main): <https://commits.webkit.org/236532@main>

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