1325770 - Cannot save image by drag and drop to desktop

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Tooru Fujisawa [:arai]]

Tested on OSX 10.11.6

Steps to reproduce:
  1. Open Nightly
  2. Open https://addons.cdn.mozilla.net/static/img/icons/firefox.png in current tab
  3. Drag and drop the image to desktop

Actual result:
  The image can be dragged, but when drop it, nothing happens.
  (while bisecting, I also saw the case that image cannot be dragged, intermittently or something)

Expected result:
  The image can be dragged, and when drop it, it's saved to desktop

Regression range
  https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=09a4e9d9b8fa5b3ca47a9a582575db2ed38dc260&tochange=82ca4696b3425b6b5674ef48422d93a63dda2554

Comment 1

[Stephen A Pohl [:spohl]]

Thanks for bisecting and reporting! Looking into it.

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Patch

This was a bit more involved than expected. namesOfPromisedFilesDroppedAtDestination is no longer being called by the OS since bug 1235162 landed, so we have to drop to some lower level pasteboard manager calls to figure out what the destination of the file is. Inspiration for this was gathered from http://lists.apple.com/archives/cocoa-dev/2012/Feb/msg00706.html.

Comment 3

[Stephen A Pohl [:spohl]]

Attachment: Formatting changes

This simply improves formatting and one of the comments.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8821954 [details] [diff] [review]
Patch

Review of attachment 8821954 [details] [diff] [review]:
-----------------------------------------------------------------

Oh dear. This was quite the rabbit hole.

It looks like namesOfPromisedFilesDroppedAtDestination is still called and respected when the drag is kicked off using -[NSView dragPromisedFilesOfTypes:...]. But that API doesn't let us add other pasteboard types or specify the drag image.

It looks like another way to get the destination directory would be to do this: https://gist.github.com/Wevah/10145527 (which Wevah found when implementing this for Chrome, apparently). But I think the way you chose is better.

And I agree with https://twitter.com/bjhomer/status/395985854151159809 .

Comment 5

[Markus Stange [:mstange]]

With this patch we don't ever write the names of the created files back to the pasteboard, do we? We just sneakily create them where the user wants them and don't tell the system about the fact that this was caused by the drag action.

Comment 6

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #5)
> With this patch we don't ever write the names of the created files back to
> the pasteboard, do we? We just sneakily create them where the user wants
> them and don't tell the system about the fact that this was caused by the
> drag action.

That's correct. I suppose namesOfPromisedFilesDroppedAtDestination meant to do this via:

  NSPasteboard* generalPboard = [NSPasteboard pasteboardWithName:NSDragPboard];
  NSData* data = [generalPboard dataForType:@"application/x-moz-file-promise-dest-filename"];
  NSString* name = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
  NSArray* rslt = [NSArray arrayWithObject:name];

But when I tested this, this always resulted in an empty array so I dropped it. Did I miss something?

Comment 7

[Markus Stange [:mstange]]

No I don't think you missed anything. But I'm not sure how the old code could have resulted in an empty array - +[NSArray arrayWithObject:] should create an array with exactly one element, that element being the file name of the file that was created.
But since we don't use namesOfPromisedFilesDroppedAtDestination any more, we have no place to return the filename to, so I think what you did is fine.

Comment 8

[Stephen A Pohl [:spohl]]

Sorry, I misspoke. I meant an array with one element with an empty file name, i.e. @"".

Comment 9

[Markus Stange [:mstange]]

Ah, I see. That sounds like this wasn't really working properly before anyway and nothing broke because of it.

Comment 10

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #9)
> Ah, I see. That sounds like this wasn't really working properly before
> anyway and nothing broke because of it.

Yes, that was my understanding as well.

Comment 11

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9412916a5fbea552e88ec76cf90592c96b76c8f5
Bug 1325770: Fix regression from bug 1235162 that prevented drag/dropping of images to the OSX Desktop. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/26a1994127d21cf82cb9c2936ff42a923ac2d928
Bug 1325770: Change formatting to abide by code style guidelines and improve a comment. r=mstange

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9412916a5fbe
https://hg.mozilla.org/mozilla-central/rev/26a1994127d2

Comment 13

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 53.0a1 (2016-12-24) (64-bit) on WIndows 7, 64 Bit!

This bug's fix is verified with latest Beta!

Build   ID : 20170327081421
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170322]

Comment 14

[Stephen A Pohl [:spohl]]

(In reply to Maruf Rahman[:mMARUF] from comment #13)
> I have reproduced this bug with Nightly 53.0a1 (2016-12-24) (64-bit) on
> WIndows 7, 64 Bit!

This bug was on OSX and cannot be verified on Windows.

Comment 15

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 53.0a1 (2016-12-24) on MacOS Sierra 10.12!

This bug's fix is verified with Firefox 53.0!

Build   ID : 20170413192749
User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:53.0) Gecko/20100101 Firefox/53.0

Comment 16

[Stephen A Pohl [:spohl]]

Per comment 15.