Bug 214042 – [iOS] Sharing an <img> element with a base64-encoded URL shares the URL as raw text instead of an image

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Antoine Quint]

Sharing an <img> element with a base64-encoded URL shares the URL as raw text instead of an image

[Antoine Quint]

Created attachment 403683 [details]
Patch

[Antoine Quint]

<rdar://problem/56669102>

[Antoine Quint]

Created attachment 403684 [details]
Patch

[Darin Adler]

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?

[Tim Horton]

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? 😳

[Antoine Quint]

(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`.

[Darin Adler]

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.

[Antoine Quint]

(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 :)

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 403742 [details]
Patch

[Antoine Quint]

Created attachment 403743 [details]
Patch

[Wenson Hsieh]

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 *.

[Antoine Quint]

Committed r264101: <https://trac.webkit.org/changeset/264101>