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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: reyre, Assigned: ralin, Mentored, NeedInfo)

Tracking

(Depends on 3 bugs, Blocks 2 bugs, {access})

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(6 attachments, 7 obsolete attachments)

262.15 KB, image/png
Details
266.98 KB, image/png
Details
583 bytes, image/png
Details
1.30 KB, image/png
Details
58 bytes, text/x-review-board-request
jaws
: review+
Details
413.81 KB, image/png
Details
Reporter

Description

6 years ago
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.
Reporter

Updated

6 years ago
Blocks: webvtt
Reporter

Updated

6 years ago
Assignee: nobody → rick.eyre
Reporter

Comment 1

5 years ago
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.
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles)
Adding Sabrina who expressed interest in the ui aspect.
Reporter

Comment 5

5 years ago
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?
Attachment #8381527 - Flags: review?(giles)
Reporter

Comment 6

5 years ago
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.
Attachment #8381528 - Flags: feedback?(giles)
Reporter

Updated

5 years ago
Attachment #8381527 - Attachment description: Expose a flag on the MediaElement that controls if TextTracks are enabled → Part 1: Expose a flag on the MediaElement that controls if TextTracks are enabled
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.
Attachment #8381527 - Flags: review?(giles)
Adding phlsa and boriss for ui review, per madhava's suggestion.
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
Jared may have something to add here, he maintains the video controls.
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).
Reporter

Comment 11

5 years ago
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?
Yes, we should do the same. Until we have a proper icon you can reuse one of the videocontrols icons.
(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 :)
Flags: needinfo?(shorlander)
Reporter

Comment 14

5 years ago
Posted patch WIP (obsolete) — Splinter Review
Attachment #8381527 - Attachment is obsolete: true
Attachment #8381528 - Attachment is obsolete: true
Attachment #8381528 - Flags: feedback?(giles)
Reporter

Comment 15

5 years ago
Posted image basic on off.png
Reporter

Comment 16

5 years ago
Posted image not flexible.png
Reporter

Updated

5 years ago
Depends on: 977299
Thanks! Can you also attach an svg version? The other controls scale with screen res.
Reporter

Comment 20

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

Comment 21

5 years ago
See comment 20.
Flags: needinfo?(jaws)
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?
Flags: needinfo?(jaws) → needinfo?(dannychen210)

Comment 23

5 years ago
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.
Flags: needinfo?(rick.eyre)
Flags: needinfo?(jaws)
Reporter

Comment 24

5 years ago
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 :-).
Flags: needinfo?(rick.eyre)
Assignee: rick.eyre → eric.phan
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Flags: needinfo?(dannychen210)
Whiteboard: [mentor=padenot]

Comment 25

5 years ago
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)
Flags: needinfo?(rick.eyre)

Comment 26

5 years ago
Posted patch menu, but no css (obsolete) — Splinter Review

Updated

5 years ago
Flags: needinfo?(rick.eyre)
Reporter

Comment 27

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

Comment 28

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

Comment 29

5 years ago
Awesome that's great to hear :-).

Comment 30

5 years ago
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 ?
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.
Flags: needinfo?(jaws)
Reporter

Comment 32

5 years ago
Yeah, Jared's probably the one you want to talk to Richard.
Reporter

Comment 33

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

Updated

5 years ago
Attachment #8440656 - Attachment is obsolete: true

Comment 34

5 years ago
Posted patch functions_ok_but_miss_css_v1 (obsolete) — Splinter Review
Attachment #8440751 - Flags: review?(rick.eyre)
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!
Flags: needinfo?(jaws)

Comment 36

5 years ago
Indeed it helps !
Thanks
Reporter

Comment 37

5 years ago
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.
Attachment #8440751 - Flags: review?(rick.eyre) → review-

Comment 38

5 years ago
(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?

Updated

5 years ago
Attachment #8440751 - Attachment is obsolete: true

Comment 39

5 years ago
Posted patch function-css-ok (obsolete) — Splinter Review
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.
Attachment #8441246 - Flags: review?(rick.eyre)
Attachment #8441246 - Flags: review?(jaws)
Mentor: paul
Whiteboard: [mentor=padenot]
Reporter

Comment 40

5 years ago
(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.
Flags: needinfo?(giles)
Reporter

Comment 41

5 years ago
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?
Mentor: paul
Attachment #8441246 - Flags: review?(rick.eyre) → review-
(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!
Flags: needinfo?(giles)
Reporter

Comment 43

5 years ago
(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

5 years ago
(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 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).
Attachment #8441246 - Flags: review?(jaws)
Reporter

Comment 46

5 years ago
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
Reporter

Comment 47

5 years ago
(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.
Mentor: rick.eyre, jaws

Comment 48

5 years ago
> @@ +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?
(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

5 years ago
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.
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

5 years ago
Hi, what is the status of this work?
Are you still working on this, Eric?
Flags: needinfo?(eric.phan)

Comment 54

5 years ago
(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.
Flags: needinfo?(eric.phan)

Updated

4 years ago
Assignee: eric.phan → nobody
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)
Flags: needinfo?(jaws)
Keywords: access

Comment 56

4 years ago
This is quite an important addition, it would be good to see this getting worked on.
(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.
Flags: needinfo?(jaws)
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.
Also, Readium Foundation needs this feature for accessibility with their cloud-based Firefox EPUB reader.
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.
Added the sec508 keyword to flag this as having an impact on those requirements, or the proposed refresh thereof.
Keywords: sec508
Assignee: nobody → jaws

Updated

4 years ago
Duplicate of this bug: 1069384

Comment 63

3 years ago
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.
Duplicate of this bug: 515898
Tim, would you or your team want to pick this up?
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(timdream)
Sounds like something Ray could help! Ray, please told me otherwise if your plate is full.
Flags: needinfo?(timdream) → needinfo?(ralin)
Assignee

Comment 67

3 years ago
No problem, I can take care of this issue. Thanks.
Flags: needinfo?(ralin)
Assignee

Updated

3 years ago
Assignee: nobody → ralin

Comment 69

3 years ago
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.
Assignee

Comment 71

3 years ago
(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!
Assignee

Comment 72

3 years ago
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/
Assignee

Comment 73

3 years ago
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.
Flags: needinfo?(philipp)

Comment 74

3 years ago
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

3 years ago
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!
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.
Assignee

Comment 77

3 years ago
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/
Assignee

Comment 78

3 years ago
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!
Flags: needinfo?(jaws)

Comment 79

3 years ago
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 !
(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

3 years ago
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

3 years ago
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!
(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.
Flags: needinfo?(jaws)
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/closeCaptionButton@2x.png                (media/closeCaptionButton@2x.png)

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
Assignee

Comment 85

3 years ago
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.
Flags: needinfo?(shorlander)
Assignee

Comment 86

3 years ago
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/
(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.
Flags: needinfo?(philipp)
Blocks: 1271768
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?
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?
Flags: needinfo?(ralin)
Assignee

Comment 90

3 years ago
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.
Flags: needinfo?(ralin)
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
Assignee

Comment 92

3 years ago
Review commit: https://reviewboard.mozilla.org/r/54830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54830/
Attachment #8743870 - Attachment description: MozReview Request: Bug 887934 - [WIP] CC in <video> controls → MozReview Request: Bug 887934 - Part 1 - Add closed caption button to video control. r?jaws
Attachment #8755810 - Flags: review?(jaws)
Attachment #8743870 - Flags: review?(jaws)
Assignee

Comment 93

3 years ago
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/
Assignee

Comment 94

3 years ago
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 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 on attachment 8755810 [details]
MozReview Request: Bug 887934 - Part 2 - Reposition closed caption according to video control. r?rillian

Ralph should review these changes.
Attachment #8755810 - Flags: review?(jaws) → review?(giles)

Updated

3 years ago
Blocks: 985915
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?
Attachment #8755810 - Flags: review?(giles) → review+
Assignee

Comment 98

3 years ago
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/
Attachment #8755810 - Attachment description: MozReview Request: Bug 887934 - Part 2 - Reposition closed caption according to video control. r?jaws → MozReview Request: Bug 887934 - Part 2 - Reposition closed caption according to video control. r?rillian
Attachment #8743870 - Flags: review?(jaws)
Assignee

Comment 99

3 years ago
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/
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
Assignee

Comment 101

3 years ago
(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 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.
Attachment #8743870 - Flags: review?(jaws)
Assignee

Comment 103

3 years ago
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/
Attachment #8743870 - Attachment description: MozReview Request: Bug 887934 - Part 1 - Add closed caption button to video control. r?jaws → MozReview Request: Bug 887934 - Add closed caption button to video control. r?jaws
Attachment #8743870 - Flags: review?(jaws)
Assignee

Comment 104

3 years ago
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/
Attachment #8743870 - Attachment description: MozReview Request: Bug 887934 - Part 1 - Add closed caption button to video control. r?jaws → MozReview Request: Bug 887934 - Add closed caption button to video control. r?jaws
Attachment #8743870 - Flags: review?(jaws)
Assignee

Updated

3 years ago
Attachment #8755810 - Attachment is obsolete: true
Assignee

Comment 105

3 years ago
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 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.
Attachment #8743870 - Flags: review?(jaws) → review+
Comment on attachment 8743870 [details]
Bug 887934 - Add closed caption button to video control.

um, double r+ i guess?
Attachment #8743870 - Flags: review?(jaws) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
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!
Flags: needinfo?(ralin)
Keywords: checkin-needed
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
relnote-firefox: --- → ?
Assignee

Comment 110

3 years ago
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/
Attachment #8743870 - Attachment description: MozReview Request: Bug 887934 - Add closed caption button to video control. r?jaws → Bug 887934 - Add closed caption button to video control.
Attachment #8743870 - Flags: review+
Assignee

Comment 111

3 years ago
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/
Assignee

Updated

3 years ago
Flags: needinfo?(ralin)
Keywords: checkin-needed
Assignee

Comment 112

3 years ago
(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

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/28646bde9f7a
Add closed caption button to video control. r=jaws
Keywords: checkin-needed
sorry Ray, had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9825738&repo=fx-team
Flags: needinfo?(ralin)
Assignee

Comment 115

3 years ago
No problem, I'll see what happened. Thank you :tomcat
Flags: needinfo?(ralin)

Comment 116

3 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c227ee36b484
Backed out changeset 28646bde9f7a for android videocontrols test failures
Assignee

Comment 117

3 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: ? → ---
Assignee

Comment 119

3 years ago
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
Flags: needinfo?(ralin) → needinfo?(bechen)
Depends on: 1280192
Assignee

Comment 120

3 years ago
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/
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 121

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/a1746be133df
Add closed caption button to video control. r=jaws
Keywords: checkin-needed
Assignee

Updated

3 years ago
Blocks: 1281414
Flags: needinfo?(bechen)

Comment 122

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1746be133df
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Comment 123

3 years ago
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

3 years ago
herrernst, thanks, that's a good point. I think you should create a new issue and add the link to it here.
I have filed bug 1281888 for the "captions" support.

Comment 126

3 years ago
Thanks!
Assignee

Updated

3 years ago
Blocks: 1282678
Depends on: 1291009
Depends on: 1291013
Depends on: 1291018
Depends on: 1295412

Updated

3 years ago
Depends on: 1300805

Updated

3 years ago
Depends on: 1327061
Assignee

Updated

3 years ago
See Also: → 1329555

Updated

3 years ago
Depends on: 1327891

Updated

3 years ago
Depends on: 1329117
Assignee

Updated

2 years ago
Depends on: 1332994
You need to log in before you can comment on or make changes to this bug.