The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 3 alpha7

Status

()

Firefox
Menus
--
enhancement
VERIFIED FIXED
11 years ago
5 years ago

People

(Reporter: Gijs, Assigned: Dolske)

Tracking

Trunk
Firefox 3 alpha7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
[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.

Updated

11 years ago
Depends on: 245684

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

10 years ago
(Reporter)

Comment 1

10 years ago
Created attachment 260121 [details] [diff] [review]
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #260121 - Flags: review?(mano)
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.
Attachment #260121 - Flags: review?(mano) → review-
(Assignee)

Comment 3

10 years ago
Created attachment 267509 [details] [diff] [review]
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.
Attachment #260121 - Attachment is obsolete: true
Attachment #267509 - Flags: review?(mano)
(Assignee)

Comment 4

10 years ago
Oh, I also set the default filename to "canvas.png", as otherwise it just seems to pick "index", which isn't very useful. 
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.
Attachment #267509 - Flags: review?(mano) → review+
I would also rather comment and check on onImage first, to reflect WWW reality.
(Reporter)

Comment 7

10 years ago
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).
Assignee: gijskruitbosch+bugs → dolske
Status: ASSIGNED → NEW
(Assignee)

Updated

10 years ago
Target Milestone: --- → Firefox 3 beta1
(Assignee)

Comment 8

10 years ago
Created attachment 270083 [details] [diff] [review]
(bad patch, i suck)
Attachment #267509 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
Created attachment 270084 [details] [diff] [review]
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...
Attachment #270083 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #270083 - Attachment description: Patch with review comments addressed. → (bad patch, i suck)
(Assignee)

Updated

10 years ago
Whiteboard: [checkin needed]
(Assignee)

Comment 10

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Updated

10 years ago
Depends on: 386744
Flags: in-testsuite- → in-litmus?
Litmus Triage Team: marcia will create a Litmus test case for this bug.
Flags: in-testsuite?
Dao, do you know if we already test for context menu entries for canvas elements in an automated test?
Flags: in-litmus? → in-litmus-
Unlikely... Also, the saving itself should probably be tested rather than the presence of the menu item.
Flags: in-litmus- → in-litmus?
(Assignee)

Comment 14

8 years ago
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.
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
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7780
Blocks: 713173
You need to log in before you can comment on or make changes to this bug.