Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(FEC-11655): System highlights 2 options (captions) after clicking the "Change media" button on Safari #612

Merged
merged 2 commits into from
Nov 1, 2021

Conversation

JonathanTGold
Copy link
Contributor

@JonathanTGold JonathanTGold commented Oct 31, 2021

Description of the Changes

issue: look at _getParsedTextTracks() on https://github.com/kaltura/playkit-js/blob/master/src/engines/html5/media-source/adapters/native-adapter.js#L696
when this method called at the second time (like after calling changeMedie() in this case) the textTracks including the external text trcak.

and the first condition evaluates to false and therefore the indexes start from 1 instead of 0 which leads to an incorrect index value (that is - same index for two different tracks) on the getExternalTracks() method in (https://github.com/kaltura/playkit-js/blob/master/src/track/external-captions-handler.js#L118) since it is relying on the textTracks.langth.

and that bring the method the _markActiveTrack(track: Track) method on https://github.com/kaltura/playkit-js/blob/master/src/player.js#L2414 to mark both tracks as active

fix: in the native adapter not relying on the for index which progresses regardless of actual track index

solves: FEC-11655

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

…s 2 options (captions) after clicking the "Change media" button on Safari
@JonathanTGold JonathanTGold requested review from Yuvalke, OrenMe, yairans, SivanA-Kaltura and RoyBregman and removed request for Yuvalke and OrenMe October 31, 2021 14:57
@RoyBregman
Copy link
Contributor

RoyBregman commented Nov 1, 2021

Please fix Title to "fix(FEC-11655):..."
Besides that LGTM

Yuvalke
Yuvalke previously approved these changes Nov 1, 2021
@Yuvalke
Copy link
Contributor

Yuvalke commented Nov 1, 2021

@JonathanTGold u can change the naming from index to native_text_track_index something more meaningful 👍

@JonathanTGold JonathanTGold changed the title fix:(FEC-11655): V3 - External captions - VTT - HLS: System highlights 2 options (captions) after clicking the "Change media" button on Safari fix(FEC-11655): V3 - External captions - VTT - HLS: System highlights 2 options (captions) after clicking the "Change media" button on Safari Nov 1, 2021
@JonathanTGold JonathanTGold requested review from Yuvalke and RoyBregman and removed request for RoyBregman November 1, 2021 08:54
@JonathanTGold JonathanTGold changed the title fix(FEC-11655): V3 - External captions - VTT - HLS: System highlights 2 options (captions) after clicking the "Change media" button on Safari fix(FEC-11655): System highlights 2 options (captions) after clicking the "Change media" button on Safari Nov 1, 2021
@JonathanTGold JonathanTGold merged commit ef16680 into master Nov 1, 2021
@JonathanTGold JonathanTGold deleted the FEC-11655 branch November 1, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants