461136 - <video> context menu for SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

Bug 449522 implements a context menu for <video> in Firefox only, we should port this to the SeaMonkey browser.

Comment 1

[Krang]

Currently working on this.  The patch will be ready within the next few days.

Comment 2

[Robert Kaiser]

Thanks! Be sure to incorporate the change of bug 461810, btw.

Comment 3

[Philip Chee]

Adding to the radar Bug 461306 -  "Mute" / "Unmute" and "Show Controls" / "Hide Controls" in <video> context menu should be checkboxes instead

Comment 4

[Krang]

Attachment: Context menu patch

Here is the patch.  A few notes:

- The changes from bug 461810 have been included.
- In the Firefox patch, backwards-compatibility wrappers were included for both the saveImage and sendImage functions.  However, since our sendImage function is located in mailNavigatorOverlay.xul (and probably not used by any extensions), I didn't include the backwards-compatibility wrapper for it, although I could add one if it would be useful.
- Since we use the "C" access key for "Copy Link Location", the access key for "Show/Hide Media Controls" had to be changed.

Comment 5

[Philip Chee]

> However, since our sendImage function
> is located in mailNavigatorOverlay.xul (and probably not used by any
> extensions), I didn't include the backwards-compatibility wrapper for it,
> although I could add one if it would be useful.

If this wrapper doesn't take up too much bloat it would be make it easier for extension authors porting their Firefox extensions to SeaMonkey.

Comment 6

[[e-mail]]

(In reply to comment #4)
> I didn't include the backwards-compatibility wrapper for it,
> although I could add one if it would be useful.
I don't think so, since we lost nsContextMenu.imageURL anyway.

> - Since we use the "C" access key for "Copy Link Location", the access key for
> "Show/Hide Media Controls" had to be changed.
I keep meaning to rework all the access keys...

Comment 7

[[e-mail]]

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

>-      var shouldShowSendPage = !(gContextMenu.onTextInput || 
>-                               gContextMenu.isContentSelected) && 
>+      var shouldShowSendPage = !(gContextMenu.onTextInput || gContextMenu.isContentSelected ||
>+                               gContextMenu.onVideo || gContextMenu.onAudio) && 
Nit: you only fixed one trailing space

>+    mediaCommand : function CM_mediaCommand(command) {
Nit: We don't seem to name member functions in this file, but even if we did we would not prefix with CM_.

r=me with those nits fixed.

Comment 8

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #4)
> > I didn't include the backwards-compatibility wrapper for it,
> > although I could add one if it would be useful.
> I don't think so, since we lost nsContextMenu.imageURL anyway.
Hmm, so that doesn't matter because it's an internal variable.

Comment 9

[Justin Wood (:Callek)]

(In reply to comment #4)
> - In the Firefox patch, backwards-compatibility wrappers were included for both
> the saveImage and sendImage functions.  However, since our sendImage function
> is located in mailNavigatorOverlay.xul (and probably not used by any
> extensions), I didn't include the backwards-compatibility wrapper for it,
> although I could add one if it would be useful.

Per my scanning of the attachment it does look like you did include it, or am I wrong?

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

Comment 10

[Krang]

Attachment: Context menu patch v1.1 [Checkin: Comment 14]

Here is an updated patch.  Carrying over Neil's r+.

(In reply to comment #7)
> Nit: you only fixed one trailing space

Fixed.

> Nit: We don't seem to name member functions in this file, but even if we did we
> would not prefix with CM_.

Fixed.

I also noticed that I forgot a space in the function definition for initMediaPlayerItems, so I added one in this patch.

(In reply to comment #5)
> If this wrapper doesn't take up too much bloat it would be make it easier for
> extension authors porting their Firefox extensions to SeaMonkey.

Good point, but sendImage currently can't be called from an instance of nsContextMenu anyway.  For better compatibility, it would probably be best to have wrappers for both sendImage and sendMedia within nsContextMenu, although I'm not sure how to do that without moving the entire openComposeWindow function as well.

(In reply to comment #9)
> Per my scanning of the attachment it does look like you did include it, or am I
> wrong?

I did keep the saveImage wrapper, but I did not include the sendImage wrapper for reasons mentioned above.

Comment 11

[Robert Kaiser]

Should we integrate bug 462294 here as well? Looks like an easy addition to this...

Comment 12

[[e-mail]]

Comment on attachment 345654 [details] [diff] [review]
Context menu patch v1.1
[Checkin: Comment 14]

And then there's the issue of the checkbox menuitems (I think ✔Mute works well here, but I'm not sure about the other options). I think it would probably be best to land this now and then visit further enhancements in a new bug.

Comment 13

[Krang]

(In reply to comment #11)
> Should we integrate bug 462294 here as well? Looks like an easy addition to
> this...

(In reply to comment #12)
> And then there's the issue of the checkbox menuitems (I think ✔Mute works well
> here, but I'm not sure about the other options). I think it would probably be
> best to land this now and then visit further enhancements in a new bug.

Thanks, I'll file a follow-up bug to port bug 462294 and bug 461306 to SeaMonkey.  On bug 461306, should we wait for Firefox, or should we just go ahead and make those two items checkboxes?

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 345654 [details] [diff] [review]
Context menu patch v1.1
[Checkin: Comment 14]

http://hg.mozilla.org/comm-central/rev/67f13193a501

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #13)
> (In reply to comment #12)
> > And then there's the issue of the checkbox menuitems (I think ✔Mute works well
> > here, but I'm not sure about the other options). I think it would probably be
> > best to land this now and then visit further enhancements in a new bug.
> 
> Thanks, I'll file a follow-up bug to port bug 462294 and bug 461306 to
> SeaMonkey.  On bug 461306, should we wait for Firefox, or should we just go
> ahead and make those two items checkboxes?

My opinion is to just go for it; if you're feeling ambitious you can probably fix FF in the same patch (see also c#2 on Bug 461306, Neil may differ in opinion vs that anyway).

Comment 16

[Krang]

(In reply to comment #15)
> My opinion is to just go for it; if you're feeling ambitious you can probably
> fix FF in the same patch (see also c#2 on Bug 461306, Neil may differ in
> opinion vs that anyway).

Thanks for the feedback (and thanks to Serge for the checkin).  I went ahead and filed bug 462714, so I'll add a patch that takes care of both bugs there.

Comment 17

[Philip Chee]

>> I didn't include the backwards-compatibility wrapper for it,
>> although I could add one if it would be useful.
> I don't think so, since we lost nsContextMenu.imageURL anyway.

Bug 497098 just added back nsContextMenu.imageURL to Firefox. Apparently there are extensions depending on this.