Sharing an <img> element with a base64-encoded URL shares the URL as raw text instead of an image
Created attachment 403683 [details] Patch
<rdar://problem/56669102>
Created attachment 403684 [details] Patch
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 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? 😳
(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 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.
(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 :)
(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.
Created attachment 403742 [details] Patch
Created attachment 403743 [details] Patch
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 *.
Committed r264101: <https://trac.webkit.org/changeset/264101>