Closed Bug 885506 Opened 6 years ago Closed 6 years ago

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

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set

Tracking

(seamonkey2.22 fixed)

RESOLVED FIXED
seamonkey2.22
Tracking Status
seamonkey2.22 --- fixed

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch patchSplinter Review
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.
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #766289 - Flags: review?(neil)
Whiteboard: [good first bug][lang=xul/js][mentor=InvisibleSmiley]
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;
>> +                    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;)"
(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 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×.]
(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...
> In that case you might as well extract the rate from the id...
media.playbackRate = parseInt(aTarget.id.replace(/.*-/, ""))/100
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 ;-)
(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)
(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")
(In reply to neil@parkwaycc.co.uk 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.
Assignee: jh → nobody
Status: ASSIGNED → NEW
Comment on attachment 766289 [details] [diff] [review]
patch

Well it would be unfair not to give you at least a f+ for posterity.
Attachment #766289 - Flags: review?(neil) → feedback+
>>+                    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.
Attachment #768287 - Flags: review?(neil)
Attachment #768287 - Flags: feedback?(jh)
(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: "&#215;". I don't know why
> &times; didn't work.
&#216; is an XML entity. &times; 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 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 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 &#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.
Attachment #768287 - Flags: feedback?(jh) → feedback+
> (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: "&#215;". I don't know why
>> &times; didn't work.
> &#216; is an XML entity. &times; 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,
Attachment #768287 - Attachment is obsolete: true
Attachment #768287 - Flags: review?(neil)
Attachment #768942 - Flags: review?(neil)
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?
Attachment #768942 - Flags: review?(neil) → review+
>> +<!ENTITY mediaPlaybackRate150.label     "High Speed (1.5×)">
> (1½×) perhaps?
Done!

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/9cf4459806ca
Assignee: nobody → jh
No longer blocks: FF2SM
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.22
Depends on: 840745
You need to log in before you can comment on or make changes to this bug.