887934 - [webvtt] Update <video> controls to include options for closed captioning

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Rick Eyre (:reyre)]

We need to add some kind of UI to the <video> controls to be able to toggle closed captioning on and off and switch between languages at the minimum.

Comment 1

[Rick Eyre (:reyre)]

Could you point me in the right direction for me to get started on this Ralph? I'm not quite sure what files I need to touch or how to go about doing this.

Comment 2

[Ralph Giles (:rillian)]

Our video controls pre-date HTML5, and are written in a thing called XBL. It's different, but similar enough (XML elements + javascript) to figure out easily. The main file is

  https://github.com/mozilla/gecko-dev/blob/master/toolkit/content/widgets/videocontrols.xml

along with the parallel css file and related tests. Look at how the current controls are implemented, and see if you can get a popup menu reflecting TextTrackList on the parent media element.

Bug 649490 is the last time we modified the controls, so look there for test file locations, how to get UX review, a11y labels, etc.

The controls run in chrome space, so you can plumb through chrome-only interfaces if you need them, but I suspect we don't unless we need to distinguish user selections from those made by page content.

Finally, that file is only desktop afaik. There are separate controls used on mobile (Android, Firefox OS, Windows Metro). Look for touchcontrols.css.

Comment 3

[Ralph Giles (:rillian)]

https://developer.mozilla.org/en-US/docs/XBL has some reference links.

Comment 4

[Ralph Giles (:rillian)]

Adding Sabrina who expressed interest in the ui aspect.

Comment 5

[Rick Eyre (:reyre)]

Attachment: Part 1: Expose a flag on the MediaElement that controls if TextTracks are enabled

I think this property on the MediaElement should be ChromeOnly, but I'm having trouble getting videocontrols.xml access to it when it's set to ChromeOnly. From my understanding it should have access to ChromeOnly properties by default.

Is there something I'm missing?

Comment 6

[Rick Eyre (:reyre)]

Attachment: Part 2: Add a simple on/off button for text tracks r=rillian

I just wanted to get some feedback about going in this direction. Right now I'm just using the text 'CC' for the button, but I'm assuming ideally we would want a image for that.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8381527 [details] [diff] [review]
Part 1: Expose a flag on the MediaElement that controls if TextTracks are enabled

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

Hmm. You don't say why you want to do it this way. I imagined you'd have a button which would reflect the union of the enabled (showing) text tracks on the element. Clicking it would toggle them on or off. I guess it's hard to remember state with multiple tracks that way, but at least you would get default display of the default track if there was one, with a way to turn them off, and a way to turn them on (the first one until we have a menu?) if there isn't a default track. Is there a reason that can't work.

You should be able to do that without chrome-only methods.

Comment 8

[Ralph Giles (:rillian)]

Adding phlsa and boriss for ui review, per madhava's suggestion.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Jared may have something to add here, he maintains the video controls.

Comment 10

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

Hi Rick, I'm happy to help. The videocontrols do not have chrome permissions, they run in content (see bug 704326 for one case that I had to work around).

Comment 11

[Rick Eyre (:reyre)]

Thanks for the CC Chris.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Hi Rick, I'm happy to help. The videocontrols do not have chrome
> permissions, they run in content (see bug 704326 for one case that I had to
> work around).

Thanks for the help Jared. I'm not really a designer so I'm not sure how far I should go on this. Do you have any recommendations or on how the UI should look? I'm comfortable getting basic functionality in for turning tracks on and off, but would have a harder time with what it's actually going to look like.

Chrome and Youtube have a popup for closed captions that has the available tracks that are on the video, etc, would we want to do a similar thing?

Comment 12

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

Yes, we should do the same. Until we have a proper icon you can reuse one of the videocontrols icons.

Comment 13

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Yes, we should do the same. Until we have a proper icon you can reuse one of
> the videocontrols icons.

The icon for closed captions is usually the »cc« in some sort of rectangle. It should have the same styling as the other controls. needinfo shorlander because he probably knows if there is a PSD for this :)

Comment 14

[Rick Eyre (:reyre)]

Attachment: WIP

Comment 15

[Rick Eyre (:reyre)]

Attachment: basic on off.png

Comment 16

[Rick Eyre (:reyre)]

Attachment: not flexible.png

Comment 17

[Stephen Horlander [:shorlander]]

Attachment: closeCaptionButton.png

Comment 18

[Stephen Horlander [:shorlander]]

Attachment: [e-mail]

Comment 19

[Ralph Giles (:rillian)]

Thanks! Can you also attach an svg version? The other controls scale with screen res.

Comment 20

[Rick Eyre (:reyre)]

I'm not sure if I'll be able to finish this anytime soon. Would you be able to take over this Jared?

Comment 21

[Rick Eyre (:reyre)]

See comment 20.

Comment 22

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

I don't have time to take this right now, but I can help mentor it. Danny, do you think you could pick this up?

Comment 23

[Eric Phan]

Hi, we are a team of 5 students (mentored by padenot) and we are interested in contributing to this bug. But we were wondering what remains to be done.

Comment 24

[Rick Eyre (:reyre)]

IIRC, I got most of the JS working that will control the state of the tracks being turned on/off and whether or not there displayed. Most of what is left to do is UI work. What I remember being left to do is:

- The .png files that Stephen attached to this bug need to be used for the closed captioning and buttong list.
- The UI list of tracks that you can toggle on/off wasn't expanding correctly as tracks were being added to it.
- The relevant CSS for Android, Windows, etc, platforms needs to be added. It looks like I've only done OSX so far.

Other then that, please look at the WIP I attached and try to build it. You'll probably need to resolve some merge conflicts since it's been a while since I worked on this and the tree has changed.

Feel free to ask me any more questions :-).

Comment 25

[Richard Garnier]

Hello, we've been trying to continue your work, but there is a couple of things we don't understand.
First, what is textTrackChanged supposed to do? 
We guess that the function textTrackAdded is called when Mozilla found a vtt file.
So, we supposed that the function textTrackRemoved is called when the vtt file disappear (dynamic html code??)

Then, we don't fully understand what are textTrackStates and video.textTracks.
We think that textTracks represent which subtitle have to be shown, and that textTrackStates contain all of the subtitles. Are we correct?
We also think that we are treating sentences one by one, but we aren't sure.

Thanks a lot for your time.

(Currently, we managed to display a menu with the different subtitles available, and to close captions.
But to open then again, one button open them all when the other one are useless)

Comment 26

[Richard Garnier]

Attachment: menu, but no css

Comment 27

[Rick Eyre (:reyre)]

Sorry for not getting back to you note quickly Richard. Do you still need answers to those questions?

Comment 28

[Richard Garnier]

No, thank you :)
I think this is working pretty well now. We are now working on the css part.

Comment 29

[Rick Eyre (:reyre)]

Awesome that's great to hear :-).

Comment 30

[Richard Garnier]

By the way, we got a problem with the css.
We created buttons dynamically with javascript, and the css we wrote doesn't apply to those buttons. The class is changed, but the css doesn't seem to reach the buttons. We tried to change the namespace, we tried to use classList.add , className += but nothing worked. Paul Adenot suggested that we change everything to controlList but we haven't tried it yet.
Do you have any idea ?

Comment 31

[Paul Adenot (:padenot)]

Jared, do you have an idea about comment 30 ? Would the issue be that creating HTML element in a XUL document would do strange things when trying to do the selector matching ?

What is the cleanest way to solve this? The solution that does not work is attached here.

Comment 32

[Rick Eyre (:reyre)]

Yeah, Jared's probably the one you want to talk to Richard.

Comment 33

[Rick Eyre (:reyre)]

For CSS issues. I can help with JS stuff, or questions about WebVTT and it's implementation.

Comment 34

[Eric Phan]

Attachment: functions_ok_but_miss_css_v1

Comment 35

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

Comment on attachment 8440656 [details] [diff] [review]
menu, but no css

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

::: toolkit/content/widgets/videocontrols.xml
@@ +293,5 @@
>                        <scale class="volumeControl" movetoclick="true"/>
>                      </stack>
> +		    <button class="ccButton ccPanelButton" type="panel">
> +                      <panel class="ccPanel" position="topleft bottomleft">
> +			 <ul id="ccLanguageList">

If you are trying to reference the ccLanguageList from a CSS file, you will need to include the html namespace as part of the selector.

The namespaces are already declared at the top of the CSS files, so you will just need to prefix the selector with `html|`.

For example, 
html|#ccLanguageList {
}

Hope that helps!

Comment 36

[Rose Badoux]

Indeed it helps !
Thanks

Comment 37

[Rick Eyre (:reyre)]

Comment on attachment 8440751 [details] [diff] [review]
functions_ok_but_miss_css_v1

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

Looking good so far! Just a few things to sort out on the JS side from what I can see.

::: toolkit/content/widgets/videocontrols.xml
@@ +334,5 @@
>                  stats          : {},
>                  controlsOverlay : null,
>                  fullscreenButton : null,
> +                textTrackStates : [],
> +                //textTracksOn : true, 

Remove this commented code. If you're just asking for feedback and you're planning to uncomment this in your next version of this patch then that's fine.

@@ +499,5 @@
>  
>                      this._handleCustomEventsBound = this.handleCustomEvents.bind(this);
>                      this.video.addEventListener("media-showStatistics", this._handleCustomEventsBound, false, true);
> +
> +		    //this.ccInit();

Delete this if you don't plan to use it in the next version of your patch.

@@ +1357,5 @@
> +                  for each (let textTrack in this.video.textTracks) {
> +                    textTrack.mode = "disabled";
> +                  }
> +                },
> +		

Please remove this whitespace.

@@ +1360,5 @@
> +                },
> +		
> +		changeCaptions : function(trackNum) {
> +                  for each (let ttState in this.textTrackStates) {
> +                    if (ttState.trackNum == trackNum) {

Please use the identity operator here, i.e., '==='

@@ +1363,5 @@
> +                  for each (let ttState in this.textTrackStates) {
> +                    if (ttState.trackNum == trackNum) {
> +                      ttState.mode = "showing";
> +                    } else {
> +                      ttState.mode = "disabled";

If I understand this correctly you're going to only allow one caption to be showing at once? I'm not sure this is the desired behaviour we should have. Conceptually a user should be able to toggle on as many captions as they want at the same time. This doesn't make much sense for captions that are languages, but it does if you have tracks corresponding to subtitles and, say, a descriptions track.

@@ +1367,5 @@
> +                      ttState.mode = "disabled";
> +		    }
> +                  }
> +		},
> +	

Remove whitespace.

@@ +1373,5 @@
> +		  this.changeCaptions(trackNum);
> +                  var self = this;
> +                  function findState(trackNum, textTrack) {
> +                    for each (let ttState in self.textTrackStates) {
> +                      if (ttState.textTrack == textTrack) {

Please use the '===' operator here.

@@ +1379,5 @@
> +                      }
> +                    }
> +                  }
> +                  for each (let textTrack in this.video.textTracks) {
> +                    textTrack.mode = findState(trackNum, textTrack);

I'm not quite sure what you're trying to do here. Doesn't this just reset the text track to the state that it already is?

@@ +1393,5 @@
> +		  track.trackNum = this.trackNumMax;
> +		  this.trackNumMax++;
> +		  button.innerHTML = track.label || track.language || track.id;
> +		  var self = this;
> +		  button.addEventListener("click", function() { self.openCaptions(this.getAttribute("trackNum")); });

Please put this anonymous function on a newline:

button.addEventListener("click", function() {  
  self.openCaptions(this.getAttribute("trackNum"));
});

@@ +1577,5 @@
>                      addListener(this.videocontrols, "transitionend", this.onTransitionEnd);
>                      addListener(this.video.ownerDocument, "mozfullscreenchange", this.onFullscreenChange);
>                      addListener(this.video, "keypress", this.keyHandler);
>  
> +                    addListener(this.ccOffButton, "click", this.closeCaptions);

We also need to be able to turn captions back on once this button is clicked again. So this doesn't just turn off all captions, it would turn them back on as well. Essentially disabling/enabling them globally, instead of on a per track basis.

Comment 38

[Richard Garnier]

(In reply to Rick Eyre (:reyre) from comment #37)

> @@ +1379,5 @@
> > +                      }
> > +                    }
> > +                  }
> > +                  for each (let textTrack in this.video.textTracks) {
> > +                    textTrack.mode = findState(trackNum, textTrack);
> 
> I'm not quite sure what you're trying to do here. Doesn't this just reset
> the text track to the state that it already is?

It is refreshing what we are currently displaying, because we just changed the state of the structure (textTrackStates). Nevertheless, it is true that a lot of those operation were already done. So I've been doing sligthly differently to reduce the cost.

Regarding the fonctionnalities, what should happen if we click on a language right after turning off the caption?
For example :
- activate french
- turn it off with "no subtitles"
- click on english
Right now, it is currently displaying english only, should it also display french?

Comment 39

[Richard Garnier]

Attachment: function-css-ok

Here's the patch, I would like to get it reviewed.
However we noticed two bugs :
- if you go fullscreen, the subtitle menu doesn't work anymore even if you leave the fullscreen.
- it seems that it doesn't work on OSX.

Comment 40

[Rick Eyre (:reyre)]

(In reply to Richard Garnier from comment #38)

> Regarding the fonctionnalities, what should happen if we click on a language
> right after turning off the caption?
> For example :
> - activate french
> - turn it off with "no subtitles"
> - click on english
> Right now, it is currently displaying english only, should it also display
> french?

No, I think it should only display English. So here's how I picture these actions working:

* Clicking captions off toggles all them off and disables all display of all tracks with a kind of 'caption' or 'subtitle'. Other kinds such as 'metadata' or 'chapters' should still be enabled as they might be used to do something else with scripts that isn't really related to closed captioning.
* Clicking captions on after previously turning them off should reset all the tracks 'showing' or 'disabled' values to what they were when the media element was first loaded. I.E. whatever tracks were automatically selected to be turned on by the media element should be turned on.
* Clicking specific caption on/off should turn only that one on or off. If captions have been disabled and one is clicked it should only turn that one on.

Ralph, does this sound like reasonable behaviour for the CC functionality?

I'd also go take a look at what YouTube and others do for CC buttons on the media element. We'd probably want something close to that.

Comment 41

[Rick Eyre (:reyre)]

Comment on attachment 8441246 [details] [diff] [review]
function-css-ok

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

It's looking good, but a lot of my previous review comments haven't been addressed yet. Please read through those.

::: toolkit/content/widgets/videocontrols.xml
@@ +332,5 @@
>                  stats          : {},
>                  controlsOverlay : null,
>                  fullscreenButton : null,
> +                textTrackStates : [],
> +                textTracksOn : true, 

Please remove the whitespace at the end of this line.

@@ +1369,5 @@
> +                      textTrack.mode = findState(textTrack);
> +                    }
> +                  }
> +                },
> +		

Please remove this white space here.

@@ +1376,5 @@
> +                    if (ttState.trackNum == trackNum) {
> +		      if (ttState.mode == "showing") {
> +                        ttState.mode = "disabled";
> +                      } else {
> +                        ttState.mode = "showing";

Please refer to my previous review for my issues with this.

@@ +1386,5 @@
> +		openCaptions : function(trackNum) {
> +		  if (!this.textTracksOn) {
> +                    this.textTracksOn = true;
> +                    for each (let ttState in this.textTrackStates) {
> +                      ttState.mode = "disabled";

I'm not sure what you're trying to do here. If !this.textTracksOn is true then all the textTrackStates should be set to disabled already, correct?

Comment 42

[Ralph Giles (:rillian)]

(In reply to Rick Eyre (:reyre) from comment #40)

> * Clicking captions off toggles all them off and disables all display of all
> tracks with a kind of 'caption' or 'subtitle'. Other kinds such as
> 'metadata' or 'chapters' should still be enabled as they might be used to do
> something else with scripts that isn't really related to closed captioning.

Yep.

> * Clicking captions on after previously turning them off should reset all
> the tracks 'showing' or 'disabled' values to what they were when the media
> element was first loaded. I.E. whatever tracks were automatically selected
> to be turned on by the media element should be turned on.

This doesn't make sense to me. If the user has selected some non-default tracks, it's less surprising if we restore whatever tracks was displaying previously. If I switch to Portuguese, turn captions off to read a sign in the background, or because I understand this bit, and then turn them back on, I want Portuguese back not English or whatever the default was. We should respect the user's choice.

> * Clicking specific caption on/off should turn only that one on or off. If
> captions have been disabled and one is clicked it should only turn that one
> on.

That makes sense to me.

> I'd also go take a look at what YouTube and others do for CC buttons on the
> media element. We'd probably want something close to that.

Safari only allows selecting one language at a time from a single option group, so selecting 'Subtitles off' perforce deselects the current track, and the only way to turn them on is to select a specific text track. That won't work for us since we're planning to allow multiple tracks to display at once.

Chrome only seems to have an on/off button, with no way to select non-default tracks.

The Youtube player has a separate on/off button and a popup menu for selecting the (single, like Safari) enabled language. Turning titles off and on again resets the selected language, like Rick described. I think we should remember the previously enabled set and restore that when cycling from off to on instead.

I'm not sure how that should interact with changes made by script. Probably the most recent change should win, with the controls reflecting changes made by scripts, unless we see problems with sites fighting users. Something to write into the ui tests!

Comment 43

[Rick Eyre (:reyre)]

(In reply to Ralph Giles (:rillian) from comment #42)

> This doesn't make sense to me. If the user has selected some non-default
> tracks, it's less surprising if we restore whatever tracks was displaying
> previously. If I switch to Portuguese, turn captions off to read a sign in
> the background, or because I understand this bit, and then turn them back
> on, I want Portuguese back not English or whatever the default was. We
> should respect the user's choice.

Ah, yes. I agree with this. I guess I was thinking along the lines that the browser selected a language based on a browser user preference (possibly in the future?), but if the user selects a language that should take precedence over that as well.

Comment 44

[Richard Garnier]

(In reply to Rick Eyre (:reyre) from comment #41)
> Comment on attachment 8441246 [details] [diff] [review]
> function-css-ok
> 
> Review of attachment 8441246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's looking good, but a lot of my previous review comments haven't been
> addressed yet. Please read through those.
> 
> ::: toolkit/content/widgets/videocontrols.xml
> @@ +332,5 @@
> >                  stats          : {},
> >                  controlsOverlay : null,
> >                  fullscreenButton : null,
> > +                textTrackStates : [],
> > +                textTracksOn : true, 
> 
> Please remove the whitespace at the end of this line.
>
> @@ +1369,5 @@
> > +                      textTrack.mode = findState(textTrack);
> > +                    }
> > +                  }
> > +                },
> > +		
> 
> Please remove this white space here.

Sorry for the whitespace, I'll be more careful in the future.
 
> @@ +1376,5 @@
> > +                    if (ttState.trackNum == trackNum) {
> > +		      if (ttState.mode == "showing") {
> > +                        ttState.mode = "disabled";
> > +                      } else {
> > +                        ttState.mode = "showing";
> 
> Please refer to my previous review for my issues with this.
> 

The utility of this code has changed since the last review. This is changing the state of the track that the user is selecting. For example : user click "English subtitle", if this track was in "showing", he wants to disable it and if it was not "showing", he wants to enable it.

> @@ +1386,5 @@
> > +		openCaptions : function(trackNum) {
> > +		  if (!this.textTracksOn) {
> > +                    this.textTracksOn = true;
> > +                    for each (let ttState in this.textTrackStates) {
> > +                      ttState.mode = "disabled";
> 
> I'm not sure what you're trying to do here. If !this.textTracksOn is true
> then all the textTrackStates should be set to disabled already, correct?

No, I'm using the textTrackStates to remember the states of the subtitles. It is not yet set to disabled (but this.video.textTracks is disabled) in case the user would want to reopen them all.
This for loop is here for the following case :
1 : user close all captions
2 : user click on a specific track
In this case, we need to forget which tracks were enabled, and this is what it's doing.

Comment 45

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

Comment on attachment 8441246 [details] [diff] [review]
function-css-ok

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

::: toolkit/content/widgets/videocontrols.xml
@@ +293,5 @@
>                        <scale class="volumeControl" movetoclick="true"/>
>                      </stack>
> +		    <button class="ccButton ccPanelButton" type="panel">
> +                      <panel class="ccPanel" id="ccLanguageList" position="topleft bottomleft">
> +                        <button anonid="ccOffButton" class="ccPanelButton">On/Off</button>

This text should be removed. We may need a separate icon to use to indicate when it is off, since we already reduce the opacity of the buttons when they are not hovered.

@@ +1357,5 @@
> +                    }
> +		  } else {
> +                    this.textTracksOn = true;
> +                    var self = this;
> +		    function findState(textTrack) {

If you declare the function as:
let findState = (textTrack) => {
};
then you won't need to do the 'self = this' dance. This is called a (fat) arrow function, more information on it can be found at http://robcee.net/2013/fat-arrow-functions-in-javascript/

@@ +1412,5 @@
> +		  this.trackNumMax++;
> +		  button.innerHTML = track.label || track.language || track.id;
> +		  var self = this;
> +		  button.addEventListener("click", function() {
> +		    self.openCaptions(this.getAttribute("trackNum"));

Please add the event listener to 'this', and use handleEvent to respond to the event. Doing so will eliminate the need to do the 'var self = this' dance here, as handleEvent uses the 'this' object that you would expect.
button.addEventListener("click", this);

::: toolkit/themes/windows/global/media/videocontrols.css
@@ +15,5 @@
>  }
>  
> +.ccPanel {
> +  border: none;
> +  opacity: 0,7;

I think you meant: opacity:0.7; here (note the comma was used instead of a decimal point).

Comment 46

[Rick Eyre (:reyre)]

Comment on attachment 8441246 [details] [diff] [review]
function-css-ok

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

::: toolkit/content/widgets/videocontrols.xml
@@ +1392,5 @@
> +                  }
> +		  this.changeCaptions(trackNum);
> +                  for each (let ttState in this.textTrackStates) {
> +		    if (ttState.trackNum == trackNum) {
> +                      for each (let textTrack in this.video.textTracks) {

You don't need to loop though the video's text tracks. Since  you already have the textTrack in the ttState you could just do ttState.textTrack.mode = ttState.mode

Comment 47

[Rick Eyre (:reyre)]

(In reply to Richard Garnier from comment #44)

> Sorry for the whitespace, I'll be more careful in the future.

No worries :-).

> The utility of this code has changed since the last review. This is changing
> the state of the track that the user is selecting. For example : user click
> "English subtitle", if this track was in "showing", he wants to disable it
> and if it was not "showing", he wants to enable it.

Ah, yes I see now.

Comment 48

[Richard Garnier]

> @@ +1412,5 @@
> > +		  this.trackNumMax++;
> > +		  button.innerHTML = track.label || track.language || track.id;
> > +		  var self = this;
> > +		  button.addEventListener("click", function() {
> > +		    self.openCaptions(this.getAttribute("trackNum"));
> 
> Please add the event listener to 'this', and use handleEvent to respond to
> the event. Doing so will eliminate the need to do the 'var self = this'
> dance here, as handleEvent uses the 'this' object that you would expect.
> button.addEventListener("click", this);

Will it not gonna reduce (slightly) the performance?
I mean, we know which fonction is supposed to be called on this event, and it will always be the same. Why would we want to treat an event each time the button is clicked instead of always calling the same function?
Unless, doing self=this hide a deeper problem than I tought?

Comment 49

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

(In reply to Richard Garnier from comment #48)
> Will it not gonna reduce (slightly) the performance?
> I mean, we know which fonction is supposed to be called on this event, and
> it will always be the same. Why would we want to treat an event each time
> the button is clicked instead of always calling the same function?
> Unless, doing self=this hide a deeper problem than I tought?

The performance cost of a string comparison and object comparison are negligible (in the range of nanoseconds) when compared to the cost of having readable/maintainable code as well as the reduced risk of memory leaks.

Comment 50

[Richard Garnier]

I don't get how I'm supposed to get which button has been clicked on, in the event handler.
Is it attached to the event?

button.addEventListener("click", function() {
    self.openCaptions(this.getAttribute("trackNum"));
}
Here this refer to the button.


switch (aEvent.type) {
   case "click":
       this.openCaptions(???);
       break;
Here, this refer to the window.

Comment 51

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

Yeah, it's attached to the event. Is your second example within a `handleEvent` function or still the anonymous function that is referred to in the beginning of your comment? If you are referring to a `handleEvent` function, then event.target may be what you are looking for.

You can see https://developer.mozilla.org/en-US/docs/Web/API/Event/Comparison_of_Event_Targets for more information.

Comment 52

[Yonas]

Hi, what is the status of this work?

Comment 53

[Ralph Giles (:rillian)]

Are you still working on this, Eric?

Comment 54

[Eric Phan]

(In reply to Ralph Giles (:rillian) from comment #53)
> Are you still working on this, Eric?

Hi,

No, I don't have enough free time to work on it.
The attachments should be up to date.

Comment 55

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Jared what is the best way forward here? Can someone finish off the patch? (I'm getting asked in the wild about this bug)

Comment 56

[Ian Devlin]

This is quite an important addition, it would be good to see this getting worked on.

Comment 57

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

(In reply to David Bolter [:davidb] from comment #55)
> Hi Jared what is the best way forward here? Can someone finish off the
> patch? (I'm getting asked in the wild about this bug)

Yeah, someone can resume the work that Eric had started. They should import the patch from comment #39 and then address the feedback in comments that follow.

Comment 58

[Rich Schwerdtfeger]

The US 508 refresh (government procurement criteria for accessibility) public rule making for final comments just came out. 

Here is the requirement:
503.4 User Controls for Captions and Audio Description. Where ICT displays video with synchronized audio, ICT shall provide user controls for closed captions and audio description conforming to 503.4.

This is part of why the other browser vendors have added support. 508 is also adopted by many US states, including California and Texas.

The requirement is addition is very important for people with hearing, vision, and cognitive impairments.

Comment 59

[Rich Schwerdtfeger]

Also, Readium Foundation needs this feature for accessibility with their cloud-based Firefox EPUB reader.

Comment 60

[Rich Schwerdtfeger]

when designing this I see there are 2 related 508 requirements:

503.4.1 Caption Controls. Where user controls are provided for volume adjustment, ICT shall provide user controls for the selection of captions at the same menu level as the user controls for volume or program selection.

503.4.2 Audio Description Controls. Where user controls are provided for program selection, ICT shall provide user controls for the selection of audio description at the same menu level as the user controls for volume or program selection.

Comment 61

[Marco Zehe (:MarcoZ)]

Added the sec508 keyword to flag this as having an impact on those requirements, or the proposed refresh thereof.

Comment 63

[herrernst]

Is there any progress on this? All other browsers have controls to toggle closed captions. We would be forced to build custom controls just because of this issue.

BTW, #515898 also seems to be a duplicate.

Comment 65

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

Tim, would you or your team want to pick this up?

Comment 66

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sounds like something Ray could help! Ray, please told me otherwise if your plate is full.

Comment 67

[Ray Lin[:ralin]]

No problem, I can take care of this issue. Thanks.

Comment 68

[Ralph Giles (:rillian)]

Thanks ralin!

Comment 69

[philipp.naderer]

There has been a discussion in German on Twitter during the last weekend. Summary / Conclusion: It could be enough to provide WebVTT for a video, but the current implementation in Firefox requires a workaround with custom controls from each web developer working with HTML5 video. Therefore, if the Firefox team adds a custom control (like Chrome) it would be sufficient to fulfill WCAG level AA. Until then web devs need custom controls, but we all know how this would end: everyone re-invents the wheel building custom UI controls instead of one single bugfix in Firefox.

Comment 70

[Ray Lin[:ralin]]

Attachment: Bug 887934 - Add closed caption button to video control.

Review commit: https://reviewboard.mozilla.org/r/48079/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48079/

Comment 71

[Ray Lin[:ralin]]

(In reply to philipp.naderer from comment #69)
> There has been a discussion in German on Twitter during the last weekend.
> Summary / Conclusion: It could be enough to provide WebVTT for a video, but
> the current implementation in Firefox requires a workaround with custom
> controls from each web developer working with HTML5 video. Therefore, if the
> Firefox team adds a custom control (like Chrome) it would be sufficient to
> fulfill WCAG level AA. Until then web devs need custom controls, but we all
> know how this would end: everyone re-invents the wheel building custom UI
> controls instead of one single bugfix in Firefox.

Thanks for the kind information philipp. 

I just push a very basic version patch to this bug. Though there's many details and feature like selecting text track not yet implemented, in this patch, CC button works well on switch on/off close caption.

I'll keep my patch updated if any progress, and please feel free to give me any feedback. Thanks!

Comment 72

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/1-2/

Comment 73

[Ray Lin[:ralin]]

Update:
* Clock on CC button will show a menu to select or off closed caption.
* Change/Off closed caption function is implemented and tested in normal/fullscreen mode.

Philipp

I added two screenshots on mozreview in comment 72, and current concept is copied from Safari.
Could you give me some feedback or provide me detailed spec? Thanks.

Comment 74

[philipp.naderer]

Thx for the screenshots. I think this is how an implementation should look like. The captions can be turned off and on, and multiple tracks are offered separately if present. I'm not sure if it needs a highlight / on-off state of the CC icon itself, if any of the tracks is active or none of them. If a track is on, the CC icon could be in the white of the current volume indicator.

Comment 75

[herrernst]

Thanks for your work, looks great!
I would also give the cc button a lighter (gray) color if disabled and white if it is enabled. Also, I think it looks nice in Chrome.
One thing I noticed is that (at least) if there's only one track element, Chrome shows no menu, but just toggles the subtitle, while Safari always shows a menu with "Off - Auto - $language" (I've not figured out what Auto means/does).
You probably know the specification for the <track> element, e.g. the differences between closed captions, subtitles etc (if it matters in this context): https://html.spec.whatwg.org/multipage/embedded-content.html#the-track-element
Although I have not found any spec how browser venders should implement the control interface.
Thanks again for your work!

Comment 76

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

https://reviewboard.mozilla.org/r/48079/#review45543

::: toolkit/themes/osx/global/media/videocontrols.css:126
(Diff revision 2)
> +  transition: transform 250ms ease-in;
> +}
> +
> +.textTrackList > html|*.textTrackButton {
> +  -moz-appearance: none;
> +  -moz-box-align: center;

Please keep the text aligned at the start of the line instead of centering.

It makes it much easier for readers to scan the text as they read each line since their eye only has to track back to the beginning of each line as compared to finding the beginning of the line before reading it.

Comment 77

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/2-3/

Comment 78

[Ray Lin[:ralin]]

Thank you philipp and herrernst! I think this revision is more close to complete. 

current works:
1. Hide CC icon if no track present
2. Toggle closed caption if only one track, otherwise show menu
3. Highlight CC button when closed captions is on
4. Highlight selected menu item

Jared, could you give me some feedback about this patch and tell me who can help with ui review? Many thanks!

Comment 79

[antistress]

Hi,

Here is how controls are shown within GNOME Web (aka Epiphany)
http://img4.hostingpics.net/pics/957923GNOMEWeb.png

I wander if it's a good idea to show a text into the button ("CC"), for i18n reasons.
As a native French speaker, CC doesn't mean anything to me related to captions (but Creative Commons comes to my mind)
Thanks !

Comment 80

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to antistress from comment #79)
> As a native French speaker, CC doesn't mean anything to me related to
> captions (but Creative Commons comes to my mind)
> Thanks !

I agreed. "CC" is very English-centric.

Comment 81

[philipp.naderer]

Hi!

Chrome uses the "CC" icon on my Mac, but I think there is no generic defacto-standard icon for subtitles / closed captions. YouTube shows in it's player imho a complicated icon. Other german streaming sites use a "UT" icon (for "Untertitel" = subtitles).

Please don't consider something like this ;) https://goo.gl/iNdm7i

---

YouTube in German:
http://imgur.com/4pMv7et

The "UT"-icon on the top left:
http://tv.orf.at/untertitel/

Comment 82

[herrernst]

Hi,
I also think that CC is quite uncommon e. g. for German speakers. Nonetheless, IE11 and Edge also use the 'CC' icon. Safari on OS X and iOS, on the other hand, use some kind of speech bubble:
Safari on OS X: http://i.imgur.com/RtX4CsL.png
Safari on iOS: http://i.imgur.com/1Njjw8F.png
Personally, I would prefer something like Safari, but I'm no expert and I don't know which one would be more comprehensible, also given that Chrome and IE/Edge already use 'CC'.
By the way, thanks for your work so far!

Comment 83

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

(In reply to Ray Lin[:ralin] from comment #78)
> Jared, could you give me some feedback about this patch and tell me who can
> help with ui review? Many thanks!

Please include r?jaws in your patch commit message when pushing to mozreview so that I get properly notified about the review request. I tend to prioritize my needinfo requests a bit lower than review requests.

Comment 84

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

https://reviewboard.mozilla.org/r/48079/#review47001

::: toolkit/content/widgets/videocontrols.xml:271
(Diff revision 3)
>                          </tr>
>                      </table>
>                  </html:div>
>              </vbox>
>  
> +

Nit, please remove this extra line.

::: toolkit/content/widgets/videocontrols.xml:276
(Diff revision 3)
> +                    <vbox class="textTrackList" hidden="true">
> +                    </vbox>

Nit, please place the closing tag on the same line as the opening tag since there is nothing between the tags and the line will be short.

::: toolkit/content/widgets/videocontrols.xml:318
(Diff revision 3)
>              <![CDATA[
>              this.isTouchControl = false;
>              this.randomID = 0;
>  
>              this.Utils = {
> -                debug : false,
> +                debug : true,

Nit, remember to undo this change.

::: toolkit/content/widgets/videocontrols.xml:1360
(Diff revision 3)
>  
>                      event.preventDefault(); // Prevent page scrolling
>                  },
>  
> +                isClosedCaptionOn : function () {
> +                  let isCCOn = false;

This variable can be removed.

::: toolkit/content/widgets/videocontrols.xml:1364
(Diff revision 3)
> +                isClosedCaptionOn : function () {
> +                  let isCCOn = false;
> +
> +                  for (let tt of this.video.textTracks) {
> +                    if (tt.mode === "showing") {
> +                      isCCOn = true;

You can just return `true` here.

::: toolkit/content/widgets/videocontrols.xml:1368
(Diff revision 3)
> +                    if (tt.mode === "showing") {
> +                      isCCOn = true;
> +                    }
> +                  }
> +
> +                  return isCCOn;

You can just return `false` here.

::: toolkit/content/widgets/videocontrols.xml:1372
(Diff revision 3)
> +                  if (this.video.textTracks.length !== 0) {
> +                    this.closedCaptionButton.removeAttribute("hidden");
> +                  } else {
> +                    return this.closedCaptionButton.setAttribute("hidden", "true");
> +                  }

Because there is an early-return here, please refactor this like so:

if (!this.video.textTracks.length) {
  this.closedCaptionButton.setAttribute("hidden", "true");
  return;
}

this.closedCaptionButton.removeAttribute("hidden");

::: toolkit/content/widgets/videocontrols.xml:1386
(Diff revision 3)
> +                    this.closedCaptionButton.setAttribute("closed", "true");
> +                  }
> +
> +                  const ttList = document.getAnonymousElementByAttribute(
> +                    this.videocontrols, "class", "textTrackList");
> +                  const ttItems = ttList.childNodes;

Nit, the nodelist here is const but we are modifying individual items of the nodelist so it's a bit misleading to call this const. We can just use `let` here instead.

::: toolkit/content/widgets/videocontrols.xml:1391
(Diff revision 3)
> +                  const ttItems = ttList.childNodes;
> +
> +                  for (let tti of ttItems) {
> +                    const idx = +tti.getAttribute("index");
> +
> +                    if (idx === this.currentTextTrackIndex) {

We prefer double-equals unless type coercion is specifically not desired.

::: toolkit/content/widgets/videocontrols.xml:1410
(Diff revision 3)
> +
> +                  const label = tt.label || "";
> +                  const ttText = document.createTextNode(label);
> +                  const ttBtn = document.createElement("button");
> +
> +                  ttBtn.setAttribute("class", "textTrackItem");

ttBtn.classList.add("textTrackItem");

::: toolkit/themes/osx/global/jar.mn:152
(Diff revision 3)
> +  skin/classic/global/media/closeCaptionButton.png                   (media/closeCaptionButton.png)
> +  skin/classic/global/media/[e-mail]                (media/[e-mail])

These should also be placed in /toolkit/themes/windows/global/media

::: toolkit/themes/osx/global/media/videocontrols.css:117
(Diff revision 3)
> +  -moz-appearance: none;
> +  -moz-box-pack: end;
> +  -moz-box-align: end;
> +  -moz-margin-end: 10px;
> +  padding: 0;
> +  /*transform: translate(-50px, -105px);*/

this line can be removed

Comment 85

[Ray Lin[:ralin]]

Attachment: Screenshot 2016-05-05 22.43.25.png

Thanks for all your feedback :) I think it is a good idea to use speech bubble like Safari.

Hi, Stephen
I not sure how UI should be implemented in this bug, but I've made a simple layout as screenshot shown. Could you help to check it? and what do you think about change "CC" icon? Thanks.

Comment 86

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/3-4/

Comment 87

[Stephen Horlander [:shorlander]]

(In reply to Ray Lin[:ralin] from comment #85)
> Thanks for all your feedback :) I think it is a good idea to use speech
> bubble like Safari.
> 
> Hi, Stephen
> I not sure how UI should be implemented in this bug, but I've made a simple
> layout as screenshot shown. Could you help to check it? and what do you
> think about change "CC" icon? Thanks.

I agree we should probably pick something that is not English specific. I think the speech bubble could work, the problem is that we are already using that for notifications: https://dxr.mozilla.org/mozilla-central/raw/browser/themes/shared/web-notifications-tray.svg

Let me try and few things and see if I can come up with something.

Comment 88

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Any updates? Maybe a TV frame w/ two lines at the bottom of the screen? Or maybe we should land this with the CC icon and come up with something better later on?

Comment 89

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

Let's move forward with the CC icon if we have it in the sizes that we need. It will be much easier to update the icon later after the feature has landed as it will be a simple icon swap.

:ralin, do you need anything from me?

Comment 90

[Ray Lin[:ralin]]

Thanks Tim and :jaws.

I was thinking if we can have further ui spec(e.g. menu or icon), then implement on all platforms. However, current patch is almost ready to land once other themes are accomplished. Therefore let us move forward with this ui first.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #88)
> maybe we should land this with the CC icon and come up with something better
> later on?

It's fine to land first since the hard part is done. Some polishment works could leave to other bugs.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #89)
> :ralin, do you need anything from me?

I'll need your review then. Thanks.

Comment 91

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

https://reviewboard.mozilla.org/r/48079/#review49744

Please address the review feedback from my earlier pass, and mark each issue as "fixed" on reviewboard as you fix them so I will know what was fixed and what wasn't. 

The button is missing the styling on Windows (and presumbaly Linux).

When the controls are showing, the closed captions should not appear behind the controls. They should be moved up vertically so that they are still readable when the control bar is showing, and they should move down vertically when the control bar is hidden.

::: toolkit/content/widgets/videocontrols.xml:1364
(Diff revision 4)
> +                    if (tt.mode === "showing") {
> +                      return true;
> +                    }
> +                  }
> +
> +                  return fasle;

typo, false

::: toolkit/content/widgets/videocontrols.xml:1406
(Diff revision 4)
> +                  tt.index = this.textTracksCount++;
> +
> +                  const label = tt.label || "";
> +                  const ttText = document.createTextNode(label);
> +                  const ttBtn = document.createElement("button");
> + 

nit, trailing space

Comment 92

[Ray Lin[:ralin]]

Attachment: MozReview Request: Bug 887934 - Part 2 - Reposition closed caption according to video control. r?rillian

Review commit: https://reviewboard.mozilla.org/r/54830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54830/

Comment 93

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/4-5/

Comment 94

[Ray Lin[:ralin]]

https://reviewboard.mozilla.org/r/48079/#review49744

Sorry for that, I'll remember it for next better workflow.

In this update, I seperated patch into two parts. 
Part 1: implement layout and switching closed caption function on all platforms.
Part 2: adjust caption box's style when video control is showed or hiden. 

For part2, I'm not sure it is a legitimate approach to pass <videocontrol> to processCues, since caption box is implicit to others. 
Another event called "texttrackchange" is to update caption display when switching texttrack on a pasued video. Due to caption re-render only while video timestamp is updating.

Thanks.

Comment 95

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

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

https://reviewboard.mozilla.org/r/48079/#review52374

A couple notes:

The closed caption button should be brighter when they are enabled. Right now it looks the same as the fullscreen and play button. The button should have opacity:1 when it is enabled or hovered. Otherwise it should be 0.7. If it is :hover:active, then it should be 0.4.

We shouldn't use an orange background when hovering items in the menu. The menu's hovered background color should be rgba(0,0,0,.55) and the [on] background color should be black.

Clicking on text tracks within the menu was toggling playback of the video for me. Interactions within the menu, as well as opening the menu, shouldn't toggle playback.

::: toolkit/content/widgets/videocontrols.xml:289
(Diff revisions 4 - 5)
>                                 "seeking", "seeked", "emptied", "loadedmetadata",
>                                 "error", "suspend", "stalled",
>                                 "mozinterruptbegin", "mozinterruptend" ],
>  
>                  firstFrameShown : false,
> +                controlBarShown : true,

controlBarShown is unused.

::: toolkit/content/widgets/videocontrols.xml:1366
(Diff revision 5)
> +
> +                  this.setClosedCaptionButtonState();
> +                },
> +
> +                initTextTracks : function () {
> +                  this.addNewTextTrack({label: "off"});

This needs to be a localized string, and should be set to "Off"

::: toolkit/themes/windows/global/media/videocontrols.css:31
(Diff revision 5)
>    opacity: 0.7;
>  }
>  
>  .playButton:hover,
>  .muteButton:hover,
> +.closedCaptionButton

This should be .closedCaptionButton:hover

::: toolkit/themes/windows/global/media/videocontrols.css:121
(Diff revision 5)
> +  padding: 0;
> +}
> +
> +.textTrackList[hidden] {
> +  display: none;
> +  transition: transform 250ms ease-in;

This transition won't work because the item is display:none.

::: toolkit/themes/windows/global/media/videocontrols.css:127
(Diff revision 5)
> +}
> +
> +.textTrackList > html|*.textTrackItem {
> +  -moz-appearance: none;
> +  -moz-box-align: start;
> +  text-align: left;

text-align: start;

Comment 96

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

Comment on attachment 8755810 [details]
MozReview Request: Bug 887934 - Part 2 - Reposition closed caption according to video control. r?rillian

Ralph should review these changes.

Comment 97

[Ralph Giles (:rillian)]

Comment on attachment 8755810 [details]
MozReview Request: Bug 887934 - Part 2 - Reposition closed caption according to video control. r?rillian

https://reviewboard.mozilla.org/r/54830/#review52508

::: dom/html/TextTrackManager.cpp:388
(Diff revision 1)
>      }
>    }
> +
> +  if (type.EqualsLiteral("controlbarchange")) {
> +    for (uint32_t i = 0; i< mTextTracks->Length(); i++) {
> +      ((*mTextTracks)[i])->SetCuesDirty();

Seems like this could be slow. Does calling ->SetDirty() instead work?

Comment 98

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/5-6/

Comment 99

[Ray Lin[:ralin]]

Comment on attachment 8755810 [details]
MozReview Request: Bug 887934 - Part 2 - Reposition closed caption according to video control. r?rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54830/diff/1-2/

Comment 100

[Alastor Wu [:alwu][PTO 8/28-9/6]]

Hi, all,

The step5 in the spec [1] mentions that it's optional to recompute the cue position when the control interface changes from invisible to visible. 

Therefore, the cue position shouldn't be changed when the control interface changes from visible to invisible. I also filed a issue [2] to confirm this behavior.

[1] https://w3c.github.io/webvtt/#processing-model
[2] https://github.com/w3c/webvtt/issues/294

Comment 101

[Ray Lin[:ralin]]

(In reply to Alastor Wu [:alwu] from comment #100)
It more reasonable to add an empty cue instead of restyling directly. Perhaps we don't need part 2 patch to fulfill this behavior.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #95)
> Comment on attachment 8743870 [details]
> MozReview Request: Bug 887934 - Part 1 - Add closed caption button to video
> control. r?jaws
> 
Thank you :jaws. The issues are fixed in the main, but there's a problem with making CC button brighter when enabled. Due to the CC button PNG is partial transparent which affected by background color, it could not be as bright as volume stack even set its opacity to 1.


:shorlander, could you provide me a new icon which is only background transparent? Thanks

Comment 102

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

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

https://reviewboard.mozilla.org/r/48079/#review53154

::: toolkit/content/widgets/videocontrols.xml:1309
(Diff revision 6)
> +                toggleClosedCaption : function () {
> +                  if (this.video.textTracks.length === 1) {
> +                    const lastTTIdx = this.video.textTracks[0].index;
> +
> +                    return this.changeTextTrack(this.isClosedCaptionOn() ? 0 : lastTTIdx);
> +                  }
> +
> +                  if (this.textTrackList.hasAttribute("hidden")) {
> +                    this.textTrackList.removeAttribute("hidden");

Video playback is toggled when there are multiple text tracks (the menu shows or hides).

The control bar will autohide while the text track list is open. The controls should stay visible as long as the text track list is open.

::: toolkit/content/widgets/videocontrols.xml:1339
(Diff revision 6)
> +                onTextTrackAdd : function (trackEvent) {
> +                  this.addNewTextTrack(trackEvent.track);

We should not be showing Chapter items in the subtitle menu. Please only include items that have kind="subtitles". We can add other kinds in the future, or relax this and allow items that don't have a specified kind if we find that sites aren't using the kind attribute.

::: toolkit/content/widgets/videocontrols.xml:1532
(Diff revision 6)
>  
>                      addListener(this.muteButton, "command", this.toggleMute);
>                      addListener(this.playButton, "click", this.clickToPlayClickHandler);
>                      addListener(this.fullscreenButton, "command", this.toggleFullscreen);
>                      addListener(this.clickToPlay, "click", this.clickToPlayClickHandler);
> +                    addListener(this.closedCaptionButton, "click", this.toggleClosedCaption);

The video playback toggles between playing and pausing when the button is clicked and there are multiple text tracks.

::: toolkit/themes/windows/global/media/videocontrols.css:31
(Diff revision 6)
>    opacity: 0.7;
>  }
>  
>  .playButton:hover,
>  .muteButton:hover,
> +.closedCaptionButton:hover

This is missing the comma at the end.

Comment 103

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/6-7/

Comment 104

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/6-7/

Comment 105

[Ray Lin[:ralin]]

https://reviewboard.mozilla.org/r/48079/#review53154

Thank for you review :jaws.


As mentioned in Bug 985915, I want to transfer part 2 to 985915. The previous version of part 2 moving caption box by CSS transform doesn't conform to spec and there's still some work to be done.

> Video playback is toggled when there are multiple text tracks (the menu shows or hides).
> 
> The control bar will autohide while the text track list is open. The controls should stay visible as long as the text track list is open.

make text track list hidden right after control bar fade out now.

> The video playback toggles between playing and pausing when the button is clicked and there are multiple text tracks.

I can't inspect this problem on my computer, but I changed the event to "command" which is the same as muteButton. Could you try again?

Comment 106

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

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

https://reviewboard.mozilla.org/r/48079/#review54088

Looks good.

The toggling of play / pause when I clicked on the CC button was actually not from your patch. The webpage that I was using to test your patch was doing it, so nothing to worry about there.

Comment 107

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

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

um, double r+ i guess?

Comment 108

[Carsten Book [:Tomcat]]

has problems to apply:

applying bdc32a792292
patching file toolkit/content/widgets/videocontrols.xml
Hunk #8 FAILED at 1502
1 out of 8 hunks FAILED -- saving rejects to file toolkit/content/widgets/videocontrols.xml.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

could you take a look, thanks!

Comment 109

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am not sure if this will reach Aurora 49, but if so we should get this into relnote.

Release Note Request
[Why is this notable]: User will be able to change subtitle on <video> controls.
[Suggested wording]: Built-in video controls now offers subtitles of your choice (if available).
[Links (documentation, blog post, etc)]: N/A

Comment 110

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/7-8/

Comment 111

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/8-9/

Comment 112

[Ray Lin[:ralin]]

(In reply to Carsten Book [:Tomcat] from comment #108)
> has problems to apply:
> 
> applying bdc32a792292
> patching file toolkit/content/widgets/videocontrols.xml
> Hunk #8 FAILED at 1502
> 1 out of 8 hunks FAILED -- saving rejects to file
> toolkit/content/widgets/videocontrols.xml.rej
> patch failed to apply
> abort: fix up the working directory and run hg transplant --continue
> 
> could you take a look, thanks!

:Tomcat

Rebased and solved merge conflicts. Could you try again? Thanks.

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/28646bde9f7a
Add closed caption button to video control. r=jaws

Comment 114

[Carsten Book [:Tomcat]]

sorry Ray, had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9825738&repo=fx-team

Comment 115

[Ray Lin[:ralin]]

No problem, I'll see what happened. Thank you :tomcat

Comment 116

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c227ee36b484
Backed out changeset 28646bde9f7a for android videocontrols test failures

Comment 117

[Ray Lin[:ralin]]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 118

[Carsten Book [:Tomcat]]

ray, i guess this also caused https://treeherder.mozilla.org/logviewer.html#?job_id=9828609&repo=fx-team

Comment 119

[Ray Lin[:ralin]]

Thanks for your info :tomcat.

Talked to :alwu offline and currently suspect the failure might be related to text track manager (Bug 882718).
:bechen could you help to investigate this? Thanks 

Error messages are around https://treeherder.mozilla.org/logviewer.html#?job_id=22157093&repo=try#L75700-L75725
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46270c30106ebbb9d6d144723badde7532c1c02f&selectedJob=22157093

Comment 120

[Ray Lin[:ralin]]

Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48079/diff/9-10/

Comment 121

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a1746be133df
Add closed caption button to video control. r=jaws

Comment 122

[Carsten Book [:Tomcat]]

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

Comment 123

[herrernst]

Hi,
sorry for the late reply, I've only had the chance to look at the code now.
If I'm reading that correctly, you only show the button if there's a track whose kind is "subtitles". Maybe you should also look for tracks of kind "captions".

HTML5 states:

"subtitles": Transcription or translation of the dialogue, suitable for when the sound is available but not understood (e.g. because the user does not understand the language of the media resource's audio track). Overlaid on the video.
"captions": Transcription or translation of the dialogue, sound effects, relevant musical cues, and other relevant audio information, suitable for when sound is unavailable or not clearly audible (e.g. because it is muted, drowned-out by ambient noise, or because the user is deaf). Overlaid on the video; labeled as appropriate for the hard-of-hearing.
https://www.w3.org/TR/html5/embedded-content-0.html#attr-track-kind

There's also this code example:
<video src="brave.webm">
 <track kind=subtitles src=brave.en.vtt srclang=en label="English">
 <track kind=captions src=brave.en.hoh.vtt srclang=en label="English for the Hard of Hearing">
 <track kind=subtitles src=brave.fr.vtt srclang=fr lang=fr label="Français">
 <track kind=subtitles src=brave.de.vtt srclang=de lang=de label="Deutsch">
</video>

This note about WCAG 2.0 also uses "captions":
http://www.w3.org/TR/2016/NOTE-WCAG20-TECHS-20160317/H95.html

So I think (although I'm not an expert) you should consider both kinds, subtitles and captions.

Comment 124

[yonas]

herrernst, thanks, that's a good point. I think you should create a new issue and add the link to it here.

Comment 125

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

I have filed bug 1281888 for the "captions" support.

Comment 126

[herrernst]

Thanks!

Comment 127

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381