Closed Bug 462714 Opened 14 years ago Closed 10 years ago

Improve SeaMonkey's <video> context menu

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 483727

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

The improvements made in Firefox to the <video> context menu in bug 462294 and bug 461306 should be ported to SeaMonkey.
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.
Attachment #345930 - Flags: review?(neil)
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 ;-)
(In reply to comment #2)
> This file uses 4-space indentation.
On second thoughts, the indentation is a mess :-(
Comment on attachment 345930 [details] [diff] [review]
Context menu patch

OK, so r=me if you use media.controls
Attachment #345930 - Flags: review?(neil) → review+
(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.
Depends on: 462892
(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.
(forgot to mention that I'm on mac)
Blocks: TrackAVUI
No longer blocks: TrackAVUI
Depends on: 470627, 467434
Blocks: TrackAVUI
Depends on: 483727
> 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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 483727
You need to log in before you can comment on or make changes to this bug.