fix: trigger change events on remoteTextTrack when nativeTextTrack is set to true #6410
Conversation
| @@ -273,13 +273,26 @@ class Html5 extends Tech { | |||
| return; | |||
| } | |||
| const listeners = { | |||
| change(e) { | |||
| techTracks.trigger({ | |||
| change: (e) => { | |||
There was a problem hiding this comment.
this was changed to an arrow function so that line 294 can be used as is without requiring to get the remoteText getter in the higher scope as we only need it in one usecase.
|
|
||
| techTracks.trigger(event); | ||
|
|
||
| // if we are a text track change event, we should also notify the |
There was a problem hiding this comment.
May be worth adding a note about the alternative approach in case there's ever time to implement it, if it's still seen as a benefit.
There was a problem hiding this comment.
The comment is really long. Maybe I'll short it to with a link here and elaborate a bit more here.
|
Makes sense to add a test. |
|
Friendly ping- what's the status of this PR? Is it awaiting the addition of tests? |
|
A test has been added for this! |
… native text tracks (#6434) Co-authored-by: Kyle Boutette <kyleveB@gmail.com>
|
Looks like the tests are failing now in CI (firefox & IE) |
|
Looks like firefox doesn't like the way that we're loading the track. I even changed it to explicitly set the mode to showing on |
|
Firefox is refusing to add the track to the player in the test. |
|
running the same code outside of the test works but fails in the test 🤔 |
|
It seems like everything should be working with changes I have locally but for whatever reason even though the video element has the track element added to it, it seems like things aren't linked properly when in the test. |
|
I got the test passing once and got excited so when to simply what I had written and now can't get it working again. |
…racks object is refusing to show the track properly
|
@third774 if you have some time to take a look at why firefox isn't working, that would be appreciated, but I don't think it should be a blocker anymore. If you do figure it out after this has been merged, we can always have a separate PR. |
|
@gkatsev if you say so! Thanks for all the help! I'll poke at it a bit and let you know if I make any progress. |
|
videojs/http-streaming#759 depends on this PR, though, they can be merged separately. |
When native text tracks are used, the cues added need to be native cues. It works on conjuction with videojs/video.js#6410. Co-authored-by: Kevin Kipp <kevin.kipp@gmail.com> Co-authored-by: Kyle Boutette <kyleveb@gmail.com>
When native text tracks are used, the cues added need to be native cues. It works on conjuction with videojs/video.js#6410. Co-authored-by: Kevin Kipp <kevin.kipp@gmail.com> Co-authored-by: Kyle Boutette <kyleveb@gmail.com>
When native text tracks are used, the cues added need to be native cues. It works on conjuction with videojs/video.js#6410. Co-authored-by: Kevin Kipp <kevin.kipp@gmail.com> Co-authored-by: Kyle Boutette <kyleveb@gmail.com>
When native text tracks are used, the cues added need to be native cues. It works on conjunction with videojs/video.js#6410. Co-authored-by: Kevin Kipp <kevin.kipp@gmail.com> Co-authored-by: Kyle Boutette <kyleveb@gmail.com>
It seems we have never triggered
changeevents onremoteTextTrackwhen we were using native text tracks. This was a problem for VHS, which exclusively uses text tracks.This makes it so we do trigger the event. Main issue with this change is that it creates a potential for a false positive where a change event was triggered from a non-remote text track but the remoteTextTrack list still received a change event. This issue is mitigated by best practices of looping through the list looking for the modes that you care about.