Closed Bug 449522 Opened 16 years ago Closed 16 years ago

Context menu for HTML5 <video> elements

Categories

(Firefox :: Menus, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: BijuMailList, Assigned: Dolske)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

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
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.  
(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)
Status: NEW → ASSIGNED
* Email Video/Audio URL
looks like I accidentally ASSIGNED to "nobody".
how do make status back to NEW
Ta-da. :)
Assignee: nobody → dolske
Status: ASSIGNED → NEW
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.
Component: Video/Audio → Menus
Product: Core → Firefox
QA Contact: video.audio → menus
Target Milestone: --- → Firefox 3.1b1
(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
Blocks: video
Summary: Context menu on VIDEO should show propeties and view options → Context menu for HTML5 <video> elements
Attached patch Patch v.1 (obsolete) — Splinter Review
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.
(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
Attached patch Patch v.2 (obsolete) — Splinter Review
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.)
Attachment #336270 - Attachment is obsolete: true
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.
Attached patch Patch v.3 (obsolete) — Splinter Review
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.
Attachment #336408 - Attachment is obsolete: true
Attachment #336746 - Flags: ui-review?(beltzner)
Attachment #336746 - Flags: review?(gavin.sharp)
Attached patch Patch v.4 (obsolete) — Splinter Review
Bah, forgot to qdiff before attaching.
Attachment #336746 - Attachment is obsolete: true
Attachment #336747 - Flags: ui-review?(beltzner)
Attachment #336747 - Flags: review?(gavin.sharp)
Attachment #336746 - Flags: ui-review?(beltzner)
Attachment #336746 - Flags: review?(gavin.sharp)
+ // 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.
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?
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.
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
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'>".
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?
Attached image Screen of last patch + GTK icons (obsolete) —
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.
Depends on: 449360
Flags: blocking-firefox3.1?
(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.
Correct, Fullscreen isn't in the menu because there's no support for it. It can be added then that support does.
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 ).
Attachment #336747 - Flags: review?(gavin.sharp) → review+
(In reply to comment #24)
> (AMO MXR should help in the future, if that
> ever happens).

It's happening very soon!
(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...
(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.
adding in-litmus ?
Flags: in-litmus?
Attached patch Patch v.5 (obsolete) — Splinter Review
* 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.
Attachment #336563 - Attachment is obsolete: true
Attachment #336747 - Attachment is obsolete: true
Attachment #337395 - Attachment is obsolete: true
Attachment #343357 - Flags: review?(gavin.sharp)
Attachment #336747 - Flags: ui-review?(beltzner)
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.
Attachment #343357 - Flags: review?(gavin.sharp) → review+
(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.
IIRC, any usage of timeouts is known to intermittently cause failures on unit test VMs.
Attached patch Patch v.6Splinter Review
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)
Attachment #343357 - Attachment is obsolete: true
Attachment #344215 - Flags: ui-review?(jboriss)
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.
Blocks: 461136
Comment on attachment 344215 [details] [diff] [review]
Patch v.6

Looks good
Attachment #344215 - Flags: ui-review?(jboriss) → ui-review+
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.
Yes, I've been waiting for tinderboxes to green up.
Pushed changeset 50053981fd5d (Patch v.6 + change from comment 35)
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Flags: in-litmus?
Flags: blocking-firefox3.1?
Resolution: --- → FIXED
Target Milestone: Firefox 3.1b1 → Firefox 3.1b2
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.
Blocks: 461306
adding in-litmus+ back.  there are front end pieces to this that we can write testcases and post to bfts.
adding in-litmus+ back.  there are front end pieces to this that we can write testcases and post to bfts and ffts section
Flags: in-litmus+
thanks all, 
with this context menu and the ui controls the <VIDEO> is at least presentable to public.
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.
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
Status: RESOLVED → VERIFIED
Depends on: 467434
Blocks: 470627
Blocks: TrackAVUI
This caused bug 497098, which is a serious extension compatibility break. :-(
You need to log in before you can comment on or make changes to this bug.