Krang Assignee
	Description • 16 years ago

The improvements made in Firefox to the <video> context menu in bug 462294 and bug 461306 should be ported to SeaMonkey.

	Krang Assignee
	Comment 1 • 16 years ago

Here is the patch.  I haven't done anything regarding bug 461306 comment #2 yet, but if it is decided that we don't want the checkmarks for Mac in SeaMonkey, I can look into that.

	neil@parkwaycc.co.uk
	Comment 2 • 16 years ago

Comment on attachment 345930 [details] [diff] [review]
Context menu patch

>+      <menuitem id="context-viewvideo"
>+                label="&viewVideoCmd.label;"
>+                accesskey="&viewVideoCmd.accesskey;"
>                 oncommand="gContextMenu.viewMedia();"/>
Ideally this would be hidden for full-page video but I was unable to determine whether it was possible to detect that. (Might be worth a backend bug?)

>         this.showItem( "context-media-sep-commands", onMedia );
>+
>+        if (onMedia) {
>+          var mutedMenu = document.getElementById("context-media-mute");
>+          mutedMenu.setAttribute("checked", this.target.muted);
>+          var controlsMenu = document.getElementById("context-media-showcontrols");
>+          controlsMenu.setAttribute("checked", this.target.controls);
>+        }
This file uses 4-space indentation.

>+          case "mute_toggle":
>+            media.muted = !media.muted;
>             break;
>+          case "showcontrols_toggle":
>+            if (media.hasAttribute("controls"))
>+                media.removeAttribute("controls");
>+            else
>+                media.setAttribute("controls", "true");
Does media.controls = !media.controls not work? Also the indentation of the cases is wrong, good thing you're changing most of the code anyway ;-)

	neil@parkwaycc.co.uk
	Comment 3 • 16 years ago

(In reply to comment #2)
> This file uses 4-space indentation.
On second thoughts, the indentation is a mess :-(

	neil@parkwaycc.co.uk
	Comment 4 • 16 years ago

Comment on attachment 345930 [details] [diff] [review]
Context menu patch

OK, so r=me if you use media.controls

	Krang Assignee
	Comment 5 • 16 years ago

(In reply to comment #2)
> Ideally this would be hidden for full-page video but I was unable to determine
> whether it was possible to detect that. (Might be worth a backend bug?)

Bug 462892 was filed recently to deal with this in Firefox, so I'll update the patch here as soon as the backend changes are implemented there.

> Does media.controls = !media.controls not work?

Good idea, it looks like that works just as well.  I will make that change in the next version of the patch.

	Stefan [:stefanh]
	Comment 6 • 16 years ago

(In reply to comment #1)
> Created an attachment (id=345930) [details]
> Context menu patch
> 
> Here is the patch.  I haven't done anything regarding bug 461306 comment #2
> yet, but if it is decided that we don't want the checkmarks for Mac in
> SeaMonkey, I can look into that.

It'd be nice to not have checkbox menuitems since afaik we don't use that in other context menus, but then we need to fix the fit image menuitem as well.

	Stefan [:stefanh]
	Comment 7 • 16 years ago

(forgot to mention that I'm on mac)

	Philip Chee
	Comment 8 • 12 years ago

> The improvements made in Firefox to the <video> context menu in bug 462294 and
> bug 461306 should be ported to SeaMonkey.
The video menu item was eventually added in Bug 483727. Bug 461306 hasn't seen any movement since 2008. If it ever gets fixed we should file a new bug to cover that.