User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release) Build ID: 20140127194636 Steps to reproduce: Tested on Mac OS X 10.9.1 and Firefox Nightly 29.0a1 (2014-02-02) 1. Make following 3 webloc files on Desktop * URL: https://bugzilla.mozilla.org/ Filename: Bugzilla Main Page.webloc * URL: http://www.mozilla.org/en-US/ Filename: Home of the Mozilla Project — Mozilla.webloc * URL: https://developer.mozilla.org/en-US/ Filename: Mozilla Developer Network.webloc 2. Choose them and drag and drop to Browser window 3. call event.dataTransfer.getData("text/x-moz-url") 4. call event.dataTransfer.mozGetDataAt("text/x-moz-url", index) with 0..2 for index Actual results: event.dataTransfer.getData("text/x-moz-url") returns following text: > https://bugzilla.mozilla.org/ > http://www.mozilla.org/en-US/ > https://developer.mozilla.org/en-US/ event.dataTransfer.mozGetDataAt("text/x-moz-url", index) also returns above text, regardless of index argument Expected results: According to following document: https://developer.mozilla.org/en-US/docs/DragDrop/Recommended_Drag_Types text/x-moz-url is following format: > http://www.mozilla.org > Mozilla > http://www.xulplanet.com > XUL Planet So, event.dataTransfer.getData("text/x-moz-url") should return following text: > https://bugzilla.mozilla.org/ > Bugzilla Main Page > http://www.mozilla.org/en-US/ > Home of the Mozilla Project — Mozilla > https://developer.mozilla.org/en-US/ > Mozilla Developer Network then, event.dataTransfer.mozGetDataAt("text/x-moz-url", 0) should return following text: > https://bugzilla.mozilla.org/ > Bugzilla Main Page Similar problem happens when I drag and drop bookmarks from Safari window.

When I drop single image file and single webloc file, null is returned from mozGetDataAt("text/x-moz-url", index), if I image filename is smaller than webloc filename in alphabetical order (such as a.png and b.webloc). If webloc filename is smaller than image filename (such as a.webloc and b.png), single URL (of webloc file) is returned from mozGetDataAt("text/x-moz-url", index), regardless of index argument.

If we retrieve URL list from [globalDragPboard stringForType:pboardType], It's difficult to know which URL is generated from which file, so simplest solution may be making URL itself as its title by duplicating each line, such like: > https://bugzilla.mozilla.org/ > https://bugzilla.mozilla.org/ > http://www.mozilla.org/en-US/ > http://www.mozilla.org/en-US/ > https://developer.mozilla.org/en-US/ > https://developer.mozilla.org/en-US/ Then, I noticed that flavors other than "application/x-moz-file" are not designed to return content of multiple entries separately. So, now I think it's okay that event.dataTransfer.mozGetDataAt("text/x-moz-url", index) returns same value as event.dataTransfer.getData("text/x-moz-url"). This patch also fixes the bug in comment 1 by calling [globalDragPboard stringForType:pboardType] again with alternative type.

Sorry, my assumption was wrong. If I drag and drop 2 bookmark items from Library window, mozGetDataAt("text/x-moz-url", 0) and mozGetDataAt("text/x-moz-url", 1) return different URLs. Thus, as you wrote in bug 92737 comment 22, we have to check all items in the dataTransfer for any flavor. If so, we should avoid returning duplicated entries for different index. How about following solution? If we use the return value of [globalDragPboard stringForType:pboardType], mozGetDataAt("text/x-moz-url", 0) and getData("text/x-moz-url") return all URLs, but mozGetDataAt("text/x-moz-url", 1) and others return nothing. So, we can use the return value of [globalDragPboard stringForType:pboardType], and there is no duplicated URLs.

Sorry, I forgot to update the patch.

Comment on attachment 8406732 [details] [diff] [review] do not return duplicated entries >+ if (flavorStr.EqualsLiteral(kURLMime)) { >+ NSArray* lines = [pString componentsSeparatedByString:@"\n"]; >+ NSMutableArray* namedLines = [[NSMutableArray alloc] init]; >+ for (uint32_t j = 0, lineCount = [lines count]; j < lineCount; j ++) { >+ [namedLines addObject:[lines objectAtIndex:j]]; >+ [namedLines addObject:[lines objectAtIndex:j]]; >+ } Is this meant to return all lines? text/x-moz-url only has one url, no?

Thanks for feedback. (In reply to Neil Deakin from comment #5) > Is this meant to return all lines? text/x-moz-url only has one url, no? Yes, it will return all dropped URLs. The block duplicates all lines, to make the URL itself as a title of each URL. (Every URL corresponds to one of dropped items, but we don't know which URL corresponds to which item) There is an example of text/x-moz-url which contains 2 URLs (and 2 titles) in MDN: https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Recommended_Drag_Types#Dragging_Links If this is wrong example, and text/x-moz-url can contain only one pair of URL and title, please tell me, I'll fix the MDN page, and try to find another solution :)

Sorry for the long delay. Rebased the patch to latest trunk, with minor tweaks. I guess there is no way to test the feature in automated tests, since it requires drag and drop from other application (e.g. from Finder). So need to do manual test (maybe MozTrap?). almost green on try run, except two (intermittent?) orange which shouldn't be related to this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc07c43663e (also contains patches for bug 92737 and bug 417918)

This patch shouldn't be a problem, but it would seem more correct to me to provide each url as a separate item. If you're up to it though, it may actually be better to change all of this code to use the 10.6 clipboard api instead [NSPasteboard readObjectsForClasses] which just returns an array.

Thanks, I replaced `[NSPasteboard stringForType:]` by `[NSPasteboard readObjectsForClasses:]`, and call `[NSPasteboardItem stringForType:]` for each item. Green on try runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=248b20c7b6cf (mac) https://treeherder.mozilla.org/#/jobs?repo=try&revision=da5901d205ff (linux) Then currently `dataTransfer.mozTypesAt` returns same entries for all items, since `nsDragService::IsDataFlavorSupported` does not have item index parameter. But for Mac's case, it may be nice to return different entries per each item. For example, if I dropped PNG file and WEBLOC file, PNG file has only one UTI: * "public.file-url" (returns file URL) and WEBLOC file has following UTIs: * "public.file-url" (returns file URL) * "com.apple.traditional-mac-plain-text" * "public.utf8-plain-text" * "public.url" (returns the URL in the WEBLOC file) (I guess this is the reason why we get the problem in comment #1) In this patch, for now, `nsDragService::IsDataFlavorSupported` returns true if at least one item supports the flavor. So, `dataTransfer.mozTypesAt` returns types supported by at least one item, regardless of index parameter. In future version, it might be better to support item index parameter, separated from this bug. (it may cause compatibility problem though...)

Also, I added kURLDataMime and kURLDescriptionMime to IsDataFlavorSupported, since they're handled in GetData.

Comment on attachment 8543012 [details] [diff] [review] Part 1: Fix format of DataTransfer.getData(text/x-moz-url) and return each url as a separated item on Mac. Ah, I was expecting to just change the url handling. For IsDataFlavorSupported, canReadItemWithDataConformingToTypes should be used instead, as you don't want to retrieve the data until its asked for. Similarly some mechanism should be used for GetNumDropItems. >+ NSArray* classes = [[NSArray alloc] initWithObjects:[NSPasteboardItem class], nil]; >+ NSDictionary* options = [NSDictionary dictionary]; >+ return [globalDragPboard readObjectsForClasses:classes options:options]; I'm not entirely sure, but I think classes will need to be released afterwards? >+nsDragService::GetText(NSPasteboardItem* item, NSArray* types) >+{ >+ NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; >+ >+ if ([types containsObject:(id)kUTTypeUTF8PlainText]) { >+ NSString* str = [item stringForType:(id)kUTTypeUTF8PlainText]; >+ if (str) >+ return str; >+ } >+ if ([types containsObject:(id)kUTTypeURL]) { >+ NSString* str = [item stringForType:(id)kUTTypeURL]; >+ if (str) >+ return str; >+ } >+ if ([types containsObject:(id)kUTTypeURLName]) { >+ NSString* str = [item stringForType:(id)kUTTypeURLName]; >+ if (str) >+ return str; >+ } This should only return the text for kUTTypeUTF8PlainText. You should also make sure to have a Cocoa module reviewer review this as well. The other patch is ok though, plus has the bonus of supporting text/uri-list. Thanks!

Updated demo

Thank you for reviewing :) Now this patch uses following methods: * [NSPasteboard pasteboardItems], [NSPasteboardItem availableTypeFromArray:], and [NSPasteboardItem stringForType:] for nsDragService::GetData * [NSPasteboard availableTypeFromArray:] for nsDragService::IsDataFlavorSupported * [NSPasteboard pasteboardItems] for nsDragService::GetNumDropItems (In reply to Neil Deakin from comment #11) > Comment on attachment 8543012 [details] [diff] [review] > Part 1: Fix format of DataTransfer.getData(text/x-moz-url) and return each > url as a separated item on Mac. > > Ah, I was expecting to just change the url handling. Oops, you are right, now this patch also modifies handling for all types, for consistency and simplicity, so the commit message was not precise. > For IsDataFlavorSupported, canReadItemWithDataConformingToTypes should be > used instead, as you don't want to retrieve the data until its asked for. Thank you for the hint, I totally misunderstood about UTI conformance. I shouldn't use [NSArray containsObject:]. Then, I need to use [NSPasteboard availableTypeFromArray:] instead of [NSPasteboard canReadItemWithDataConformingToTypes] to determine if "public.file-url" is used for "public.url". If file is dropped, we need URL provided by the webloc file, but don't need the file's URL. I added `IsValidType` method to prevent exposing file URI to content. > Similarly some mechanism should be used for GetNumDropItems. There seems to be private function `CFPasteboardGetItemCount` which returns item count directly, but there is no information about the function :( (and I guess it should be avoided to use such function) Can I use [NSPasteboard pasteboardItems]? Which returns same as the array returned by [NSPasteboard readObjectsForClasses:] with "NSPasteboardItem" class. I don't see any other way to know correct item count. Of course, dropping multiple item can happen only with files, afaik. So I'm okay to revert the patch for GetNumDropItems, and use [NSPasteboard types] and [NSPasteboard propertyListForType:] as before. It's done just for consistency, with other functions. > >+ NSArray* classes = [[NSArray alloc] initWithObjects:[NSPasteboardItem class], nil]; > >+ NSDictionary* options = [NSDictionary dictionary]; > >+ return [globalDragPboard readObjectsForClasses:classes options:options]; > > I'm not entirely sure, but I think classes will need to be released > afterwards? Yes, it should be [NSArray arrayWithObjects:]. > >+nsDragService::GetText(NSPasteboardItem* item, NSArray* types) > >+{ > >+ NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; > >+ > >+ if ([types containsObject:(id)kUTTypeUTF8PlainText]) { > >+ NSString* str = [item stringForType:(id)kUTTypeUTF8PlainText]; > >+ if (str) > >+ return str; > >+ } > >+ if ([types containsObject:(id)kUTTypeURL]) { > >+ NSString* str = [item stringForType:(id)kUTTypeURL]; > >+ if (str) > >+ return str; > >+ } > >+ if ([types containsObject:(id)kUTTypeURLName]) { > >+ NSString* str = [item stringForType:(id)kUTTypeURLName]; > >+ if (str) > >+ return str; > >+ } > > This should only return the text for kUTTypeUTF8PlainText. Okay, removed other types.

Rebased. Then, I noticed that "text/x-moz-url-desc" (kURLDescriptionMime) is not handled in dom/events/DataTransfer.cpp, but currently it's supported only by Mac, so it should be better to leave it, right? https://dxr.mozilla.org/mozilla-central/source/dom/events/DataTransfer.cpp#1125 > // there isn't a way to get a list of the formats that might be available on > // all platforms, so just check for the types that can actually be imported > // XXXndeakin there are some other formats but those are platform specific. > const char* formats[] = { kFileMime, kHTMLMime, kURLMime, kURLDataMime, kUnicodeMime };

This bug is complex, and I'll have to dig around quite a bit before I understand it. So I may not be able to finish my review before next week.

I've taken a closer look ... and found that I'd need to know a lot more about how dragging works (in Gecko and on OS X) to do intelligent reviews of these patches. I can acquire this knowledge, by digging around in the Mozilla and various docs (both from Mozilla and Apple), but it would take more time than I currently have. I'm really sorry about this, but I'm going to have to put off my reviews for rather a long time -- possibly a month or two.

No problem, thank you for taking your time :D

Rebased onto bug 1199336. Basically this reverts changes from bug 1199336. This patch removes the use of propertyListForType, so we no more need wrapper. No other changes from previous one. Do you have time to review?

Just rebased.

I'm retiring at the end of this month, and have one large project to finish by then (bug 1186187). If I have time to spare from that, this will be at the top of my list.

Comment on attachment 8660382 [details] [diff] [review] Part 1: Fix format of DataTransfer.getData(text/x-moz-url) and return each text and URL as a separated item on Mac. I don't know much about drag-and-drop in either Gecko or Cocoa. And I'm not going to have time to learn. But I *can* evaluate these patches as additions to existing OS X Widgets code. In this respect they look fine to me. The changes they make to our dragging behavior make sense -- though I frankly don't know enough to say whether or not they're correct. Someone else will have to evaluate this. Neil Deakin liked earlier versions of your patches. But I think he should also review your latest patches. I do have one style nit: + if (!allowFileURL && [availableType isEqualToString:(id)kUTTypeFileURL]) + return false; In this and several other cases you don't put curly braces around one-line if blocks. That used to be our style (and the code you're changing is quite old). But it's not the current style. The example above should look like this: + if (!allowFileURL && [availableType isEqualToString:(id)kUTTypeFileURL]) { + return false; + } (We generally only change the style of old code when we make substantive changes. Style-only changes would pollute the change history, and make it harder to trace.)

By the way I did test these patches (in current trunk code), and they do get rid of the problem described in comment #0.

Markus, I don't know how much you know about drag-and-drop. But do feel free to jump in here if you think that's appropriate.

Thank you for reviewing and testing! :D

Rebased and fixed styles. Neil, do you have time to review?

Comment on attachment 8676539 [details] [diff] [review] Part 1: Fix format of DataTransfer.getData(text/x-moz-url) and return each text and URL as a separated item on Mac. r=smichaud r+'ing based on smichaud's previous r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/200d18cf0eecdc929dfc1a26f985c807d1924de2 Bug 966986 - Part 1: Fix format of DataTransfer.getData(text/x-moz-url) and return each text and URL as a separated item on Mac. r=smichaud, vlad https://hg.mozilla.org/integration/mozilla-inbound/rev/97e7e631a9f5adba2616189199ea5f6122b21e27 Bug 966986 - Part 2: Return true from nsDragService::IsDataFlavorSupported for text/x-moz-url-desc and text/x-moz-url-data on Mac. r=smichaud, vlad

https://hg.mozilla.org/mozilla-central/rev/200d18cf0eec https://hg.mozilla.org/mozilla-central/rev/97e7e631a9f5