Bug 224517 - [iOS] Make image extraction interactions work for elements inside links
Summary: [iOS] Make image extraction interactions work for elements inside links
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-13 16:26 PDT by Wenson Hsieh
Modified: 2021-04-14 18:36 PDT (History)
5 users (show)

See Also:


Attachments
For EWS (13.00 KB, patch)
2021-04-13 17:52 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Add a new test (15.53 KB, patch)
2021-04-14 11:35 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Add a new test (15.52 KB, patch)
2021-04-14 11:36 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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)
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].