Update display, instead of toggling it #2215

gkatsev · 2015-05-29T20:46:35Z

pam · 2015-05-29T20:55:58Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: bd427ac
Build details: https://travis-ci.org/pam/video.js/builds/64636040

(Please note that this is a fully automated comment.)

gkatsev · 2015-05-29T21:00:35Z

@pam retry

pam · 2015-05-29T21:08:25Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: bd427ac
Build details: https://travis-ci.org/pam/video.js/builds/64638271

(Please note that this is a fully automated comment.)

heff · 2015-05-29T23:12:45Z

src/js/tech/tech.js

    let textTracksChanges = function() {
      let updateDisplay = Fn.bind(this, function() {
-        this.trigger('texttrackchange');
+        player.trigger('texttrackchange');


Are you sure this is needed? There's no player anymore in techs, so we need to find another option. The player does listen for texttrackchange events on the tech. I think this is still actually triggering the event on the tech, because the component init is setting the tech as player.

Not sure. I guess this isn't quite ready. I only looked quickly at this and this was a quick fix for the issue. It worked locally.
Ill take another look later.

I was trying to track this down, but got confused between texttrackchange and cuechange events.

The Captions Settings control explicitly calls an updateDisplay on the textTrackDisplay when a setting is changed. When a cue changes because of the video moving along the timeline, the track fires a cuechange event which causes the tech to fire a texttrackchange event, which seems to just cause the player to fire a texttrackchange event. Nothing fires an updateDisplay on the textTrackDisplay.

EDIT: Okay, the problem is in the migration of textTracksChanges in media/media.js of stable to the one in tech/tech.js of master.

Yep, your EDIT is what when the issue occurs.
Also, I see now what is trying to happen. I went down the rabbithole of only listening to the text tracks events in the display, but that doesn't work anymore because techs own the text tracks now. I might keep the listener in there and then have the tech tell the display to update its listeners.

Sounds good. The event architecture is getting a little opaque; I'll leave it to you core developers, but it'd be nice to have some docs on it at some point in the future ;-)

I might keep the listener in there and then have the tech tell the display to update its listeners.

If there's a way we can do that through an event instead of a direct connection between the tech and the display, that'd be ideal.

The event texttrackchange is triggered by the tech. Then, the player re-trigger it on himself if it is the active tech. I'm not sure what the problem is. Is it happening before the tech is loading?

The main issue was that the display's listener was calling the wrong method. Though, when I changed that, it seemed to not work still.
Also, I'm actually not a fan of the texttrackchange event since it isn't in the spec. Though, perhaps there isn't a better way of doing it.

I might keep the listener in there and then have the tech tell the display to update its listeners.

If there's a way we can do that through an event instead of a direct connection between the tech and the display, that'd be ideal.

If there was an event for tech switching, the display could listen to it :)

Ah, I think the issue is that the updateDisplay function in the tech triggers the texttrackchange event on the wrong object.

I think I have it.

gkatsev · 2015-06-01T18:07:08Z

Ugh, that isn't what I meant to do.

pam · 2015-06-01T18:11:33Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: b624a5c
Build details: https://travis-ci.org/pam/video.js/builds/64957812

(Please note that this is a fully automated comment.)

The actual issue was that we were triggering the events on the incorrect objects. This commit makes sure that `this` is getting set correctly on the correct things.

gkatsev · 2015-06-01T18:15:52Z

Alright, I think I have it. The main issue was that we were triggering the texttrackchange event on the text tracks object instead of on the tech. I've fixed up the references to this.

eXon · 2015-06-01T18:18:58Z

Good catch, probably my fault when I refactored. It looks better now :)

pam · 2015-06-01T18:20:33Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: eeff082
Build details: https://travis-ci.org/pam/video.js/builds/64958864

(Please note that this is a fully automated comment.)

gkatsev · 2015-06-01T18:26:02Z

The error I'm getting locally is a vjs_getProperty error which seems separate from anything I did. Maybe related to #2205?

gkatsev · 2015-06-01T18:26:12Z

@heff thoughts?

dmlap · 2015-06-01T21:18:36Z

@gkatsev I think you're seeing the same error I was getting in #2208

gkatsev · 2015-06-01T21:19:48Z

@dmlap yep, looks like the same issue.

heff · 2015-07-08T21:11:43Z

src/js/tech/tech.js

@@ -234,10 +232,10 @@ class Tech extends Component {
      }
    };

-    tracks.addEventListener('change', textTracksChanges);
+    tracks.addEventListener('change', Fn.bind(this, textTracksChanges));

    this.on('dispose', Fn.bind(this, function() {


this.on already does a Fn.bind(this internally, so this one is redundant.

heff · 2015-07-08T21:15:22Z

Two minor notes, looks good to me.

pam · 2015-07-08T21:42:36Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 61e820b
Build details: https://travis-ci.org/pam/video.js/builds/70134333

(Please note that this is a fully automated comment.)

gkatsev · 2015-07-08T21:44:27Z

@pam retry

pam · 2015-07-08T21:50:11Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 61e820b
Build details: https://travis-ci.org/pam/video.js/builds/70135836

(Please note that this is a fully automated comment.)

Update display, instead of toggling it

bd427ac

heff reviewed May 29, 2015
View reviewed changes

Trigger texttrackchange on correct object

eeff082

The actual issue was that we were triggering the events on the incorrect objects. This commit makes sure that `this` is getting set correctly on the correct things.

gkatsev force-pushed the fix-emulated-captions branch from b624a5c to eeff082 Compare June 1, 2015 18:14

gkatsev mentioned this pull request Jul 8, 2015

v5.0.0-rc.4 Captions issues in IE8/9/10 #2314

Closed

heff reviewed Jul 8, 2015
View reviewed changes

heff added enhancement confirmed labels Jul 8, 2015

gkatsev added 2 commits July 8, 2015 17:30

remove superfluous bind

f320472

bind function on creation not usage

61e820b

gkatsev closed this in 2b79dac Jul 8, 2015

heff mentioned this pull request Jul 8, 2015

video.js captions (not native browser captions) are broken in V5 #2172

Closed

gkatsev deleted the fix-emulated-captions branch August 12, 2015 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update display, instead of toggling it #2215

Update display, instead of toggling it #2215

gkatsev commented May 29, 2015

pam commented May 29, 2015

gkatsev commented May 29, 2015

pam commented May 29, 2015

heff May 29, 2015

gkatsev May 30, 2015

OwenEdwards May 30, 2015

gkatsev Jun 1, 2015

OwenEdwards Jun 1, 2015

heff Jun 1, 2015

eXon Jun 1, 2015

gkatsev Jun 1, 2015

gkatsev Jun 1, 2015

gkatsev Jun 1, 2015

gkatsev commented Jun 1, 2015

pam commented Jun 1, 2015

gkatsev commented Jun 1, 2015

eXon commented Jun 1, 2015

pam commented Jun 1, 2015

gkatsev commented Jun 1, 2015

gkatsev commented Jun 1, 2015

dmlap commented Jun 1, 2015

gkatsev commented Jun 1, 2015

heff Jul 8, 2015

gkatsev Jul 8, 2015

heff commented Jul 8, 2015

pam commented Jul 8, 2015

gkatsev commented Jul 8, 2015

pam commented Jul 8, 2015

Update display, instead of toggling it #2215

Update display, instead of toggling it #2215

Conversation

gkatsev commented May 29, 2015

pam commented May 29, 2015

gkatsev commented May 29, 2015

pam commented May 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Jun 1, 2015

pam commented Jun 1, 2015

gkatsev commented Jun 1, 2015

eXon commented Jun 1, 2015

pam commented Jun 1, 2015

gkatsev commented Jun 1, 2015

gkatsev commented Jun 1, 2015

dmlap commented Jun 1, 2015

gkatsev commented Jun 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Jul 8, 2015

pam commented Jul 8, 2015

gkatsev commented Jul 8, 2015

pam commented Jul 8, 2015