862399 - Add UI to loop playback of audio files

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8750794 [details] [diff] [review]
Patch

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

The summary of this bug is for audio files - does it make sense to have this UI on video files, as the patch does?

r+ but if you redo the patch based on this question and/or the below, happy to have another look.

::: browser/base/content/browser-context.inc
@@ +125,5 @@
> +      <menuitem id="context-media-loop"
> +                label="&mediaLoop.label;"
> +                accesskey="&mediaLoop.accesskey;"
> +                type="checkbox"
> +                oncommand="gContextMenu.mediaCommand('loop');"/>

It feels to me like this could/should be further down the menu? Maybe underneath "Play speed" ?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +557,5 @@
>  <!ENTITY mediaPlay.accesskey         "P">
>  <!ENTITY mediaPause.label            "Pause">
>  <!ENTITY mediaPause.accesskey        "P">
> +<!ENTITY mediaLoop.label             "Loop">
> +<!ENTITY mediaLoop.accesskey         "L">

This access key is in use for "P_lay speed". "o" is in use for Copy Video Location, and p for "Play", so it feels like we should just remove the access key from either play speed or from this item. It doesn't seem like the kind of item that you would repeatedly use and/or toggle...

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #2)
> The summary of this bug is for audio files - does it make sense to have this
> UI on video files, as the patch does?

Yeah, we should have it for both. Video files, such as music videos, may be beneficial to loop, and also for parity with Chrome which also has this present on videos.

> ::: browser/base/content/browser-context.inc
> @@ +125,5 @@
> > +      <menuitem id="context-media-loop"
> > +                label="&mediaLoop.label;"
> > +                accesskey="&mediaLoop.accesskey;"
> > +                type="checkbox"
> > +                oncommand="gContextMenu.mediaCommand('loop');"/>
> 
> It feels to me like this could/should be further down the menu? Maybe
> underneath "Play speed" ?

I went back and forth on the location. I put it higher because I had recently looked at the context menu on Chrome, but I think if we order based on usage, it should be lower. I'll move it below playback speed.

> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +557,5 @@
> >  <!ENTITY mediaPlay.accesskey         "P">
> >  <!ENTITY mediaPause.label            "Pause">
> >  <!ENTITY mediaPause.accesskey        "P">
> > +<!ENTITY mediaLoop.label             "Loop">
> > +<!ENTITY mediaLoop.accesskey         "L">
> 
> This access key is in use for "P_lay speed". "o" is in use for Copy Video
> Location, and p for "Play", so it feels like we should just remove the
> access key from either play speed or from this item. It doesn't seem like
> the kind of item that you would repeatedly use and/or toggle...

I don't know how I didn't notice this in my testing. I would like to keep an access key because one may fit in other locales. I will go with a letter not in "Loop".

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

Play Speed access key was changed from "l" to "d" so Loop can use "L".

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d8df7fcffeb1

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_contextmenu.js failures.
https://hg.mozilla.org/integration/fx-team/rev/b8f01297e8fc

https://treeherder.mozilla.org/logviewer.html#?job_id=9277122&repo=fx-team

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in (test fixed)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Release Note Request (optional, but appreciated)
[Why is this notable]: audio and video can now be looped using the built-in controls
[Suggested wording]: Loop HTML5 audio and video files through the context menu.
[Links (documentation, blog post, etc)]: n/a

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6afc5dab5979

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6afc5dab5979