885506 - Port |Bug 840745 - default html5 audio/video player controls should facilitate changing playback rate|

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

FF allows to adjust the HTML5 audio/video playback rate through the context menu since version 22 (equivalent SM 2.19). Since the feature has been implemented under browser/ instead of toolkit/, it's not available to SM automatically.

Original changeset: http://hg.mozilla.org/mozilla-central/rev/1b54bc328c1d

Porting should be simple.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Actually I changed my mind. We're late with this already and I don't want to miss another release due to the upcoming string freeze, so I went ahead myself.

Comment 2

[Philip Chee]

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

> +      <menu id="context-media-playbackrate"
> +            label="&mediaPlaybackRate.label;"
> +            accesskey="&mediaPlaybackRate.accesskey;">
> +        <menupopup>
> +          <menuitem id="context-media-playbackrate-050x"
> +                    label="&mediaPlaybackRate050x.label;"
> +                    accesskey="&mediaPlaybackRate050x.accesskey;"
> +                    type="radio"
> +                    name="playbackrate"
What's this name for?

> +                    oncommand="gContextMenu.mediaCommand('playbackRate', 0.5);"/>
I suggest putting the oncommand on the menupopup:

<menupopup id="???" oncommand="gContextMenu.mediaCommand('playbackRate', event.target)">

Add a rate attribute to the menuitems e.g. rate="0.5"

> -  mediaCommand: function(aCommand) {
> +  mediaCommand: function(aCommand, aData) {

mediaCommand: function(aCommand, aTarget) 

> +      case "playbackRate":
> +        media.playbackRate = aData;
> +        break;

case "playbackRate":
  media.playbackRate = parseFloat(aTarget.getAttribute("rate"));
  break;

Or perhaps:
rate="15"
....
  media.playbackRate = parseInt(aTarget.getAttribute("rate"))/10;
  break;

Comment 3

[Philip Chee]

>> +                    name="playbackrate"
> What's this name for?
Oh a radiogroup.

> +<!ENTITY mediaPlaybackRate200x.label "Ludicrous Speed (2×)">
> +<!ENTITY mediaPlaybackRate200x.accesskey "L">
Hrm Warp Speed?

> +<!ENTITY mediaPlaybackRate200x.label "Ludicrous Speed (2×)">
I think you can probably use &times; here e.g.
"Ludicrous Speed (2&times;)"

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #3)
> > +<!ENTITY mediaPlaybackRate200x.label "Ludicrous Speed (2×)">
> > +<!ENTITY mediaPlaybackRate200x.accesskey "L">
> Hrm Warp Speed?

I don't really care either way, also because I can hardly guess which phrase is better understood by the average user. Can you check with Neil and sort out the l10n details so we can check in that part before the string freeze? Judging from your other comments, the code changes will need some more discussion...

Comment 5

[[e-mail]]

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

(HTML5 video crashed my Linux build last time I tried, and my Windows build is still rebuilding because of Win7 SDK issues...)

>+                    name="playbackrate"
[Not 100% sure that the name is necessary as you're manually checking/unchecking the menuitems.]

>+          <menuitem id="context-media-playbackrate-100x"
[The x is confusing because it's a percentage, not a multiplier. Zoom just uses 100.label for instance.]

>+                    checked="true"
[Don't need to check this because you're checking it in code anyway.]

>+      this.setItemAttr("context-media-playbackrate", "disabled", hasError);
>+      this.setItemAttr("context-media-playbackrate-050x", "disabled", hasError);
>+      this.setItemAttr("context-media-playbackrate-100x", "disabled", hasError);
>+      this.setItemAttr("context-media-playbackrate-150x", "disabled", hasError);
>+      this.setItemAttr("context-media-playbackrate-200x", "disabled", hasError);
[Don't need to disable the menuitems if the entire submenu is disabled.]

>+<!-- LOCALIZATION NOTE: "Ludicrous Speed" is a reference to the
>+movie "Space Balls" and is meant to say that this speed is very
>+fast. -->
>+<!ENTITY mediaPlaybackRate200x.label "Ludicrous Speed (2Ã)">
>+<!ENTITY mediaPlaybackRate200x.accesskey "L">
[I would be happy with Double Speed. Also, I think Bugzilla corrupts unicode when editing as comment, which is why it doesn't say 2×.]

Comment 6

[[e-mail]]

(In reply to Philip Chee from comment #2)
> Or perhaps:
> rate="15"
> ....
>   media.playbackRate = parseInt(aTarget.getAttribute("rate"))/10;
>   break;

In that case you might as well extract the rate from the id...

Comment 7

[Philip Chee]

> In that case you might as well extract the rate from the id...
media.playbackRate = parseInt(aTarget.id.replace(/.*-/, ""))/100

Comment 8

[[e-mail]]

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

>-  mediaCommand: function(aCommand) {
>+  mediaCommand: function(aCommand, aData) {
(Several existing commands could be updated to take an aData parameter)

>+<!ENTITY mediaPlaybackRate.label     "Play Speed">
Nit: Playback Speed

>+<!-- LOCALIZATION NOTE: "Ludicrous Speed" is a reference to the
>+movie "Space Balls" and is meant to say that this speed is very
>+fast. -->
Double speed is in fact ludicrous, you can hardly follow the video any more ;-)

Comment 9

[[e-mail]]

(In reply to Philip Chee from comment #7)
> > In that case you might as well extract the rate from the id...
> media.playbackRate = parseInt(aTarget.id.replace(/.*-/, ""))/100

I would probably use
<menupopup oncommand="gContextMenu.mediaCommand('playbackRate', event.target.id.replace(/.*-/, "") / 100);">
(this requires removing the trailing x of course)

Comment 10

[[e-mail]]

(In reply to comment #8)
> (Several existing commands could be updated to take an aData parameter)
(Note to self for future bug: "muted", "controls", "stats")

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #9)
> I would probably use
> <menupopup oncommand="gContextMenu.mediaCommand('playbackRate',
> event.target.id.replace(/.*-/, "") / 100);">
> (this requires removing the trailing x of course)

It seems you two mostly think alike and complement one another. This, in combination with time limitations on my side, suggests that you should drive this forward as an assignee and reviewer team. If l10n changes like above are needed and you'd like to make the string freeze, please go ahead and take over.

Comment 12

[[e-mail]]

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

Well it would be unfair not to give you at least a f+ for posterity.

Comment 13

[Philip Chee]

Attachment: Patch v1.0 address review comments. Based on patch by Jens Hatlak.

>>+                    name="playbackrate"
> [Not 100% sure that the name is necessary as you're manually checking/unchecking the menuitems.]
Since it's a radiogroup, I'm leaving this in.

>>+          <menuitem id="context-media-playbackrate-100x"
> [The x is confusing because it's a percentage, not a multiplier. Zoom just uses 100.label for instance.]
Fixed. Now using -05, -10, -15, and -20

>>+                    checked="true"
> [Don't need to check this because you're checking it in code anyway.]
Fixed.

>>+      this.setItemAttr("context-media-playbackrate", "disabled", hasError);
>>+      this.setItemAttr("context-media-playbackrate-050x", "disabled", hasError);
>>+      this.setItemAttr("context-media-playbackrate-100x", "disabled", hasError);
>>+      this.setItemAttr("context-media-playbackrate-150x", "disabled", hasError);
>>+      this.setItemAttr("context-media-playbackrate-200x", "disabled", hasError);
> [Don't need to disable the menuitems if the entire submenu is disabled.]
Fixed. But I had to adjust the context menu test for this change.

>>+<!-- LOCALIZATION NOTE: "Ludicrous Speed" is a reference to the
>>+movie "Space Balls" and is meant to say that this speed is very
>>+fast. -->
>>+<!ENTITY mediaPlaybackRate200x.label "Ludicrous Speed (22×)">
>>+<!ENTITY mediaPlaybackRate200x.accesskey "L">
> [I would be happy with Double Speed.
I'm now using "Double Speed" after deciding that cutesy names would be inappropriate here.

> Also, I think Bugzilla corrupts unicode when editing as comment, which is why it doesn't say 2×.]
This is why I'm now using the HTML entity: "&#215;". I don't know why &times; didn't work.

>>+<!ENTITY mediaPlaybackRate.label     "Play Speed">
> Nit: Playback Speed
Fixed.

>>+<!-- LOCALIZATION NOTE: "Ludicrous Speed" is a reference to the
>>+movie "Space Balls" and is meant to say that this speed is very
>>+fast. -->
> Double speed is in fact ludicrous, you can hardly follow the video any more ;-)
Removed this NOTE since I'm not using a humorous phrase here. Instead, I've added an note for the multiplication sign as a HTML entity.

+<!-- LOCALIZATION NOTE: &#215; is the HTML entity for the Unicode Character
+'MULTIPLICATION SIGN' (U+00D7). -->

>> > In that case you might as well extract the rate from the id...
>> media.playbackRate = parseInt(aTarget.id.replace(/.*-/, ""))/100
> 
> I would probably use
> <menupopup oncommand="gContextMenu.mediaCommand('playbackRate', event.target.id.replace(/.*-/, "") / 100);">
> (this requires removing the trailing x of course)
Removed the trailing x. I also left the calculations in nsContextMenu.js because I felt this was (slightly) more readable:

> +      case "playbackRate":
> +        media.playbackRate = aData.id.replace(/.*-/, "")/10;
> +        break;

TEST_PATH=suite/browser/test/test_contextmenu.html  pymake -C ../objdir-sm/ mochitest-plain

Passed: 2084
Failed: 48
Todo: 0

The failures appear to be related to breakage caused by async inline spell checker and occur with or without this patch.

Comment 14

[[e-mail]]

(In reply to Philip Chee from comment #13)
> > Also, I think Bugzilla corrupts unicode when editing as comment, which is why it doesn't say 2×.]
> This is why I'm now using the HTML entity: "×". I don't know why
> × didn't work.
Ø is an XML entity. × is an HTML entity, and therefore doesn't work in XUL. But unless you're on Windows (Notepad always adds a UTF-8 BOM which we don't want), you should prefer UTF-8.

> I also left the calculations in nsContextMenu.js
> because I felt this was (slightly) more readable:
It would make more sense for the parameter to be the value that you're trying to set. For instance, a "muted" command would take a boolean parameter.

Comment 15

[[e-mail]]

Comment on attachment 768287 [details] [diff] [review]
Patch v1.0 address review comments. Based on patch by Jens Hatlak.

>+        media.playbackRate = aData.id.replace(/.*-/, "")/10;
Nit: spaces around /
Since the entities use three digits, I think the id should too.

>+<!ENTITY mediaPlaybackRate050.label     "Slow Motion (0.5&#215;)">
>+<!ENTITY mediaPlaybackRate050.accesskey "S">
>+<!ENTITY mediaPlaybackRate100.label     "Normal Speed">
>+<!ENTITY mediaPlaybackRate100.accesskey "N">
>+<!ENTITY mediaPlaybackRate150.label     "High Speed (1.5&#215;)">
>+<!ENTITY mediaPlaybackRate150.accesskey "H">
Sorry for bikeshedding, but should we use the ½ sign here perhaps?

>+<!ENTITY mediaPlaybackRate200.label     "Double Speed (2&#215;)">
>+<!ENTITY mediaPlaybackRate200.accesskey "D">
Do we need to mention that double speed is 2×?

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 768287 [details] [diff] [review]
Patch v1.0 address review comments. Based on patch by Jens Hatlak.

f=me without the "(2x)". Reasoning: It is in fact redundant now, and without it there's an alternating scheme (first and third entries with parentheses, second and fourth without) which I find visually more attractive.

Regarding the × in the l10n note one could argue that "XML entity" is more correct and will hopefully prevent translators from trying to use other HTML entities.

Comment 17

[Philip Chee]

Attachment: Patch v1.1 fix more nits. Carrying forward f=InvisibleSmiley.

> (In reply to Philip Chee from comment #13)
>>> Also, I think Bugzilla corrupts unicode when editing as comment, which is why it doesn't say 2Î.]
>> This is why I'm now using the HTML entity: "×". I don't know why
>> × didn't work.
> Ø is an XML entity. × is an HTML entity, and therefore doesn't
> work in XUL. But unless you're on Windows (Notepad always adds a UTF-8 BOM > which we don't want), you should prefer UTF-8.
Now using UTF-8 with Unicode Character 'MULTIPLICATION SIGN' (U+00D7)

>> I also left the calculations in nsContextMenu.js
>> because I felt this was (slightly) more readable:
> It would make more sense for the parameter to be the value that you're trying
> to set. For instance, a "muted" command would take a boolean parameter.
Fixed.

>>+        media.playbackRate = aData.id.replace(/.*-/, "")/10;
> Nit: spaces around /
Fixed.

> Since the entities use three digits, I think the id should too.
Fixed.

>>+<!ENTITY mediaPlaybackRate050.label     "Slow Motion (0.5&#215;)">
>>+<!ENTITY mediaPlaybackRate050.accesskey "S">
>>+<!ENTITY mediaPlaybackRate100.label     "Normal Speed">
>>+<!ENTITY mediaPlaybackRate100.accesskey "N">
>>+<!ENTITY mediaPlaybackRate150.label     "High Speed (1.5&#215;)">
>>+<!ENTITY mediaPlaybackRate150.accesskey "H">
> Sorry for bikeshedding, but should we use the ¢ sign here perhaps?
Now trying with UTF-8 Unicode Character 'VULGAR FRACTION ONE HALF' (U+00BD).

>>+<!ENTITY mediaPlaybackRate200.label     "Double Speed (2&#215;)">
>>+<!ENTITY mediaPlaybackRate200.accesskey "D">
> Do we need to mention that double speed is 2×?
(2×) removed.

> f=me without the "(2×)". Reasoning: It is in fact redundant now, and without it there's an alternating scheme (first and third entries with parentheses, second and fourth without) which I find visually more attractive.
Done.

> Regarding the &#215; in the l10n note one could argue that "XML entity" is
> more correct and will hopefully prevent translators from trying to use other
> HTML entities.
Point is now moot since I've switched back to UTF-8,

Comment 18

[[e-mail]]

Comment on attachment 768942 [details] [diff] [review]
Patch v1.1 fix more nits. Carrying forward f=InvisibleSmiley.

> +<!ENTITY mediaPlaybackRate.label        "Playback Speed">
> +<!ENTITY mediaPlaybackRate.accesskey    "b">
> +<!ENTITY mediaPlaybackRate050.label     "Slow Motion (½×)">
> +<!ENTITY mediaPlaybackRate050.accesskey "S">
> +<!ENTITY mediaPlaybackRate100.label     "Normal Speed">
> +<!ENTITY mediaPlaybackRate100.accesskey "N">
> +<!ENTITY mediaPlaybackRate150.label     "High Speed (1.5×)">
(1½×) perhaps?

Comment 19

[Philip Chee]

>> +<!ENTITY mediaPlaybackRate150.label     "High Speed (1.5×)">
> (1½×) perhaps?
Done!

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/9cf4459806ca