Closed Bug 463155 Opened 11 years ago Closed 11 years ago

Add video/audio controls to message context menu, since the regular controls depend on JavaScript

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: JoeS1, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [has l10n impact])

Attachments

(2 files, 1 obsolete file)

The video tag works fine in trunk mailnews.
This includes the "autoplay" attribute.
STR:
Paste this code into an email or news post:
<video src="http://www.double.co.nz/video_test/ascannerdarkly480.ogg"
 autoplay="true"></video>

Open the composition

The video plays (a good thing)
No controls to stop the video (a bad thing)

This is because the controls were implemented in XBL ??

Bug 461136 ported the Firefox context menu to SeaMonkey, maybe that's an answer to the lack of controls in TB
Component: General → Mail Window Front End
OS: Windows XP → All
QA Contact: general → front-end
Hardware: PC → All
Dup of bug 449358?
Only up until the last sentence of comment 0, which given the conclusion of bug 449358 for 1.9.1, is a blocker.
Assignee: nobody → philringnalda
No longer blocks: 453943
Status: NEW → ASSIGNED
No longer depends on: 453928
Flags: blocking-thunderbird3+
Summary: Video tag "controls" depend on Javascript → Add video/audio controls to message context menu, since the regular controls depend on JavaScript
Target Milestone: --- → Thunderbird 3.0b3
I think the minimum solution we need is some way to disable the autoplay of this element, providing a context menu still requires that someone find the menu and use it..

For added fun I tried out the audio version and you get the same effect, except there is no visible object to interact with.

<audio src="http://www.double.co.nz/video_test/ascannerdarkly480.ogg"
 autoplay="true"></audio>

Stopping the autoplay functionality of both these elements would solve the problem of possibly receiving messages with video or audio playback that can't be stopped.
I had several paragraphs typed about how disabling autoplay (which is bug 449531, with zero traction, or something better scoped that isn't yet filed) without having a context menu is actually worse than just disabling audio/video, since you then have no way of playing it at all, just the tease of "it's supported, it just doesn't ever play," but then what you said about no visible object finally clicked: with autoplay, that means ten million griefers mass-mailing <audio src="imlookingatgayporn.ogg" autoplay>, without autoplay it means practically speaking we don't support audio.

Lightly edited, from a conversation with bz:

<bz>: You could do an evil hack, like have some chrome that detects <video>s in the mail and injects controls and listens for events using a listener in chrome to detect clicks on those controls and scripts the <video> elements accordingly.

<philor>: you're reminding me of my only experience with that sort of thing, where the feed preview page put a little chrome in content, and moz_bug_r should have made several hundred dollars off all the bugs he's filed on it 

<bz>: it injected a chrome js object into content, I'm just suggesting injecting DOM nodes and not having any script running in content (which was the other difference with the preview page) and not injecting _chrome_ dom nodes. You create the DOM nodes using createElement on the content document. You then insert them in the content DOM. I think this would be a _lot_ safer than what feeds did. The only script running is your chrome script. It only listens for trusted mouse events (so ones that actually correspond to user clicks), that sort of thing. It might be a bit annoying to implement, but it should be doable....
• bz hates giving advice like this
<bz>: I feel like I'm suggesting that you do things in the most backwards way possible. Unfortunately, the non-backwards ways are broken, some by design

<philor>: sadly, I don't know who we can assign it to, I only know who we can assign "+MOZ_MEDIA=" to

So, this bug needs an owner and a resummary, but I don't have any idea who that owner is. I can keep it and use it as the bug where we stop building MOZ_MEDIA, but that's all I can promise to have time for.
Status: ASSIGNED → NEW
Whiteboard: [needs owner]
Well, "back in the day" when I became very excited about Gecko including video tag support, I used this script/markup to add a UI to the infant tag.

<h1>Ogg Theora Video</h1>
<script type="text/javascript">
function time_update(v, t) {
  t.innerHTML=v.currentTime; 
  if(!v.paused) 
    setTimeout(function() { time_update(v, t); }, 1000);
}
  
function play(video, time) {
  var v = document.getElementById(video);
  var t = document.getElementById(time);
  var s = document.getElementById('file').value;
  if(!v.src)
    v.src = s;
  v.play();
  if(time) 
    time_update(v, t);
}
function pause(video) {
  var v = document.getElementById(video);
  v.pause();
}
</script>

<table>
  <tbody>
    <tr>
      <td>
      <select id="file">
      <option value="transformers320.ogg">Low Resolution</option>
      <option value="transformers480.ogg" selected="selected">Medium
Resolution</option>
      <option value="transformers640.ogg">High Resolution</option>
      </select>
      </td>
    </tr>
    <tr>
      <td><button onclick="play('v1', 't1'); return false;">Play</button>
      <button onclick="pause('v1'); return false;">Pause</button> </td>
    </tr>
  </tbody>

</table>
<table border="1">
  <tbody>
    <tr>
      <td><video playbackrate="1" id="v1"
 src="http://www.double.co.nz/video_test/transformers480.ogg"></video> <br>
      <p>Current Position is: <span id="t1">0.00</span> </p>
      </td>
    </tr>
  </tbody>
</table>

This created a very nice control console.

I said "back in the day" because this sort of thing will be impossible with JS now disabled.

As far as removing MOZ_MEDIA from the build, I think that is just a cop-out in the same vein as plugins support was removed from TB2 branch.

BTW, Has everybody forgotten about View>>>Message body as>>>Simple HTML as a way of controlling what the recipient sees.

Making this option active by default could help.
(Really not sure what the default is, as I'm not a "simple HTML" type)
Depends on: 478982
As a workaround, we should very much do

+pref("media.autoplay.enabled", false)

somewhere in the prefs when bug 478982 lands.
bug 478892 landed on mozilla-central, but not yet in 1.9.1

No reason not to add the override pref to mail/app/profile/all-thunderbird.js now.

The only thing that bothers me is, messages/newsgroup posts will still show a 
media window, with no way to start the media, and leave the user totally clueless.

Although I am loathe to say this "A partially supported feature can be worse that no feature at all"

pref("media.ogg.enabled", false);
pref("media.wave.enabled", false);

While the above "might" eliminate all appearance of the media, my guess would
be that we would simply ignore that message content completely.
Which is the case where you have plugins disabled, and the embed tag in a message points to a remote source. (You never know that content is there)

At the very least, we should inform the user that some content was not rendered
If not a popup, then at least an entry in the error console.
(In reply to comment #7)
> bug 478892 landed on mozilla-central, but not yet in 1.9.1
> 
> No reason not to add the override pref to mail/app/profile/all-thunderbird.js
> now.

I assume you meant bug 478982. I wonder how seamonkey want to tackle that pref...
Yes, bug 478982 (thx magnus) has now landed in 1.9.1, so some testing:
Setting media.autoplay.enabled to false does in fact stop the playing.

The bad news is, that it does not block the media frame.
So you get the "remote content frame" regardless of your setting of
mailnews.message_display.disable_remote_image
(that could be another bug, as I thought that pref should block all remote content, image and other content as well)

If I set media.ogg.enabled to "false", the video tag is completely ignored.

But I think we owe it to the user to let them know when content is ignored because of a hidden pref setting. (error console notification at least)

Maybe there is an application here for the Activity Manager.
Like "Video blocked due to prefs settings"
(In reply to comment #9)

> The bad news is, that it does not block the media frame.
> So you get the "remote content frame" regardless of your setting of
> mailnews.message_display.disable_remote_image
> (that could be another bug, as I thought that pref should block all remote
> content, image and other content as well)

The fact that it attempts to fetch the content if the src attribute is remote is definitely a separate bug.  If you could spin that off and CC me on it, that would be much appreciated.

> If I set media.ogg.enabled to "false", the video tag is completely ignored.
> 
> But I think we owe it to the user to let them know when content is ignored
> because of a hidden pref setting. 

Some sort of notification does seem in order.  We could conceivably display a dummy media frame with informational text inside and perhaps a link to override the default of not displaying the remote contents.  I'll be interested to hear Bryan's thoughts on this.
(In reply to comment #10)
> > But I think we owe it to the user to let them know when content is ignored
> > because of a hidden pref setting. 
> 
> Some sort of notification does seem in order.  We could conceivably display a
> dummy media frame with informational text inside and perhaps a link to override
> the default of not displaying the remote contents.  I'll be interested to hear
> Bryan's thoughts on this.

I think this just depends on what we want to do and can do for the person.  There is a striking similarity between this and the remote images system.  I don't think we'd actually want to offer for people to turn on remote video completely as much as we'd want to allow people to turn it on - just now or always for this person.  Not sure if those aspects are possible.
(In reply to comment #11)

> 
> I think this just depends on what we want to do and can do for the person. 
> There is a striking similarity between this and the remote images system.  I
> don't think we'd actually want to offer for people to turn on remote video
> completely as much as we'd want to allow people to turn it on - just now or
> always for this person.  Not sure if those aspects are possible.

I think we have exactly the same scenario here as in the case of remote images.
The fixes there (address book and universal pref) should be used here as well.

In the mean time, since the pref "media.ogg.enabled" is set to true by default, that means that any mail with the tag can be confirmed as a "valid address" in the same way as a remote image, whether autoplay is enabled or not.
(Although, I haven't heard of any instances of this in the wild.)

We should probably have a spinoff bug to address the possibilities.
"Review content policy for Video/Audio tags"

I think it's important for TB to stay current for HTML5 and stay on the leading edge.
Filed bug 480674 for a content policy
Assignee: philringnalda → nobody
Assignee: nobody → dolske
Whiteboard: [needs owner]
Target Milestone: Thunderbird 3.0b3 → Thunderbird 3.0b4
Whiteboard: [has l10n impact]
I've pinged Justin but not heard back, I think dmose may have as well. So stealing this bug as we need to get a patch up for review before the string freeze - I should have a patch ready in a few hours.
Assignee: dolske → bugzilla
Whiteboard: [has l10n impact] → [has l10n impact][patch almost ready]
Adds Play/Pause and Mute/Unmute controls. When you right-click a video element, the normal context menu will be replaced by play/pause and mute/unmute options depending on the state of the video.

We could potentially add Copy Location and Save As items as well, but I don't see that as blocking and they could probably be added via an extension later if necessary. I'll file follow-up bugs for those.

I'm still examining the audio element I'm not quite sure what we can do there yet.
Attachment #399091 - Flags: ui-review?(clarkbw)
Attachment #399091 - Flags: review?(philringnalda)
Attached image Screen shot
Comment on attachment 399091 [details] [diff] [review]
Add Play/Pause, Mute/Unmute controls

this looks good for the video element.
Attachment #399091 - Flags: ui-review?(clarkbw) → ui-review+
(In reply to comment #15)
> I'm still examining the audio element I'm not quite sure what we can do there
> yet.

Should we be packaging some default <audio> graphic that we can make the target for right clicking?  I'm not sure if that's possible.
(In reply to comment #18)
> (In reply to comment #15)
> > I'm still examining the audio element I'm not quite sure what we can do there
> > yet.
> 
> Should we be packaging some default <audio> graphic that we can make the target
> for right clicking?  I'm not sure if that's possible.

Or even a text character &#9835 looks appropriate
Blocks: 515082
Having looked at the audio element a bit more:

- if controls aren't enabled, then the audio element can't be controlled (even in Firefox)
- if the controls are shown, then the audio element takes up space but still can't be controlled via the context menu.

This is possibly in part due to bug 449358 although we may be able to find ways around it. However we're not likely to do that before b4, and probably unlikely before TB 3 as well, so I'm deferring sorting out the audio element properly to bug 515082.

We have agreed, however, that autoplay should be disabled by default so that the audio element effectively can't be played at all in email messages (audio elements in content tabs will work correctly as javascript is enabled there). However, extensions could still try and provide UI to control audio elements.

Therefore this patch has the additional preference change to disable autoplay by default.

I meant to say earlier for reviewer's benefit - the context menu code in TB appears to be a bit of a mess and we should probably tidy it up some time. Firefox has it all nicely in nsContextMenu.js, Thunderbird seems to be shared between two files :-(
Attachment #399091 - Attachment is obsolete: true
Attachment #399111 - Flags: review?(philringnalda)
Attachment #399091 - Flags: review?(philringnalda)
Whiteboard: [has l10n impact][patch almost ready] → [has l10n impact][needs review philor]
Comment on attachment 399111 [details] [diff] [review]
Add Play/Pause, Mute/Unmute controls v2

>diff --git a/mail/locales/en-US/chrome/messenger/messenger.dtd b/mail/locales/en-US/chrome/messenger/messenger.dtd
>--- a/mail/locales/en-US/chrome/messenger/messenger.dtd
>+++ b/mail/locales/en-US/chrome/messenger/messenger.dtd
>@@ -684,6 +684,16 @@ you can use these alternative items. Oth
> <!ENTITY contextPrintPreview.label "Print Preview">
> <!ENTITY contextPrintPreview.accesskey "v">
> 
>+<!-- Media (video/audio) controls -->
>+<!ENTITY contextPlay.label "Play">
>+<!ENTITY contextPlay.accesskey "P">
>+<!ENTITY contextPause.label "Pause">
>+<!ENTITY contextPause.accesskey "P">
>+<!ENTITY contextMute.label "Mute">
>+<!ENTITY contextMute.accesskey "M">
>+<!ENTITY contextUnmute.label "Unmute">
>+<!ENTITY contextUnmute.accesskey "m">

Does using the same accesskeys for Play/Pause and Mute/Unmute really work? Is only one part of the combination ever active?
(In reply to comment #21)
> Does using the same accesskeys for Play/Pause and Mute/Unmute really work? Is
> only one part of the combination ever active?

If you look in the initMediaPlayerItems function, you'll see these are exclusive items. There isn't any point in putting up Mute and Unmute at the same time anyway (I suppose you could disable an option, but that's not necessary).

Also (before anyone asks ;-) ) when right-clicking on a video these items are the only ones available in the context menu which is why we can also use access keys that are used elsewhere in the menu.
Comment on attachment 399111 [details] [diff] [review]
Add Play/Pause, Mute/Unmute controls v2

>+    // Several mutall exclusive items.... play/pause, mute/unmute, show/hide

Maybe people will be more likely to realize why we can use the same accesskeys if you choose a word other than "mutall" ;)
Attachment #399111 - Flags: review?(philringnalda) → review+
Checked in with comment fixed: http://hg.mozilla.org/comm-central/rev/51ff72b37ec3
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [has l10n impact][needs review philor] → [has l10n impact]
I've added a note to devmo TB 3 page that we've now "enabled" this element:

https://developer.mozilla.org/En/Thunderbird_3_for_developers
(In reply to comment #22)
> (In reply to comment #21)
> > Does using the same accesskeys for Play/Pause and Mute/Unmute really work? Is
> > only one part of the combination ever active?
> 
> If you look in the initMediaPlayerItems function, you'll see these are
> exclusive items. There isn't any point in putting up Mute and Unmute at the
> same time anyway (I suppose you could disable an option, but that's not
> necessary).

It's still a bad idea to use the same accesskey for two strings - not for en-US but for localizations. The translation of "mute" and "unmute" to a foreign language might end up with strings that don't share any character!
So it's a good thing Mark isn't stupid, and didn't do that, isn't it?
Oh, right, I should better read patches and not just comments ;-)
You need to log in before you can comment on or make changes to this bug.