449522 - Context menu for HTML5 <video> elements

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement)

Description

[Biju]

Just like we do on <IMG>, context menu on VIDEO should show.

1. View Video
2. Copy Video Location
3. Properties

PS: I think there is also a bug asking for video items to show up on media tab of page info

Comment 1

[cmtalbert]

I'd also like a way to stop/start the video from a context menu, even if the "controls" attribute is not present.  This way the user always has ultimate control over a video.  

Comment 2

[Biju]

(from bug 448909 comment #42)
> Has any thought been given to a right click context menu?
> 
> In my opinion, it should include access to more advanced features like:
> Open media in external player
> Selection of video stream
> Selection of Audio stream
> Selection of captions
> Video Properties
> video size scaling (maybe)

Comment 3

[Biju]

* Email Video/Audio URL

Comment 4

[Biju]

looks like I accidentally ASSIGNED to "nobody".
how do make status back to NEW

Comment 5

[Justin Dolske [:Dolske]]

Ta-da. :)

Comment 6

[Justin Dolske [:Dolske]]

CCing UX folks, and moving to Firefox since the context menu is a front-end thing.

As a first pass, I'm thinking:
* Add "Save Video As..."
* Add "Copy Video Location"
* Hook up "Properties"
* Add a "Media Controls" submenu
  - "Play" / "Pause" (depending on state)
  - "Stop"
  - "Mute"
  - "Show Fullscreen"
  - "Show Controls" / "Hide Controls"

I'm not sure if some of the context menu entries for images necessarily make sense to have for videos, or if they will require additional work to make usable:

* "View Image" --> "View Video"
  Somewhat duplicates "View Fullscreen". No really sure what the use case is
  for this.

* "Copy Image" --> "Copy Video"
  Not sure if you can actually do this (does it put a 600MB video into the
  clipboard, or just a link?)

* "Send Image" --> "Send Video"
  Same issue as "Copy Video", I suspect. Emailing 600MB videos isn't going to
  work so well either, although it might be reasonable for smaller files. Not
  sure how to capture that in a UI. Maybe just "Send Video URL", as suggested
  above?

I don't think the items suggested in comment 2 are commonly useful enough to provide in the default UI, extensions can take care of that.

Comment 7

[Biju]

(In reply to comment #6)
> * "View Image" --> "View Video"
>   Somewhat duplicates "View Fullscreen". No really sure what the use case is
>   for this.
not same as "Fullscreen" 
UseCase: I want to see video in one window "video player" while continue browsing in original window.

> * "Copy Image" --> "Copy Video"
>   Not sure if you can actually do this (does it put a 600MB video into the
>   clipboard, or just a link?)
instead let it copy the HTML code of video tag, so that a blogger can paste to his blog. This is not same as coping as link to video.

> * "Send Image" --> "Send Video"
similar to above

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

First rough pass, don't have everything hooked up yet.

* Filed bug 453063 to support fullscreen playback. No menu item for that till there's code to implement it!

* Toggling "Show Controls" back on doesn't seem to make the controls appear again, not sure what's up with that. Bug in the <video> code / overlay, maybe?

* Some of the functionality in nsContextMenu.js for images overlaps with what we want to do with video/audio [eg, viewImage(), saveImage()]. I'll probably just refactor this into shared code (viewMedia/saveMedia?), unless the nitpicky differences make it easier to keep separate.

Comment 9

[Biju]

(In reply to comment #8)
> * Toggling "Show Controls" back on doesn't seem to make the controls appear
> again, not sure what's up with that. Bug in the <video> code / overlay, maybe?

is it Bug 449360

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Less-rough second pass.

* Added "View Video" (not fully working, due to bug 448603), and "Send Video" (it's only sending the URL, anyway).

* Added "Copy Video Location", but it's not yet working because this will end up needing changes down in docshell! (See the "Copy Image Location" code for the gory details of how copying a URL onto the clipboard is done.)

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Testcase (images, videos, linked, unlinked)

Comment 12

[Justin Dolske [:Dolske]]

After wading though a bunch of native code that gets invoked via the context menu's "goDoCommand()" (for copying an image's location), it turns out all the interesting work happens in nsCopySupport::ImageCopy, and that's easy enough to implement with smidgen of JS.

However, it's much harder to do "Copy Image" this way, since it puts the actual image pixel data onto the clipboard. Can't do this in JS yet (bug 315357). Since this arguably isn't useful for audio/video anyway, I'm going to leave the old code path in place for "Copy Image", and not implement a "Copy Video" (just a "Copy Video Location"). It can always be added later if there's demand.

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Ready for a review pass. A few notes on this version:

* Removed the menuitem for stopping the video; doesn't work, needs bug 449282.

* "View Video" doesn't quiiite work as expected, needs bug 448603. Instead of showing the video in the browser window (ala "View Image"), it triggers a dislog to Save/Open the file. This still seems useful, so I've left it in. Should work as expected when 448603 is fixed.

* "Show Controls" seems hooked up correctly, but the controls don't come back. I suspect a bug with the CSS or binding, looking into it.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Bah, forgot to qdiff before attaching.

Comment 15

[Justin Dolske [:Dolske]]

+ // XXX I think this is buggy, removeAttribute doesn't work for me.

Huh, can't replicate that now, WFM either way. I think the way I'm setting the muted/controls checkbox w/ setAttribute("checked", bool) is cleaner, though.

Comment 16

[Justin Dolske [:Dolske]]

Michael: I suppose the Linux menus should have moz-icon images on some of these menuitems (notably media controls like Play/Pause). How can I find which ones would be correct to use?

Comment 17

[Michael Monreal [:micmon]]

Sure, these ones would make sense to me:

- "Save Video As..." -> gtk-save-as
- "Copy Video Location" -> gtk-copy
- "Play" -> gtk-media-play
- "Pause" -> gtk-media-pause
- "Stop" -> gtk-media-stop
- "Show Fullscreen" -> gtk-fullscreen

Just ask if you add any more items.

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

Boriss should make the call here since she is driving this design, but my
feedback is that it seems we should add a command for embedding, but I really
have no idea what to call it.

Copy Embed
Copy Embed Code
Copy Code to Embed

Embed -> Dialog with more information

Comment 19

[Justin Dolske [:Dolske]]

There's unused support for something like that with images: the code serializes the image's DOM node and drops that onto the clipboard. Pasting into something that accepts (asks for) HTML would then give it "<img src='foo.png' alt='bar'>".

Comment 20

[Jennifer Morrow [:Boriss] (UX)]

Here's my suggestion re the current patch:

View Video
Save Video As...
Copy Video Location...
Properties...
-------(divider)-----
Play/Pause
Stop
Mute
Show Fullscreen
Show Control/Hide Controls

My question if we really need a separate menu item for "copy video location" and "copy the video location with an HTML tag."  This may make more sense for an image, because copying an image rather than its URL is very much a separate interaction and use case.  But if the difference here is only the wrapping HTML code, are we sure that this is such a different and used action that it warrants another item?

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Screen of last patch + GTK icons

This is what's in the last patch (also added GTK icons, I'll attach an updated patch after the first review pass, since it's trivial).

The order/grouping of the entries is basically the same as what's used for images. I think we should stay with that for consistency. I put the play/pause/mute stuff at the top because it seemed like those would be most used, and it matches having the navigation items at the top when the context menu is opened on a page body.

I guess it seems odd, now that I look again, to have the Mute / Show Controls entries be checkable instead of changing name (Mute <--> Unmute, Show Controls <--> Hide Controls).  I can fix that in the next version too.

Comment 22

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #21)
> I guess it seems odd, now that I look again, to have the Mute / Show Controls
> entries be checkable instead of changing name (Mute <--> Unmute, Show Controls
> <--> Hide Controls).  I can fix that in the next version too.

Definitely agree with this.  Is fullscreen not here because it's not being implemented?  I'm assuming it will be in in the list as soon as it can be done.

Comment 23

[Justin Dolske [:Dolske]]

Correct, Fullscreen isn't in the menu because there's no support for it. It can be added then that support does.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 336747 [details] [diff] [review]
Patch v.4

I'm a bit concerned that renaming imageURL and saveImage/viewImage will unnecessarily break a bunch of extensions, but it's hard to evaluate the tradeoff between code clarity and extension compat without an easy way to gather data about extension use (AMO MXR should help in the future, if that ever happens).

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>+    // XXX can't yet load a video directly -- bug 448603
>+    this.showItem("context-viewvideo", this.onVideo);

So should we disable this for now?

>+        // XXX I think this is buggy, removeAttribute doesn't work for me.
>         if (this.isImageBlocked())
>           blockImage.setAttribute("checked", "true");
>         else
>           blockImage.removeAttribute("checked");

Bug filed?

>+        this.mediaURL = this.target.src;

I wonder whether video/audio should implement nsIImageLoadingContent... That has the advantage of getting the actual URI in the case of redirects, IIRC.

>+  saveMedia: function() {

>+      saveURL(this.mediaURL, null, dialogTitle, false,
>                    false, doc.documentURIObject);

Fix indent here?

>+  mediaCommand : function CM_mediaCommand(command) {

>+    switch (command) {

>+      case "mute_toggle":
>+        media.muted = !media.muted;
>+        break;
>+        break;

2 breaks?

>+      case "showcontrols_toggle":
>+        // XXX reenabling the controls doesn't work for some reason.
>+        if (media.hasAttribute("controls"))
>+            media.removeAttribute("controls");
>+        else
>+            media.setAttribute("controls", "true");
>+        break;

Is there a bug filed on this?

>+  copyMediaLocation : function () {

This could just use nsIClipboardHelper's copyStringToClipboard.

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

Have you verified that this doesn't introduce any accesskey conflicts? It's kind of a pain to determine all of the possible combinations...

It would be nice to get a test for these items. One way to do it is to write a browser chrome test that instantiates a nsContextMenu, attaches it to a given target, and checks its members/methods (see e.g. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug423833.js ).

Comment 25

[Reed Loden [:reed]]

(In reply to comment #24)
> (AMO MXR should help in the future, if that
> ever happens).

It's happening very soon!

Comment 26

[Justin Dolske [:Dolske]]

(In reply to comment #24)

> I'm a bit concerned that renaming imageURL and saveImage/viewImage will
> unnecessarily break a bunch of extensions

I think my first pass at this left those in place (as compat wrappers for saveMedia/mediaURL), I can look at adding them back. I don't remember if I took them out for simplicity, or if there was some icky complication.

> >+    // XXX can't yet load a video directly -- bug 448603
> >+    this.showItem("context-viewvideo", this.onVideo);
> 
> So should we disable this for now?

Yeah, might as well, since otherwise its purpose will be a bit misleading.

> >+        // XXX I think this is buggy, removeAttribute doesn't work for me.

See comment 15. It WFM now, not sure what was originally happening.

> >+        this.mediaURL = this.target.src;
> 
> I wonder whether video/audio should implement nsIImageLoadingContent... That
> has the advantage of getting the actual URI in the case of redirects, IIRC.

Looking at that interface, it doesn't really seem appropriate. Perhaps something else could be used.

What's the purpose of this for images? The only useful case I can think of offhand is for things like <img src="random_redirect.cgi">...

> >+        break;
> >+        break;
> 
> 2 breaks?

Oops oops.

> >+      case "showcontrols_toggle":
> >+        // XXX reenabling the controls doesn't work for some reason.
> 
> Is there a bug filed on this?

This was fixed by bug 448909 (the binding is always attached now).

> Have you verified that this doesn't introduce any accesskey conflicts? It's
> kind of a pain to determine all of the possible combinations...

Hmm, I'll check. Might be easiest to add this to a test... Attach a context ment to a video/image/iframe, look to see if the displayed items are those expected, and check if the access keys for those items are all unique. (I'm assuming two different menuitems can have the same access keys, so long as they're never shown at the same time?).

> It would be nice to get a test for these items. One way to do it is to write a
> browser chrome test that instantiates a nsContextMenu, attaches it to a given
> target, and checks its members/methods

Yeah. I had been thinking of a mochitest with a page of assorted interesting elements, then triggering a context menu on each. Same basic idea...

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #26)
> What's the purpose of this for images? The only useful case I can think of
> offhand is for things like <img src="random_redirect.cgi">...

Yeah, it's not a big deal.

> (I'm assuming two different menuitems can have the same access keys, so long
> as they're never shown at the same time?).

Yep.

Comment 28

[Tony Chung [:tchung]]

adding in-litmus ?

Comment 29

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

* Addressed review nits
* Added CSS for GTK menuitem icons
* Got rid of type=checked menuitems (now Show / Hide Controls, Mute / Unmute)
* Added compat wrappers for previously-removed saveImage() / sendImage().
* OMG TESTS

The tests also check for duplicated access keys. I caught a few cases and fixed them:

  - On <canvas>, "Save Image" conflicted with "View Source", and "View Image" conflicted with "View Info". Fixed by not showing "View Source" or "View Info" (images don't have these entries).
  - On <video>, "Play" conflicted with "Properties", and "Mute" conflicted with "Bookmark This Page". Fixed by removing "Bookmark This Page" (which isn't on images either) and "Properties" (which needed more code to hook up showing info anyway. talked with mconnor about this a while ago, and it wasn't clear if we really want to continue supporting/extending that feature -- it's very barebones, and not useful in comparison to modern tools like Firebug).

I next want to make sure these tests work on Linux and Windows.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 343357 [details] [diff] [review]
Patch v.5

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>   initSaveItems: function CM_initSaveItems() {

>-    // Save image depends on whether we're on a loaded image, or a canvas.
>+    // Save image depends on having loaded it, other type don't need to.

"other type don't need to"?

>+  // Backwards-compatability wrapper
>+  saveImage : function() {
>+    if (this.onCanvas || this.onImage)
>+        this.saveMedia();
>+  },

>+  sendImage : function() {
>+    if (this.onCanvas || this.onImage)
>+        this.sendMedia();
>+  },

Neither of these match the previous behavior when called while not on an image, but I guess that doesn't make too much sense anyways.

I wonder whether it makes sense to add a getter for imageURL that returns mediaURL if (onCanvas || onImage). I also wonder whether my fears of extension compat problems are unwarranted - I can't think of what the use cases are for extensions using these, but maybe that's just my lack of imagination.

>diff --git a/browser/base/content/test/test_contextmenu.html b/browser/base/content/test/test_contextmenu.html

>+function getVisibleMenuItems(aMenu) {

>+            // for expextedItems and actualItems.

typo expextedItems

>+  setTimeout(runTest, 50, testNum + 1); // XXX 40ms was too slow, why? XXX

Surely this will cause sporadic unit test failures? You could avoid this by explicitly creating nsContextMenu objects rather than relying on the popupshowing handler to do it. Its less realistic, but still tests that majority of the context menu code.

Comment 31

[Justin Dolske [:Dolske]]

(In reply to comment #30)

> I wonder whether it makes sense to add a getter for imageURL that returns
> mediaURL if (onCanvas || onImage). I also wonder whether my fears of extension
> compat problems are unwarranted - I can't think of what the use cases are for
> extensions using these, but maybe that's just my lack of imagination.

It seems kind of unlikely that extensions would be using any of these, but the wrappers are easy enough to have so it doesn't matter to me. I don't think a imageURL really helps with much either. That said, there's a fair amount of pre-existing redundancy and spaghetti code that could probably be cleaned up. :/

> >+  setTimeout(runTest, 50, testNum + 1); // XXX 40ms was too slow, why? XXX
> 
> Surely this will cause sporadic unit test failures?

Oops, that's just a cut'n'paste leftover from when I copied the guts of password manager's test_basic_form_autocomplete.html. I don't know if a lower value has problems in this test or not. [The pwmgr test hasn't caused problems in the past, fwiw.]

I'd like to keep the test as hi-fi as possible, and I'm already kind of sad that openContextMenuFor() can't just fire a mouse right-click event. :( I should probably file a bug on that.

Comment 32

[Robert Kaiser]

IIRC, any usage of timeouts is known to intermittently cause failures on unit test VMs.

Comment 33

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

Updated nits, and changed test a bit to be driven by the popupshown event instead of timeouts. (Dropping the timeout value was fine, but then I started getting the menu in the "showing" state instead of "open", so I just got rid of it entirely)

Comment 34

[Justin Dolske [:Dolske]]

Attachment: Screenshot of patch v.6 on Linux

Comment 35

[Jennifer Morrow [:Boriss] (UX)]

Looks good, but how about we make that "Hide Controls" rather than "Hide Media Controls?"  I think the inclusion of "media" is unnecessary and inconsistent, and removing it makes the context menu easier to scan.

Comment 36

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 344215 [details] [diff] [review]
Patch v.6

Looks good

Comment 37

[Tony Chung [:tchung]]

can we get this landed in time for tomorrow's build?  I'm eager to get as much time for testing this feature before we hit beta 2.

Comment 38

[Justin Dolske [:Dolske]]

Yes, I've been waiting for tinderboxes to green up.

Comment 39

[Justin Dolske [:Dolske]]

Pushed changeset 50053981fd5d (Patch v.6 + change from comment 35)

Comment 40

[cajbir (:cajbir)]

Sorry for the late comment, I didn't know about this bug.

+      else if (this.target instanceof HTMLVideoElement) {
+        this.onVideo = true;
+        this.mediaURL = this.target.src;
+      }
+      else if (this.target instanceof HTMLAudioElement) {
+        this.onAudio = true;
+        this.mediaURL = this.target.src;

Should this.target.src be this.target.currentSrc?

.currentSrc is the actual URL playing. It can be different from .src if the video/audio has <source> elements.

Comment 41

[Tony Chung [:tchung]]

adding in-litmus+ back.  there are front end pieces to this that we can write testcases and post to bfts.

Comment 42

[Tony Chung [:tchung]]

adding in-litmus+ back.  there are front end pieces to this that we can write testcases and post to bfts and ffts section

Comment 43

[Tony Chung [:tchung]]

Added litmus tests:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7056
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7057
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7058
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7060
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7063
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7064
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7065
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7066

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need a followup bug for comment #40.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Dolske filed/fixed bug 461810.

Comment 46

[Biju]

thanks all, 
with this context menu and the ui controls the <VIDEO> is at least presentable to public.

Comment 47

[Hans Schmucker]

I'm afraid that with "Save As" present, no content provider will touch the video element, as it allows content to be saved so easily. An average user might not even be aware that there is a difference between being allowed to view the video and being allowed to distribute it. Sure, IMG has the same problems, but there's no real alternative to it, so providers have resolved to watermarks and the like. But for video, there is the established and accepted way of using Flash and if we want to even capture a bit of that market, I think we should remove "Save Video As". The functionality can stay and this is really the reason why I'm not adding a separate bug, but I don't think exposing it this way will help acceptance of VIDEO. Besides "Copy URL" allows for basically the same functionality, but allows providers at least a bit of control (not really, but "Save As" is really a bit of a kick in the... you know where).

Alternatively, the spec could be modified to allow for a "Saveable" attribute.

Comment 48

[Tony Chung [:tchung]]

marking bug as verified. 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081113 Minefield/3.1b2pre

Comment 49

[Mike Shaver (:shaver emeritus)]

This caused bug 497098, which is a serious extension compatibility break. :-(