1058712 - [e10s] Copy Image of context menu does not work in e10s

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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

Comment 1

[Neil Deakin]

Attachment: copyimagecommand

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.

Comment 2

[Dave Townsend [:mossop]]

Can you give this a points estimate please?

Comment 3

[Neil Deakin]

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.

Comment 4

[Marco Mucci [:MarcoM]]

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

Comment 5

[Neil Deakin]

Attachment: Copy Image Command, v2

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.

Comment 9

[Neil Deakin]

Attachment: Update patch

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")

Comment 10

[Neil Deakin]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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 12

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 13

[Neil Deakin]

Attachment: Updated patch

I also added a test.

Comment 14

[Neil Deakin]

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 15

[(no longer active)]

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 16

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 17

[Neil Deakin]

Attachment: Updated patch

Comment 19

[Neil Deakin]

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b75aac198d0

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/9b75aac198d0

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed on latest Nightly, build ID: 20150423030204.