346849 - Add a "Save Image as..." entry to the context menu for <canvas>

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement)

Description

[:Gijs (he/him)]

[I hope this is the correct component. I didn't put it in Core > Layout: Canvas because there should be a bug there about the backend for this, and I'm fairly sure there is, and the point is that the functionality actually gets exposed to the user]

Currently, a <canvas> element has roughly the same context menu as a blank area in the page. That's fairly suboptimal, in my opinion. It would be nice if we had some of the options you have on a normal <img> element, such as saving. Linking to it is probably hard or impossible (though perhaps one could generate a data URI? Not sure if there's a way to recreate everything done to a canvas after the fact), but being able to save it would be useful.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Some additions to contentAreaUtils, and here we are. This makes "View Image" and "Save Image As..." work, and removes back/forward/reload and all that from the menu (just like it does for an image and <html:input> and such). I didn't add "Copy Image" because that's a lot harder to do without a lot of rather big changes, or a gross hack. (in some more detail: nsIContentViewerEdit::CopyImage checks the node under the popup to be an nsIImageLoadingContent, which canvas isn't. So either we need a lot of changes to canvas to make it implement that, or we need a better API that doesn't rely on document.popupNode (these are big changes) or we could create a new Image(), load the data URL in there, and set document.popupNode to that image (this is the gross hack).

For now, just asking review for this first.

Comment 2

[Asaf Romano (gone)]

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

ToDataURL is kinda expensive, thus I would rather call it only when we actually  save; plus I would rather keep imageURL used only for the onImage case.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Gijs' patch with review comments addressed

I didn't completely understand what the urlSecurityCheck() was for, but discussion on IRC seemed to conclude that it wasn't needed for the Canvas case.

Comment 4

[Justin Dolske [:Dolske]]

Oh, I also set the default filename to "canvas.png", as otherwise it just seems to pick "index", which isn't very useful. 

Comment 5

[Asaf Romano (gone)]

Comment on attachment 267509 [details] [diff] [review]
Gijs' patch with review comments addressed

>Index: browser/base/content/nsContextMenu.js
>===================================================================

>+    if (this.onCanvas) {
>+      // Bypass cache, since it's a data: URL.
>+      saveImageURL(this.target.toDataURL(), "canvas.png", "SaveImageTitle", true,
>+                   false, doc.documentURIObject);
>+    } else {

}
else

sorry Gavin ;)

r=mano otherwise, thanks.

Comment 6

[Asaf Romano (gone)]

I would also rather comment and check on onImage first, to reflect WWW reality.

Comment 7

[:Gijs (he/him)]

I'm not going to be around much, so this bug is better of with Justin. If he doesn't have the time for it, please feel free to reassign (only reassign back to me if it's OK if I take till September to fix this).

Comment 8

[Justin Dolske [:Dolske]]

Attachment: (bad patch, i suck)

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch with all review comments addressed (oops)

Bah. I accidently deleted the first line of the file in my last attachment. Let's try this again...

Comment 10

[Justin Dolske [:Dolske]]

Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.15; previous revision: 1.14

Comment 11

[Marcia Knous [:marcia]]

Litmus Triage Team: marcia will create a Litmus test case for this bug.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dao, do you know if we already test for context menu entries for canvas elements in an automated test?

Comment 13

[Dão Gottwald [:dao]]

Unlikely... Also, the saving itself should probably be tested rather than the presence of the menu item.

Comment 14

[Justin Dolske [:Dolske]]

Bug 449522 added tests for content menu entries, including "Save As" on <canvas>. But, as Dao notes, it doesn't test that the entries actually do anything.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Justin. That means we can plus the in-testsuite flag. I added a Litmus test for the saving part. While doing that I noticed another bug with Select All. It's filed as bug 497868 now.

Marking verfied fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.0.10) Gecko/2009042315 Firefox/3.0.10 ID:2009042315

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://litmus.mozilla.org/show_test.cgi?id=7780