590719 - Play or pause a <video> from context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

A single-click on a <video> element should play or pause the video.

We might need to do something different if the web page has its own click event listener on the video element.  If necessary, we can also add a Play/Pause item to the context menu.

Comment 1

[Wesley Johnston (:wesj)]

Brubeck, you want to mark bug 465839 as a dupe of this, or would you consider it sufficiently different?

Comment 2

[Matt Brubeck (:mbrubeck)]

Let's leave it open.  We probably still want controls for video content, since they can include time indicators and seeking controls that are not covered by this bug.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Is anything holding this bug back?

Comment 4

[Wesley Johnston (:wesj)]

This should be easy to implement in the binding of bug 465839. That takes a slightly different approach though. Tapping the video shows/hides the controls, but does not affect the playback.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: Add Play/Pause to the context menu

I think the bug 465839 behavior (showing controls when a video is tapped) is better than playing/pausing immediately, because pages might have their own event listeners to play/pause videos on click, mouseover, mousedown, etc.  We don't want to play the video only to have the page pause it a moment later, or vice versa.

Instead, this patch adds "Play" and "Pause" to the context menu for video and audio elements.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 482260 [details] [diff] [review]
Add Play/Pause to the context menu


>diff -r 6eadac9795e4 -r 17e839778260 chrome/content/content.js

>   init: function ch_init() {
>     addEventListener("contextmenu", this, false);
>+    addEventListener("pagehide", this, false);
>+    addMessageListener("Browser:MediaCommand", this, false);
>+    this._mediaElement = null;
>+  },
>+
>+  reset: function ch_reset() {
>+    this._mediaElement = null;

Looks like you are using _mediaElement as a way to cache the popupNode and "pagehide" as a trigger to clear it. I think I'd rather see ContextHandler.popupNode since it will be useful for other contexts, not just media elements. I also think we should use the "Browser:MouseXxx" message in content.js to reset the popupNode much sooner than pagehide would. We access ContextHandler here (and make the "contextmenu" event):
http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#381

We could find a place in there to call ContextHandler.reset

Let's see if we can not rely on "pagehide"

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

(In reply to comment #6)
> Looks like you are using _mediaElement as a way to cache the popupNode and
> "pagehide" as a trigger to clear it. I think I'd rather see
> ContextHandler.popupNode since it will be useful for other contexts, not just
> media elements.

Done.

> I also think we should use the "Browser:MouseXxx" message in
> content.js to reset the popupNode much sooner than pagehide would. We access
> ContextHandler here (and make the "contextmenu" event):
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#381

Added a "reset" call to the Browser:MouseUp listener.  (It's not needed on MouseDown because now we always set it in the contextmenu handler, which is called on every MouseDown.)

> Let's see if we can not rely on "pagehide"

I left the pagehide listener in place, since I don't want to be sure to clear any references to the old DOM after we navigate to a new page, so it can be garbage collected.

Comment 8

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/3e47c85a6515

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified: Long tap will bring up context menu for play/pause (depending on state) share and save video:

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101014 Firefox/4.0b8pre Fennec/4.0b2pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101014 Firefox/4.0b8pre Fennec/4.0b2pre

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13675