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.

Sign up for GitHub

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-7935): fix text selection logic with hlsjs #65

Merged
merged 6 commits into from
Jul 12, 2018
Merged

fix(FEC-7935): fix text selection logic with hlsjs #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2c43373
fix(FEC-7935): fix text selection logic with hlsjs
OrenMe Feb 26, 2018
4411be8
Merge remote-tracking branch 'origin/master' into FEC-7935
OrenMe Jul 11, 2018
a9a29b8
use hlsjs `subtitleTrack` to set text track off
OrenMe Jul 11, 2018
0b1e27b
use hlsjs `subtitleDisplay` to set text track disaply mode according …
OrenMe Jul 11, 2018
005ee0d
remove redundednt tests
OrenMe Jul 11, 2018
a3cfcf8
Merge branch 'master' into FEC-7935
OrenMe Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 6 additions & 15 deletions src/hls-adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ export default class HlsAdapter extends BaseMediaSourceAdapter {
adapterConfig.hlsConfig.startPosition = config.playback.startTime;
}
}
if (Utils.Object.hasPropertyPath(config, 'playback.useNativeTextTrack')) {
adapterConfig.subtitleDisplay = Utils.Object.getPropertyPath(config, 'playback.useNativeTextTrack');
Copy link
Contributor

@yairans yairans Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add it on adapterConfig.hlsConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not an hls.js config option - it's an API. I wanted to add it as config first but it doesn't get set on hls.js config...

}
return new this(videoElement, source, adapterConfig);
}

Expand Down Expand Up @@ -202,6 +205,7 @@ export default class HlsAdapter extends BaseMediaSourceAdapter {
this._config.hlsConfig['pLoader'] = pLoader;
}
this._hls = new Hlsjs(this._config.hlsConfig);
this._hls.subtitleDisplay = this._config.subtitleDisplay;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary. see the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment above

this._addBindings();
}

Expand Down Expand Up @@ -479,8 +483,7 @@ export default class HlsAdapter extends BaseMediaSourceAdapter {
*/
selectTextTrack(textTrack: TextTrack): void {
if (textTrack instanceof TextTrack && !textTrack.active && this._videoElement.textTracks) {
this._disableAllTextTracks();
this._videoElement.textTracks[textTrack.index].mode = 'hidden';
this._hls.subtitleTrack = textTrack.index;
HlsAdapter._logger.debug('Text track changed', textTrack);
this._onTrackChanged(textTrack);
}
Expand All @@ -492,7 +495,7 @@ export default class HlsAdapter extends BaseMediaSourceAdapter {
* @public
*/
hideTextTrack(): void {
this._disableAllTextTracks();
this._hls.subtitleTrack = -1;
}

/**
Expand Down Expand Up @@ -644,18 +647,6 @@ export default class HlsAdapter extends BaseMediaSourceAdapter {
}
}

/**
* Disables all the video tag text tracks.
* @returns {void}
* @private
*/
_disableAllTextTracks() {
let vidTextTracks = this._videoElement.textTracks;
for (let i = 0; i < vidTextTracks.length; i++) {
vidTextTracks[i].mode = 'disabled';
}
}

/**
* Handles hls errors.
* @param {any} data - The event data object.
Expand Down
23 changes: 0 additions & 23 deletions test/src/hls-adapter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,29 +210,6 @@ describe('HlsAdapter Instance - Unit', function () {
JSON.parse(JSON.stringify(tracks)).should.deep.equal(allTracks);
});

it('should disable all text tracks', function () {
hlsAdapterInstance._videoElement = {
textTracks: hls_tracks.subtitles,
removeEventListener: () => {}
};
hlsAdapterInstance._disableAllTextTracks();
for (let i = 0; i < hlsAdapterInstance._videoElement.textTracks.length; i++) {
hlsAdapterInstance._videoElement.textTracks[i].mode.should.equal('disabled');
}
});

it('should hide the active text track', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why to remove this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we managed the tracks directly ourselves - so it made sense.
Now we use hlsjs API and it requires loading an actual manifest with text tracks - and we will basically be testing internals of hlsjs - which is out of scope for unit testing

Copy link
Contributor

@yairans yairans Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we just checking that our hideTextTrack does its work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand but the test doesn't reflect it - in our unit testing we inject the text tracks ourselves and not via hlsjs so we actually don't test correctly.
And now that we moved to using hlsjs API itself then the test is both irrelevant(as the testing is black boxed behind hls.js API) and not working as we are not using hlsjs to add and manage the text tracks in the test itself.

hlsAdapterInstance._videoElement = {
textTracks: hls_tracks.subtitles,
removeEventListener: () => {}
};
hlsAdapterInstance._videoElement.textTracks[0].mode = 'showing';
hlsAdapterInstance.hideTextTrack();
for (let i = 0; i < hlsAdapterInstance._videoElement.textTracks.length; i++) {
hlsAdapterInstance._videoElement.textTracks[i].mode.should.equal('disabled');
}
});

it('should enable adaptive bitrate', function () {
hlsAdapterInstance._hls = {
on: function () {
Expand Down