Play or pause a <video> from context menu

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

Bug Flags:
in-litmus +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b2+
Brubeck, you want to mark bug 465839 as a dupe of this, or would you consider it sufficiently different?
(Assignee)

Comment 2

7 years ago
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.
Is anything holding this bug back?
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.
(Assignee)

Comment 5

7 years ago
Created attachment 482260 [details] [diff] [review]
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.
Attachment #482260 - Flags: review?(mark.finkle)
Summary: Play or pause a <video> by tapping on it → Play or pause a <video> from context menu
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"
Attachment #482260 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 7

7 years ago
Created attachment 482355 [details] [diff] [review]
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.
Attachment #482260 - Attachment is obsolete: true
Attachment #482355 - Flags: review?(mark.finkle)
Attachment #482355 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 8

7 years ago
http://hg.mozilla.org/mobile-browser/rev/3e47c85a6515
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?(nhirata.bugzilla)
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13675
Flags: in-litmus?(nhirata.bugzilla) → in-litmus+
You need to log in before you can comment on or make changes to this bug.