1462262 - Pasting image files onto Twitter doesn't work properly on macOS

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

Steps to reproduce:
1. choose an image file in Finder and press cmd-c to copy it
2. open Twitter website, then click "Tweet" to open the new Tweet popup
3. cmd-v to paste the file into that box

Expected result:
The image in the file should be shown in the photo area beneath the input area.

Actual result:
The icon of the file shows up in that position.


This only happens on macOS. Everything works as expected on Windows.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

So this is definitely a bug of Firefox on macOS. This is a simplified testcase for this. Copy a file and paste into this page would display the copied image from ClipboardEvent.clipboardData.files[0]. As can be seen, that file contains the icon rather than the content which seems to be wrong.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

I believe this is an issue in cocoa widget code, and the problem is in nsClipboard::TransferableFromPasteboard[1].

The problem here is that, the code piece here only checks TIFF and PNG data. When a file is pasted, the TIFF data is its icon, not its content.

I added some code there to list all types in cocoaPasteboard, and the result is:
* public.file-url
* CorePasteboardFlavorType 0x6675726C
* dyn.ah62d4rv4gu8y6y4grf0gn5xbrzw1gydcr7u1e3cytf2gn
* NSFilenamesPboardType
* dyn.ah62d4rv4gu8yc6durvwwaznwmuuha2pxsvw0e55bsmwca7d3sbwu
* Apple URL pasteboard type
* com.apple.finder.noderef
* com.apple.icns
* CorePasteboardFlavorType 0x69636E73
* fndf
* public.utf16-external-plain-text
* CorePasteboardFlavorType 0x75743136
* public.utf8-plain-text
* NSStringPboardType
* public.tiff
* NeXT TIFF v4.0 pasteboard type

(FWIW, NSPasteboardTypeTIFF checked in the code is "public.tiff" in this list.)

I suspect that what we want is probably public.file-url here. Looking at the document of NSPasteboardType[2], I suspect that corresponds to NSPasteboardTypeFileURL, but that symbol is only available on 10.13+, which wouldn't work for us. Maybe we would need to hard-code that string somehow?


[1] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/widget/cocoa/nsClipboard.mm#288-293
[2] https://developer.apple.com/documentation/appkit/nspasteboardtype?language=objc

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

So, yep, when I query cocoaPasteboard with "public.file-url", pasteboardData contains a string of the url of the file. We need to take that.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: poc patch

So this code kinda works, but I guess we don't want to hardcode string like this.

Spohl, what do you think about this patch? Any suggestion for the hardcoded string? Also, any suggestion for writing test for clipboard code like this?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

We may also need to check that the mimetype of the file matches the flavor that the transferable wants, otherwise it feels somehow broken.

Comment 6

[Markus Stange [:mstange]]

We already hardcode some of these strings here: https://searchfox.org/mozilla-central/source/widget/cocoa/nsDragService.mm#50
So this wouldn't make anything worse.

But if there is a published name for this string in a future SDK, we usually use that same name and provide our own definition in an #ifdef that detects whether we're building with an earlier SDK. Here's an example of this pattern: https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/widget/cocoa/OSXNotificationCenter.mm#25-34

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

I *guess* it's NSPasteboardTypeFileURL but haven't verified (and not sure what's the easiest way to verify).

But I guess we cannot use that directly anyway, because for NSPasteboardType, we would need to use [UTIHelper stringFromPboardType:] to get the string. So it sounds like we should just hardcode it as a constant.

Comment 8

[Stephen A Pohl [:spohl]]

I'll take a look at this shortly. Fyi, there is sample code from Apple for a clipboard viewer[1]. I have used this tool extensively when we switched drag&drop APIs in bug 1235162.

[1] https://developer.apple.com/library/content/samplecode/ClipboardViewer/Introduction/Intro.html

Comment 9

[Stephen A Pohl [:spohl]]

Comment on attachment 8976524 [details] [diff] [review]
poc patch

For the pasteboard type, you're going to be interested in kUTTypeFileURL[1]. We have a UTIHelper class that will give you the right type no matter the OS you're on (search the code base for examples). I would then try to use this in conjunction with the CGImageSourceCreateWithURL API (as opposed to CGImageSourceCreateWithData).

Thanks for looking into this!

[1] https://developer.apple.com/library/content/documentation/Miscellaneous/Reference/UTIRef/Articles/System-DeclaredUniformTypeIdentifiers.html
[2] https://developer.apple.com/documentation/imageio/1465262-cgimagesourcecreatewithurl?language=objc

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1462262 - Handle pasting image files on macOS.

Review commit: https://reviewboard.mozilla.org/r/244888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244888/

Comment 11

[Stephen A Pohl [:spohl]]

Comment on attachment 8976787 [details]
Bug 1462262 - Handle pasting image files on macOS.

https://reviewboard.mozilla.org/r/244888/#review251414

::: commit-message-a5066:1
(Diff revision 1)
> +Bug 1462262 - Handle pasting image on macOS. r?spohl

nit: s/pasting image/pasting image files/

::: widget/cocoa/nsClipboard.mm:315
(Diff revision 1)
>        else if (flavorStr.EqualsLiteral(kGIFImageMime))
>          outputType = CFSTR("com.compuserve.gif");
>        else
>          continue;
>  
> +      CGImageSourceRef source;

nit: s/CGImageSourceRef source;/CGImageSourceRef source = nullptr;/

::: widget/cocoa/nsClipboard.mm:317
(Diff revision 1)
>        else
>          continue;
>  
> +      CGImageSourceRef source;
> +      if (type == [UTIHelper stringFromPboardType:(NSString*)kUTTypeFileURL]) {
> +        NSDictionary *options =

nit: no space between type and *, one space between * and variable name, i.e.:
NSDictionary* options

This applies throughout this patch.

::: widget/cocoa/nsClipboard.mm:317
(Diff revision 1)
>        else
>          continue;
>  
> +      CGImageSourceRef source;
> +      if (type == [UTIHelper stringFromPboardType:(NSString*)kUTTypeFileURL]) {
> +        NSDictionary *options =

We can move the declaration of |options| in the else block outside of the if/else and use here as well instead of redeclaring it.

::: widget/cocoa/nsClipboard.mm:321
(Diff revision 1)
> +      if (type == [UTIHelper stringFromPboardType:(NSString*)kUTTypeFileURL]) {
> +        NSDictionary *options =
> +          [NSDictionary dictionaryWithObjectsAndKeys:
> +            (NSNumber*)kCFBooleanTrue,
> +            kCGImageSourceShouldAllowFloat, nil];
> +        NSString *urlStr = [[NSString alloc] initWithBytes:[pasteboardData bytes]

urlStr is never released. However, let's just get the urlStr as follows (which doesn't need an explicit release):
NSString* urlStr = [cocoaPasteboard stringForType:type];

::: widget/cocoa/nsClipboard.mm:325
(Diff revision 1)
> +            kCGImageSourceShouldAllowFloat, nil];
> +        NSString *urlStr = [[NSString alloc] initWithBytes:[pasteboardData bytes]
> +                                             length:[pasteboardData length]
> +                                             encoding:NSASCIIStringEncoding];
> +        NSURL *url = [NSURL URLWithString:urlStr];
> +        // XXX Should we check that url is a url pointing to a file?

Based on my testing, this does not appear to be necessary and is handled properly.

::: widget/cocoa/nsClipboard.mm:326
(Diff revision 1)
> +        NSString *urlStr = [[NSString alloc] initWithBytes:[pasteboardData bytes]
> +                                             length:[pasteboardData length]
> +                                             encoding:NSASCIIStringEncoding];
> +        NSURL *url = [NSURL URLWithString:urlStr];
> +        // XXX Should we check that url is a url pointing to a file?
> +        source = CGImageSourceCreateWithURL((CFURLRef)url, (CFDictionaryRef)options);

nit: this line is over 80 characters

::: widget/cocoa/nsClipboard.mm:328
(Diff revision 1)
> +                                             encoding:NSASCIIStringEncoding];
> +        NSURL *url = [NSURL URLWithString:urlStr];
> +        // XXX Should we check that url is a url pointing to a file?
> +        source = CGImageSourceCreateWithURL((CFURLRef)url, (CFDictionaryRef)options);
> +      } else {
> -      // Use ImageIO to interpret the data on the clipboard and transcode.
> +        // Use ImageIO to interpret the data on the clipboard and transcode.

Let's move this comment to before the if/else, since it applies to both CGImageSourceCreateWithURL as well as CGImageSourceCreateWithData.

::: widget/cocoa/nsClipboard.mm:330
(Diff revision 1)
> +        // XXX Should we check that url is a url pointing to a file?
> +        source = CGImageSourceCreateWithURL((CFURLRef)url, (CFDictionaryRef)options);
> +      } else {
> -      // Use ImageIO to interpret the data on the clipboard and transcode.
> +        // Use ImageIO to interpret the data on the clipboard and transcode.
> -      // Note that ImageIO, like all CF APIs, allows NULLs to propagate freely
> +        // Note that ImageIO, like all CF APIs, allows NULLs to propagate freely
> -      // and safely in most cases (like ObjC). A notable exception is CFRelease.
> +        // and safely in most cases (like ObjC). A notable exception is CFRelease.

nit: this line is over 80 characters

::: widget/cocoa/nsClipboard.mm:335
(Diff revision 1)
> -      // and safely in most cases (like ObjC). A notable exception is CFRelease.
> +        // and safely in most cases (like ObjC). A notable exception is CFRelease.
> -      NSDictionary *options =
> +        NSDictionary *options =
> -        [NSDictionary dictionaryWithObjectsAndKeys:
> +          [NSDictionary dictionaryWithObjectsAndKeys:
> -          (NSNumber*)kCFBooleanTrue,
> +            (NSNumber*)kCFBooleanTrue,
> -          kCGImageSourceShouldAllowFloat,
> +            kCGImageSourceShouldAllowFloat,
> -          (type == [UTIHelper stringFromPboardType:NSPasteboardTypeTIFF] ?
> +            (type == [UTIHelper stringFromPboardType:NSPasteboardTypeTIFF] ?

Since we're touching these lines, let's just pass |type| here. It is already either [UTIHelper stringFromPboardType:NSPasteboardTypeTIFF] or [UTIHelper stringFromPboardType:NSPasteboardTypePNG], so no need to check again.

::: widget/cocoa/nsClipboard.mm:340
(Diff revision 1)
> -          (type == [UTIHelper stringFromPboardType:NSPasteboardTypeTIFF] ?
> +            (type == [UTIHelper stringFromPboardType:NSPasteboardTypeTIFF] ?
> -                     [UTIHelper stringFromPboardType:NSPasteboardTypeTIFF] :
> +                      [UTIHelper stringFromPboardType:NSPasteboardTypeTIFF] :
> -                     [UTIHelper stringFromPboardType:NSPasteboardTypePNG]),
> +                      [UTIHelper stringFromPboardType:NSPasteboardTypePNG]),
> -          kCGImageSourceTypeIdentifierHint, nil];
> +            kCGImageSourceTypeIdentifierHint, nil];
>  
> -      CGImageSourceRef source = CGImageSourceCreateWithData((CFDataRef)pasteboardData, 
> +        source = CGImageSourceCreateWithData((CFDataRef)pasteboardData, (CFDictionaryRef)options);

nit: this line is over 80 characters

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: patch with review comments addressed

Since there were so many comments in the review (and it started getting confusing even for myself) I figured it might be helpful to upload a patch that has these addressed. Feel free to incorporate this as-is in your MozReview. Thanks!

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8976787 [details]
Bug 1462262 - Handle pasting image files on macOS.

https://reviewboard.mozilla.org/r/244888/#review251414

> nit: s/CGImageSourceRef source;/CGImageSourceRef source = nullptr;/

We are assigning to it anyway, so I think it's not a big deal... but it's not a big deal either way, so I can add that initializer if you prefer.

> nit: no space between type and *, one space between * and variable name, i.e.:
> NSDictionary* options
> 
> This applies throughout this patch.

Because it seems to me that's the dominating style in that file... but if you think we should use the Mozilla style in this file I can do that.

> We can move the declaration of |options| in the else block outside of the if/else and use here as well instead of redeclaring it.

I didn't do this because it seems to me `kCGImageSourceTypeIdentifierHint` doesn't make much sense for file-uri, but I guess it doesn't harm either...

> urlStr is never released. However, let's just get the urlStr as follows (which doesn't need an explicit release):
> NSString* urlStr = [cocoaPasteboard stringForType:type];

I've forgotten about how lifetime is managed in ObjC... I suppose anything that isn't retained should get released when we return to the event loop?

`GetDataFromPasteboard` has a `@try` block around `dataForType:`. Do we need one for `stringForType:` as well? (The document doesn't seem to be very clear on this.)

> nit: this line is over 80 characters

Do we apply 80 characters line length on ObjC files? It seems to me there are tons of lines in this file exceed 80 characters... and in general it's hard to apply that restriction.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8976787 [details]
Bug 1462262 - Handle pasting image files on macOS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244888/diff/1-2/

Comment 15

[Stephen A Pohl [:spohl]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)
> Comment on attachment 8976787 [details]
> Bug 1462262 - Handle pasting image files on macOS.
> 
> https://reviewboard.mozilla.org/r/244888/#review251414
> 
> > nit: s/CGImageSourceRef source;/CGImageSourceRef source = nullptr;/
> 
> We are assigning to it anyway, so I think it's not a big deal... but it's
> not a big deal either way, so I can add that initializer if you prefer.

Right, this isn't a big deal at the moment. The intent here was to avoid possible mistakes in the future if somebody else changes this code and potentially leaves this variable uninitialized.

> > nit: no space between type and *, one space between * and variable name, i.e.:
> > NSDictionary* options
> > 
> > This applies throughout this patch.
> 
> Because it seems to me that's the dominating style in that file... but if
> you think we should use the Mozilla style in this file I can do that.

Right, we want new code to follow current Mozilla style. This particular file is not being touched very often, so it will take some time for this to propagate.

> > We can move the declaration of |options| in the else block outside of the if/else and use here as well instead of redeclaring it.
> 
> I didn't do this because it seems to me `kCGImageSourceTypeIdentifierHint`
> doesn't make much sense for file-uri, but I guess it doesn't harm either...

You're correct, it might not make a lot of sense. But luckily, this is just a hint and we can simplify the code by using it this way.

> > urlStr is never released. However, let's just get the urlStr as follows (which doesn't need an explicit release):
> > NSString* urlStr = [cocoaPasteboard stringForType:type];
> 
> I've forgotten about how lifetime is managed in ObjC... I suppose anything
> that isn't retained should get released when we return to the event loop?

It's not quite that simple. `urlStr` was alloc/init'ed in the previous patch, which means that you own the object and will have to release it. Apple's documentation gives a good overview of memory management in Objective-C:
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html

> `GetDataFromPasteboard` has a `@try` block around `dataForType:`. Do we need
> one for `stringForType:` as well? (The document doesn't seem to be very
> clear on this.)
> 
> > nit: this line is over 80 characters
> 
> Do we apply 80 characters line length on ObjC files? It seems to me there
> are tons of lines in this file exceed 80 characters... and in general it's
> hard to apply that restriction.

Yes, this rule applies to any new code. The reason why there are so many lines over 80 characters in this file is the same as what I mentioned above: this file does not see a lot of activity and it will take some time for this to be fixed across this file.

Comment 16

[Stephen A Pohl [:spohl]]

Comment on attachment 8976787 [details]
Bug 1462262 - Handle pasting image files on macOS.

https://reviewboard.mozilla.org/r/244888/#review251844

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/183970b632a3
Handle pasting image files on macOS. r=spohl

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/183970b632a3