701785 - Missing html5 video/audio controls in context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Tony Chung [:tchung]]

There seems to be a handful of missing context menu items for HTML5 video window.

- Play/Pause
- Save Video
- Share Video

Only Full screen is there.   These are available on XUL Fennec

Repro:
1) install Fennec Native nightly, 20111111 build
2) visit URL or a site that has HTML5 video samples
3) play video via webpage controls
4) context click a video.  verify missing context menu options listed above

Expected:
- main video controls appear in context click menu

ActuaL:
- only full screen is an option for video controls

Comment 1

[Aaron Train [:aaronmt]]

Wes, dupe of 699537?

Comment 2

[Aaron Train [:aaronmt]]

Yeah they're listed in bug 699537.

Comment 3

[Tony Chung [:tchung]]

reopening, for ease of tracking context menu work (via wesj)

adding blocking bug 699537

Comment 4

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

Sriram - we have code in XUL fennec that shows how to implement these menu items.

Comment 5

[Cristian Nicolae (:xti)]

The context menu options are missing, except Full Screen if a long tap is performed over a HTML5 video.

--
Firefox 16.0b1 (2012-08-28)
Device: Galaxy Note
OS: Android 4.0.4

Comment 6

[:Margaret Leibovic]

Looks like Sriram hasn't been working on this, but it would make a good mentor bug, so I'm marking it as such.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Renominating based on that we are getting more <video> support. Without this bug fixed users cannot save video/audio/

Comment 9

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

Attachment: patch

This patch adds: "Play", "Pause" and "Share Video" to the context menu. Both "Play" and "Pause" should appear for video and audio. "Share Video" only works for video.

"Play" and "Pause" are exclusive, so only one is visible at a time, depending on the state of the media element.

I tested this change on webm and ogv, both inline and fullscreen. Seems to work OK.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 684297 [details] [diff] [review]
patch

Review of attachment 684297 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +1463,5 @@
> +            if (paused && aMode == "media-paused")
> +              return true;
> +            if (!paused && aMode == "media-playing")
> +              return true;
> +          }

Desktop also checks for load errors here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#395

Probably should do the same thing here.

Comment 11

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

Attachment: patch 2

Added the "hasError" check.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 685877 [details] [diff] [review]
patch 2

Review of attachment 685877 [details] [diff] [review]:
-----------------------------------------------------------------

Seems good to me!

The desktop implementation just sets these items disabled. We actually support that, but I don't think there's anything in the context menu api for it, and it seems like a waste of space on small screens.

Comment 13

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

(In reply to Wesley Johnston (:wesj) from comment #12)

> The desktop implementation just sets these items disabled. We actually
> support that, but I don't think there's anything in the context menu api for
> it, and it seems like a waste of space on small screens.

Right. I saw no reason to show them disabled.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0d354ed028

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bc0d354ed028

Comment 16

[Andreea Pod]

I cannot reproduce this issue on the latest Nightly and Aurora. Closing bug as verified fixed on:
Firefox for Android
Version: 21.0a1 (2013-02-10)
Device: HTC Desire Z
OS: Android 2.3.3