Bug 214042 - [iOS] Sharing an <img> element with a base64-encoded URL shares the URL as raw text instead of an image
Summary: [iOS] Sharing an <img> element with a base64-encoded URL shares the URL as ra...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-07 06:46 PDT by Antoine Quint
Modified: 2020-07-08 09:16 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.45 KB, patch)
2020-07-07 06:54 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2020-07-07 07:00 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (20.90 KB, patch)
2020-07-07 16:25 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (20.95 KB, patch)
2020-07-07 16:31 PDT, Antoine Quint
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2020-07-07 06:46:21 PDT
Sharing an <img> element with a base64-encoded URL shares the URL as raw text instead of an image
Comment 1 Antoine Quint 2020-07-07 06:54:22 PDT
Created attachment 403683 [details]
Patch
Comment 2 Antoine Quint 2020-07-07 06:55:17 PDT
<rdar://problem/56669102>
Comment 3 Antoine Quint 2020-07-07 07:00:15 PDT
Created attachment 403684 [details]
Patch
Comment 4 Darin Adler 2020-07-07 09:56:13 PDT
Comment on attachment 403684 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:831
> +        if ([element.imageURL.scheme isEqualToString:@"data"] && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])

Does the "scheme" getter convert the text to lowercase? If not, then we should be checking in an ASCII case-insensitive way.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6917
> +    shareData.files = { { "Image.png", WebCore::SharedBuffer::create(UIImagePNGRepresentation(image)) } };

Is "Image.png" the best file name to use? Anything we can do better given the context? I’m not specifically suggesting this bug even "SharedImage.png" might be better? Or maybe lowercased "image.png".

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6918
> +    [self _showShareSheet:shareData inRect: { [self convertRect:boundingRect toView:self.webView] } completionHandler:[] (bool success) { }];

This seems to be a pasted copy of a line of code from above; these small possible mistakes were copied:

Extra space here after "inRect:". Not sure I understand those braces, it compiles, so OK. Don’t think we need the word "success" there in "(bool success)". Can we pass null for the completion handler instead of a block?
Comment 5 Tim Horton 2020-07-07 10:09:04 PDT
Comment on attachment 403684 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6917
>> +    shareData.files = { { "Image.png", WebCore::SharedBuffer::create(UIImagePNGRepresentation(image)) } };
> 
> Is "Image.png" the best file name to use? Anything we can do better given the context? I’m not specifically suggesting this bug even "SharedImage.png" might be better? Or maybe lowercased "image.png".

Also, should it be localized? 😳
Comment 6 Antoine Quint 2020-07-07 13:33:57 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 403684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403684&action=review
> 
> > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:831
> > +        if ([element.imageURL.scheme isEqualToString:@"data"] && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])
> 
> Does the "scheme" getter convert the text to lowercase? If not, then we
> should be checking in an ASCII case-insensitive way.

The `data` scheme is case-sensitive as far as I understand. At least a `DATA` URL in WebKit will not produce a valid image. I don't think we need to do this here.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6917
> > +    shareData.files = { { "Image.png", WebCore::SharedBuffer::create(UIImagePNGRepresentation(image)) } };
> 
> Is "Image.png" the best file name to use? Anything we can do better given
> the context? I’m not specifically suggesting this bug even "SharedImage.png"
> might be better? Or maybe lowercased "image.png".

On macOS, the sharing service in this scenario produces an image named "Image 07-07-2020 at 21.43.png", which I believe is performed by the macOS sharing system. I think we should use a localized string that, in English, works out to "Image.png".

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6918
> > +    [self _showShareSheet:shareData inRect: { [self convertRect:boundingRect toView:self.webView] } completionHandler:[] (bool success) { }];
> 
> This seems to be a pasted copy of a line of code from above; these small
> possible mistakes were copied:
> 
> Extra space here after "inRect:". Not sure I understand those braces, it
> compiles, so OK.

This is because the expected type is a FloatRect so the braces are there to construct a FloatRect from the `CGRect`. The extra space can definitely go away though.

Don’t think we need the word "success" there in "(bool
> success)". Can we pass null for the completion handler instead of a block?

We can, should and will pass `nil`.
Comment 7 Darin Adler 2020-07-07 14:36:48 PDT
Comment on attachment 403684 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:831
>>> +        if ([element.imageURL.scheme isEqualToString:@"data"] && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])
>> 
>> Does the "scheme" getter convert the text to lowercase? If not, then we should be checking in an ASCII case-insensitive way.
> 
> The `data` scheme is case-sensitive as far as I understand. At least a `DATA` URL in WebKit will not produce a valid image. I don't think we need to do this here.

You are incorrect. The scheme is *not* case-sensitive.
Comment 8 Antoine Quint 2020-07-07 16:14:50 PDT
(In reply to Antoine Quint from comment #6)
> (In reply to Darin Adler from comment #4)
> > Extra space here after "inRect:". Not sure I understand those braces, it
> > compiles, so OK.
> 
> This is because the expected type is a FloatRect so the braces are there to
> construct a FloatRect from the `CGRect`. The extra space can definitely go
> away though.

Incorrect, our style checker yells if you do :)
Comment 9 Antoine Quint 2020-07-07 16:15:20 PDT
(In reply to Darin Adler from comment #7)
> Comment on attachment 403684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403684&action=review
> 
> >>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:831
> >>> +        if ([element.imageURL.scheme isEqualToString:@"data"] && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])
> >> 
> >> Does the "scheme" getter convert the text to lowercase? If not, then we should be checking in an ASCII case-insensitive way.
> > 
> > The `data` scheme is case-sensitive as far as I understand. At least a `DATA` URL in WebKit will not produce a valid image. I don't think we need to do this here.
> 
> You are incorrect. The scheme is *not* case-sensitive.

OK. I guess we have some work to do to support that, but we can at least make this new code deal with that case.
Comment 10 Antoine Quint 2020-07-07 16:25:17 PDT
Created attachment 403742 [details]
Patch
Comment 11 Antoine Quint 2020-07-07 16:31:54 PDT
Created attachment 403743 [details]
Patch
Comment 12 Wenson Hsieh 2020-07-08 09:03:24 PDT
Comment on attachment 403743 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6927
> +    NSString *fileName = [NSString stringWithFormat:@"%@.png", (NSString*)WEB_UI_STRING("Shared Image", "Default name for the file created for a shared image with no explicit name.")];

Nit - space between NSString and *.
Comment 13 Antoine Quint 2020-07-08 09:16:51 PDT
Committed r264101: <https://trac.webkit.org/changeset/264101>