I need to land drag and drop changes that happened on the chimera branch onto the trunk.

This patch fixes a bug in nsImageMac::ConvertToPICT by always setting the colors before the CopyBits (was bug 187290).

These changes add some new flavors to nsITransferable.idl, to allow us to keep the URL, and its description, in separate flavors. This makes it much easier to map tp the OS 'url ' and 'urld' flavors, and is cleaner. The nsDragService.cpp changes add support for drags into the file system, by implementing HFS promise dragging, and support for image data in drags, so that you can drag images into PhotoShop. nsBaseDragService provides a couple of XP utility routines that will be useful to fix this for other platforms. nsClipboard.cpp was changed to enable putting image data on the clipboard.

In layout, nsCopySupport was changed to support copying image data to the clipboard, which is hooked up in nsPresShell.cpp. Nothing will call this code yet. (If you apply the diff, the nsPresShell.cpp changes may not apply, since I had other changes in that file which I edited out of the diff.)

These are changes to nsContentAreaDragDrop.cpp, which do the following: 1. Put 'url' and 'url description' data into the transferable on drag, and look for 'url' data on drop. 2. Improve dragging of images, including the image URL 3. Deal with a single image element being selected, in which case we want to drag the image, rather than some serialized HTML.

In summary, these changes provide the following: 1. Ability to copy images to the clipboard (needs hooking up to a menu item) 2. Dragging of images to the desktop, to save the image file 3. Dragging of images into graphics apps. 4. Receiving of .webloc files from the Finder (they provide 'url' data) 5. Dragging links to the Dock (it wants 'url' data)

Comment on attachment 114268 [details] [diff] [review] gfx/src/mac patch I'm not really a fan of gratuitous whitespace and formatting changes, but since the file seems to be pretty inconsistent anyway...

> Ability to copy images to the clipboard (needs hooking up to a menu item) very cool. simon tells me it "depends on stuff in gfx", and I think at least win32 should be able to do it. (not sure about linux or other ports) note, in mozilla, right clicking on an image in browser (and mailnews) currently gives you a "Copy" menuitem, but it's disabled. sfraser's work sounds like the start of getting that enabled, but there might be some mozilla/xpfe work we'd have to do to use it.

Comment on attachment 114269 [details] [diff] [review] widget changes >--- mozilla/widget/src/mac/nsDragService.cpp 24 Oct 2002 13:39:26 -0000 1.66 >+++ mozilla/widget/src/mac/nsDragService.cpp 13 Feb 2003 00:37:39 -0000 >+ else if (strcmp(actualFlavor, kFilePromiseMime) == 0) >+ { >+ nsCOMPtr<nsISupports> imageURLPrimitive; >+ PRUint32 dataSize = 0; >+ rv = item->GetTransferData(kFilePromiseURLMime, getter_AddRefs(imageURLPrimitive), &dataSize); >+ if (NS_FAILED(rv)) return cantGetFlavorErr; >+ >+ nsCOMPtr<nsISupportsString> doubleByteText = do_QueryInterface(imageURLPrimitive); >+ if (!doubleByteText) return cantGetFlavorErr; >+ >+ nsAutoString imageURLString; >+ PRUnichar* imageURL = nsnull; >+ doubleByteText->ToString(&imageURL); >+ if (imageURL) { >+ imageURLString.Assign(imageURL); >+ nsMemory::Free(imageURL); >+ } You could use .Adopt() here to avoid copying the string. >+ image->ConvertToPICT(&picture); >+ if (!picture) return cantGetFlavorErr; >+ >+ PRInt32 pictSize = ::GetHandleSize((Handle)picture); >+ char* pictData = nsnull; >+ if (pictSize > 0) >+ pictData = (char*)nsMemory::Alloc(pictSize); >+ if (pictData) { >+ ::BlockMoveData(*picture, pictData, pictSize); // doesn't move memory >+ *outData = (void*)pictData; >+ *outDataSize = pictSize; >+ retVal = noErr; > } indenting looks a bit off here. >@@ -1029,4 +1125,126 @@ > ::InitCursor(); > > return nsBaseDragService::SetDragAction(anAction); > } >+ >+#pragma mark - Can we lose the #pragma mark's? We're not building with CodeWarrior anymore; PB and gcc don't get anything from this. >+OSErr >+nsDragService::HandleHFSPromiseDrop(DragReference inDragRef, unsigned int inItemIndex, >+ FlavorType inFlavor, const nsAString& inSourceURL, void** outData, unsigned int* outDataSize) >+{ >+ *outData = NULL; >+ *outDataSize = 0; >+ >+ OSErr err; >+ nsresult rv; >+ >+ nsCOMPtr<nsIURI> sourceURI; >+ rv = NS_NewURI(getter_AddRefs(sourceURI), inSourceURL); >+ if (NS_FAILED(rv)) return paramErr; You could use NS_ENSURE_SUCCESS(rv, paramErr). Up to you. >Index: mozilla/widget/src/xpwidgets/nsBaseDragService.cpp >=================================================================== >RCS file: /cvsroot/mozilla/widget/src/xpwidgets/nsBaseDragService.cpp,v >retrieving revision 1.27 >diff -b -u -4 -r1.27 nsBaseDragService.cpp >--- mozilla/widget/src/xpwidgets/nsBaseDragService.cpp 8 Jan 2003 23:02:43 -0000 1.27 >+++ mozilla/widget/src/xpwidgets/nsBaseDragService.cpp 13 Feb 2003 00:37:37 -0000 >@@ -52,8 +52,15 @@ > #include "nsIPresShell.h" > #include "nsIDOMNode.h" > #include "nsIPresContext.h" > >+// file downloading stuff >+#include "nsNetUtil.h" >+#include "nsEscape.h" >+#include "nsIURL.h" >+#include "nsIFile.h" >+#include "nsIWebBrowserPersist.h" >+ > How is it that you didn't have to add webbrowserpersist to REQUIRES in Makefile.in? (I really don't like that dependency, either.) >+ >+nsresult >+nsBaseDragService::CreateFileInDirectory(nsIURI* inSourceURI, nsILocalFile* inParentDir, nsILocalFile** outFile) >+{ >+ NS_ENSURE_ARG(inSourceURI); >+ NS_ENSURE_ARG(inParentDir); >+ NS_ENSURE_ARG_POINTER(outFile); >+ >+ *outFile = nsnull; >+ >+ nsresult rv; >+ >+ nsCOMPtr<nsIFile> clonedFile; >+ rv = inParentDir->Clone(getter_AddRefs(clonedFile)); >+ if (NS_FAILED(rv)) return rv; >+ >+ nsCOMPtr<nsILocalFile> destFile = do_QueryInterface(clonedFile); >+ if (!destFile) return NS_ERROR_NO_INTERFACE; I think NS_ERROR_NO_INTERFACE is normally reserved as a QueryInterface or GetInterface return value. >+ >+ *outFile = destFile.get(); .get() is not necessary here. >+// SaveURIToFile >+// used on platforms where it's possible to drag items (e.g. images) >+// into the file system >+nsresult >+nsBaseDragService::SaveURIToFile(nsIURI* inURI, nsILocalFile* inFile) >+{ >+ nsresult rv; >+ // we rely on the fact that the WPB is refcounted by the channel etc, >+ // so we don't keep a ref to it. It will die when finished. >+ nsCOMPtr<nsIWebBrowserPersist> persist = do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv); >+ if (NS_FAILED(rv)) return rv; >+ If you change to #including nsCWebBrowserPersist.idl, you get a nice macro for that contract id. But like I said, is there a way we can avoid using this interface from here?

> How is it that you didn't have to add webbrowserpersist to REQUIRES in > Makefile.in? (I really don't like that dependency, either.) Oops, missed that. Still hacking in CFM :) I'd love to avoid that dependency too; it's not pretty. Maybe the drag service code could fire off a command (adding dependencies on embedding/components/commandhandler), or call via an API on the document?

This patch enables a Copy Image item in the context menu, which replaces the disabled Copy item when an image is clicked on. Note that we should add code somewhere to disable the cmd_copyImageContents commnad for those platforms that are too lame to support it.

Comment on attachment 114272 [details] [diff] [review] layout changes > + static nsresult GetContents(const nsAString& aMimeType, PRUint32 aFlags, nsISelection *aSel, nsIDocument *aDoc, nsAString& outdata); Why make the MIME type nsAString instead of nsACString? MIME types are guaranteed to be ASCII.... (and you assume they are anyway in the implementation). >+ static nsresult ImageCopy(nsIDOMHTMLImageElement* imageElement, PRInt16 aClipboardID); What about copying <input type="image"> or objects that load images? >+ // these are ripped from nsContentAreaDragDrop. This so needs factoring. >+ static nsresult GetImageFromDOMNode(nsIDOMNode* inNode, nsIImage**outImage); >+ >+ static nsresult GetImageFrame(nsIContent* aContent, nsIDocument *aDocument, >+ nsIPresContext *aPresContext, nsIPresShell *aPresShell, >+ nsIImageFrame** aImageFrame); Note that the imageframe stuff is being removed from nsContentAreaDragDrop. See bug 83774 -- once that lands you will be able to get the image data directly from the DOM node (and ImageCopy could take an nsIImageLoadingContent* instead of nsIDOMHTMLImageElement*). What's the timeframe you have for this patch? I don't mind merging after you check this in if you're trying to get it in soon... If you're aiming for 1.4a, we should coordinate a landing order for the two patches. >+ nsCOMPtr<nsIDOMNode> imageNode = do_QueryInterface(imageElement, &rv); >+ if (NS_FAILED(rv)) return rv; >+ if (!imageNode) return NS_ERROR_FAILURE; I'd shorten that to if (!imageNode) return rv; since do_QueryInterface is guaranteed to only return non-null on success and null on failure. Similarly for do_GetService, and do_CreateInstance. >+ nsCOMPtr<nsIDOMHTMLImageElement> img(do_QueryInterface(aNode, &rv)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (img) { Same here. Looks pretty good otherwise (I didn't read the imageframe code too carefully since I assume that you copied it correctly and all and since it will hopefully not exist in the tree for long, if at all).

fwiw, #pragma mark is still very useful in projectbuilder.

Comment on attachment 114272 [details] [diff] [review] layout changes r=brade extra space on this line (nsCopySupport::ImageCopy): + if (!ptrPrimitive) return NS_ERROR_FAILURE; Looks like nsCopySupport::GetImageFrame and nsCopySupport::GetImageFromDOMNode use tabs unlike the rest of the file. Is there some reason you are nesting the coe in nsCopySupport::GetImageFromDOMNode unlike the checking in nsCopySupport::ImageCopy? Somme of the lines in GetImageFromDOMNode have different whitespace around parens: if ( imgFrame ) vs if (presContext)

*** Bug 193392 has been marked as a duplicate of this bug. ***

Comment on attachment 114268 [details] [diff] [review] gfx/src/mac patch r=pink

Comment on attachment 114273 [details] [diff] [review] content changes r=pink fix the 128+256 to use the appropriate constants and you're good to go.

This patch fixes a bad merge, and tries to clarify the code that decides what to put in the transferable. Images with links should be handled correctly. I cleaned up some string stuff too.

I rewrote nsContentAreaDragDrop::BuildDragData so it's more easily understood, factoring out some code into a new method. For ease of reading, here's the rewritten method in its entirety: PRBool nsContentAreaDragDrop::BuildDragData(nsIDOMEvent* inMouseEvent, nsAString & outURLString, nsAString & outTitleString, nsAString & outHTMLString, nsAString & outImageSourceString, nsIImage** outImage, PRBool* outIsAnchor) { if ( !outIsAnchor || !outImage ) return PR_FALSE; outURLString.Truncate(); outTitleString.Truncate(); outHTMLString.Truncate(); outImageSourceString.Truncate(); *outImage = nsnull; *outIsAnchor = PR_FALSE; nsCOMPtr<nsIDOMUIEvent> uiEvent(do_QueryInterface(inMouseEvent)); if ( !uiEvent ) return PR_FALSE; nsCOMPtr<nsIDOMEventTarget> target; inMouseEvent->GetTarget(getter_AddRefs(target)); PRBool isAltKeyDown = PR_FALSE; nsCOMPtr<nsIDOMMouseEvent> mouseEvent(do_QueryInterface(inMouseEvent)); if ( mouseEvent ) mouseEvent->GetAltKey(&isAltKeyDown); // only drag form elements by using the alt key, // otherwise buttons and select widgets are hard to use nsCOMPtr<nsIFormControl> form(do_QueryInterface(target)); if (form && !isAltKeyDown) return PR_FALSE; // the resulting strings from the beginning of the drag nsAutoString urlString; nsXPIDLString titleString; nsXPIDLString htmlString; // will be filled automatically if you fill urlstring PRBool startDrag = PR_TRUE; nsCOMPtr<nsIDOMNode> draggedNode(do_QueryInterface(target)); // find the selection to see what we could be dragging and if // what we're dragging is in what is selected. nsCOMPtr<nsIDOMAbstractView> view; uiEvent->GetView(getter_AddRefs(view)); nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(view)); if (!window) return PR_FALSE; // Get the real target and see if it is in the selection nsCOMPtr<nsIDOMNode> realTargetNode; nsCOMPtr<nsIDOMNSEvent> internalEvent = do_QueryInterface(inMouseEvent); if (internalEvent) { nsCOMPtr<nsIDOMEventTarget> realTarget; internalEvent->GetExplicitOriginalTarget(getter_AddRefs(realTarget)); realTargetNode = do_QueryInterface(realTarget); } nsCOMPtr<nsISelection> selection; window->GetSelection(getter_AddRefs(selection)); PRBool getFormattedStrings = PR_FALSE; PRBool haveSelectedContent = PR_FALSE; nsCOMPtr<nsIDOMNode> selectedImageOrLinkNode; GetDraggableSelectionData(selection, realTargetNode, getter_AddRefs(selectedImageOrLinkNode), &haveSelectedContent); nsCOMPtr<nsIDOMNode> linkNode; // set for linked images, and links nsCOMPtr<nsIDOMNode> parentLink; // possible parent link node nsCOMPtr<nsIDOMNode> selectionNormalizeNode; // if set, normalize the selection around this node nsCOMPtr<nsIDOMHTMLAreaElement> area; // client-side image map nsCOMPtr<nsIDOMHTMLImageElement> image; nsCOMPtr<nsIDOMHTMLLinkElement> link; if (selectedImageOrLinkNode) { image = do_QueryInterface(selectedImageOrLinkNode); link = do_QueryInterface(selectedImageOrLinkNode); getFormattedStrings = !image && link; } else { // we're not using a selected element. Look for draggable elements // under the mouse // if the alt key is down, don't start a drag if we're in an anchor because // we want to do selection. FindParentLinkNode(draggedNode, getter_AddRefs(parentLink)); if (parentLink && isAltKeyDown) return NS_OK; area = do_QueryInterface(draggedNode); image = do_QueryInterface(draggedNode); link = do_QueryInterface(draggedNode); } if (area) { *outIsAnchor = PR_TRUE; // grab the href as the url, use alt text as the title of the area if it's there. // the drag data is the image tag and src attribute. area->GetAttribute(NS_LITERAL_STRING("href"), urlString); area->GetAttribute(NS_LITERAL_STRING("alt"), titleString); if (titleString.IsEmpty()) titleString = urlString; htmlString = NS_LITERAL_STRING("<img src=\"") + urlString + NS_LITERAL_STRING("\">"); } else if (image) { *outIsAnchor = PR_TRUE; // grab the href as the url, use alt text as the title of the area if it's there. // the drag data is the image tag and src attribute. image->GetSrc(urlString); image->GetAttribute(NS_LITERAL_STRING("alt"), titleString); if (titleString.IsEmpty()) titleString = urlString; htmlString = NS_LITERAL_STRING("<img src=\"") + urlString + NS_LITERAL_STRING("\">"); // pass out the image source string outImageSourceString = urlString; // also grab the image data GetImageFromDOMNode(draggedNode, outImage); // If we are dragging around an image in an anchor, then we // are dragging the entire anchor (but also return the image) if (parentLink) { linkNode = parentLink; selectionNormalizeNode = linkNode; } else { // select siblings up to and including the selected link. selectionNormalizeNode = draggedNode; } } else if (link) { // set linkNode. The code below will handle this linkNode = draggedNode; GetNodeString(draggedNode, titleString); } else if (parentLink) { linkNode = parentLink; selectionNormalizeNode = linkNode; getFormattedStrings = PR_TRUE; } else if (!haveSelectedContent) { // nothing draggable startDrag = PR_FALSE; } if (linkNode) { *outIsAnchor = PR_TRUE; GetAnchorURL(linkNode, urlString); } #ifdef CHANGE_SELECTION_ON_DRAG if (selectionNormalizeNode) NormalizeSelection(selectionNormalizeNode, selection); #endif if (getFormattedStrings && selection) { // find the title for the drag and any associated html nsCOMPtr<nsISelectionPrivate> privSelection(do_QueryInterface(selection)); if (privSelection) { // the window has a selection so we should grab that rather // than looking for specific elements privSelection->ToStringWithFormat("text/html", nsIDocumentEncoder::OutputAbsoluteLinks | nsIDocumentEncoder::OutputEncodeW3CEntities, 0, getter_Copies(htmlString)); privSelection->ToStringWithFormat("text/plain", 0, 0, getter_Copies(titleString)); } else selection->ToString(getter_Copies(titleString)); } if (startDrag) { // default text value is the URL if (titleString.IsEmpty()) titleString = urlString; // if we haven't constructed a html version, make one now if (htmlString.IsEmpty() && !urlString.IsEmpty()) CreateLinkText(urlString, titleString, htmlString); } outURLString = urlString; outTitleString = titleString; outHTMLString = htmlString; return startDrag; }

Comment on attachment 119752 [details] [diff] [review] Cleaner version of the nsContentAreaDragDrop patch r=pink

This patch fixes the dragging of selected text, does the right thing when you drag selected images and text (if you click on the image or the text to drag), fixes dragging of <area> nodes (now uses absolute urls), and no longer messes with the selection in the document at all.

Comment on attachment 119997 [details] [diff] [review] Final answer content patch. Fixes string usage, and dragging <area> elements sr=kin@netscape.com Looks good to me, just change |draggedNode| to |link|, like we discussed earlier, for this tidbit: + else if (link) + { + // set linkNode. The code below will handle this + linkNode = draggedNode; + GetNodeString(draggedNode, titleString); + }

Comment on attachment 114268 [details] [diff] [review] gfx/src/mac patch This patch has been checked in.

I checked in the parts of the content/base/src patch that are not dependent on the other changes.

Patch is WIP, don't review yet.

Comment on attachment 119997 [details] [diff] [review] Final answer content patch. Fixes string usage, and dragging <area> elements This was checked in

This patch uses a new lazy data provider API on nsITransferable to do the file saving. I think it's a bit neater than the last method. This patch is ready for review.

Here's how dragging files to the Finder works with these changes. Drag code in nsContentAreaDragDrop.cpp adds a kFilePromiseMime flavor to the transferable, with an nsIFlavorDataProvider. It also adds a kFilePromiseURLMime flavor, with a string containing the source url. We map that to the OS drag flavor kDragFlavorTypePromiseHFS, which promises the flavor kDragPromisedFlavor. The OS asks for kDragPromisedFlavor, and we map that back to kFilePromiseMime. When asked for kFilePromiseMime data (here), we figure out the drop location from the OS, and set that as an nsILocalFile on the kFilePromiseDirectoryMime flavor. We then call GetTransferData() for the kFilePromiseMime flavor, which triggers the nsIFlavorDataProvider to do the save.

Comment on attachment 120914 [details] [diff] [review] Final widget patch Diffs are -w, so be prepared for some whitespace misalignment

Comment on attachment 120917 [details] [diff] [review] Final layout patch Basically, all the comments from comment 11 still apply. To clarify some of them: >Index: base/src/nsCopySupport.cpp Check whether the caller actually has to pass in an nsAString MIME type to GetContents, please. If not, pass an nsACString and convert when calling Init() on the encoder... >+nsCopySupport::ImageCopy(nsIDOMHTMLImageElement* imageElement, PRInt16 aClipboardID) Make this take nsIImageLoadingContent as the first arg. That eliminates the need for that first QI if you make GetImageFromDOMNode also take nsIImageLoadingContent (and rename it if desired; maybe GetImageFromImageContent?). >+ nsCOMPtr<nsIDOMNode> imageNode = do_QueryInterface(imageElement, &rv); >+ if (NS_FAILED(rv)) return rv; >+ if (!imageNode) return NS_ERROR_FAILURE; See comment 11. >+// GetImageFrame >+// >+// Finds the image from from a content node >+// >+nsresult >+nsCopySupport::GetImageFrame(nsIContent* aContent, nsIDocument This is no longer needed and can be removed. Includes can be reduced appropriately. >+nsCopySupport::GetImageFromDOMNode(nsIDOMNode* inNode, nsIImage**outImage) Space before "outImage"? >Index: html/base/src/nsPresShell.cpp >+ // are we an image? >+ nsCOMPtr<nsIDOMHTMLImageElement> img(do_QueryInterface(aNode, &rv)); QI to nsIImageLoadingContent instead; you want to change ImageCopy to take that anyway. ;) sr=me with these changes.

Comment on attachment 120914 [details] [diff] [review] Final widget patch Looks good, sr=me.

Comment on attachment 120916 [details] [diff] [review] Final content patch sr=me.

Comment on attachment 120917 [details] [diff] [review] Final layout patch spacing in GetImageFrame looks wacky (tabs?); please fix. remove this line or make it have the method name: +// GetImage looks great! r=brade

Comment on attachment 120914 [details] [diff] [review] Final widget patch + * @param aDataLen the length of the data, or 0 if passing a nsIFlavorDataProvider can you make a constant for zero? just so in the places where it's intended to be a flavor provider callers can be explicit about their intentions. looks good other than that. you should probably also close out 39748 as a dupe of this, or at least mark a dependency. r=pink

Comment on attachment 120916 [details] [diff] [review] Final content patch + nsCOMPtr<nsIFlavorDataProvider> thisAsProvider = NS_STATIC_CAST(nsIFlavorDataProvider*, this); + trans->SetTransferData(kFilePromiseMime, thisAsProvider, 0); what's the difference between doing it this way and just passing the static_cast'ed |this| directly to SetTransferData()? If it's an object slicing issue, won't you still get bit on the first line? +#if 0 +#pragma mark - +#endif should do something useful with this, one way or the other. +// this is our nsIFlavorDataProvider callback mention in the comments that we assume that the dest dir has been placed as a nsILocalFile in the transferable. + if (!destDirectory) return NS_ERROR_FAILURE; you never init *aData or *aDataLen, nor do any callers of this check the return value. it just so happens that we get here because *aDataLen is 0 so it'll work, but that seems more lucky than correct.

Comment on attachment 120989 [details] [diff] [review] revised content/base/src patch addressing comments r=pink

Fixing widget/src/cocoa code is covered by bug 202586, which simply makes cocoa share the files from widget/src/mac. So fixed!

Verified..

For reference, the kURLDataMime definition that was added here is involved in some Terminal paste weirdness in bug 336012. Not saying that this definition is wrong, but it may have uncovered an underlying problem elsewhere. Just wanted to add a link to it.