+++ This bug was initially created as a clone of Bug #1058539 +++

There are two issues here: 1. Make sure that the correct command state is propagated up to the parent. This patch is a work in progress that does that. 2. There is a ClipboardProxy that only supports text, so copying images just fails.

Can you give this a points estimate please?

The second part of comment 1 above would fall under bug 1071562. The first part is in the patch above but needs some polish, perhaps to remove the extra argument to UpdateCommands that I added.

Hi Neil, can you mark this bug for QE verification.

This is a simpler version that doesn't modify the command updating api. The only interface change is to add a way to set the winddow root popup node. I added it to nsIContentViewerEdit as it relates to the other methods in that interface, although it isn't ideal. Once dependent bug 1071562 is fixed, this should work.

This patch removes some unneccesary bits and fixes an issue where <input> elements were treated as images as they implement nsIImageLoadingContent (for <input type="image")

Oh and this patch needs both 1071562 which in turn needs 936092.

Unfortunately, I can't reliably test this patch - the patches in bug 936092 and bug 1071562 are pretty bitrotted. So I'll just be doing inspection here.

Comment on attachment 8576691 [details] [diff] [review] Update patch Review of attachment 8576691 [details] [diff] [review]: ----------------------------------------------------------------- Hey Enn, Just a few notes / questions. Thanks! -Mike ::: browser/base/content/content.js @@ +172,5 @@ > spellInfo = > InlineSpellCheckerContent.initContextMenu(event, editFlags, this); > } > > + // Update the state of the commands on the context menu. Set the event target I think this comment should be phrased differently - first, we set the event target so that the copy image command can use it, and _then_ we update the contentcontextmenu commands so that the change to copy image is reflected. Right? ::: browser/base/content/nsContextMenu.js @@ +455,5 @@ > // Copy image contents depends on whether we're on an image. > + let allowCopyImage; > + if (this.isRemote) { > + let controller = this.browser.controllers.getControllerForCommand("cmd_copyImage"); > + allowCopyImage = controller ? Isn't this just: allowCopyImage = controller && controller.isCommandEnabled("cmd_copyImage"); ? Also, what is the case where controller.isCommandEnabled("cmd_copyImage") is false, but this.onImage is true? ::: docshell/base/nsIContentViewerEdit.idl @@ +31,5 @@ > readonly attribute boolean canGetContents; > + > + // Set the node that will be the subject of the editing commands above. > + // Usually this will be the node that was context-clicked. > + void setCommandNode(in nsIDOMNode aNode); Nit - might as well carry on the wacky alignment in this file. ::: layout/base/nsDocumentViewer.cpp @@ +2707,5 @@ > *aCanGetContents = nsCopySupport::CanCopy(mDocument); > return NS_OK; > } > > +NS_IMETHODIMP nsDocumentViewer::SetCommandNode(nsIDOMNode* aNode) You might need a layout peer to review this stuff. @@ +2714,5 @@ > + NS_ENSURE_TRUE(document, NS_ERROR_FAILURE); > + > + nsCOMPtr<nsPIDOMWindow> window(document->GetWindow()); > + NS_ENSURE_TRUE(window, NS_ERROR_NOT_AVAILABLE); > + if (window) { Why if (window)? Line 2717 ensures that it is truthy, otherwise we would have returned NS_ERROR_NOT_AVAILABLE. @@ +2716,5 @@ > + nsCOMPtr<nsPIDOMWindow> window(document->GetWindow()); > + NS_ENSURE_TRUE(window, NS_ERROR_NOT_AVAILABLE); > + if (window) { > + nsCOMPtr<nsPIWindowRoot> root = window->GetTopWindowRoot(); > + NS_ENSURE_TRUE(root, NS_ERROR_FAILURE); This is just NS_ENSURE_STATE(root); no?

I also added a test.

Try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ef60012a4a1 if you want to try it out without working out the dependencies.

Comment on attachment 8584618 [details] [diff] [review] Updated patch Review of attachment 8584618 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDocumentViewer.cpp @@ +3545,5 @@ > NS_ENSURE_TRUE(node, NS_ERROR_FAILURE); > > + // Make sure there is a URI assigned. This allows <input type="image"> to > + // be an image but rejects other <input> types. This matches what > + // nsContentMenu.js does. Nit: nsContextMenu.js. Please add a comment to nsContextMenu to remind people to update this logic if they ever change the other side.

Comment on attachment 8584618 [details] [diff] [review] Updated patch Review of attachment 8584618 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content.js @@ +175,5 @@ > } > > + // Set the event target first as the copy image command needs it to > + // determine what was context-clicked on. Then, update the state of the > + // commands on the context menu. Nit - trailing ws ::: browser/base/content/test/general/browser_clipboard.js @@ +145,5 @@ > + var doc = content.document; > + var main = doc.getElementById("main"); > + main.focus(); > + > + yield new content.Promise((resolve, reject) => { Nit - I'm pretty sure Promise is available in the ContentTask's scope, without needing to grab it from content. @@ +154,5 @@ > + // DataTransfer doesn't support the image types yet, so only text/html > + // will be present. > + if (clipboardData.getData("text/html") != > + '<img id="img" tabindex="1" src="http://example.org/browser/browser/base/content/test/general/moz.png">') { > + return "FAIL"; Should this be a rejection?

I'm not checking this one in yet due to a test failure only on Mac.

Verified fixed on latest Nightly, build ID: 20150423030204.