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(picture-in-picture-control): hide the component in non-compatible browsers #7899

Merged
merged 2 commits into from
May 31, 2023
Merged

fix(picture-in-picture-control): hide the component in non-compatible browsers #7899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3329b2f
refactor(picture-in-picture): audio mode change handler method
amtins May 21, 2023
e135cc7
fix(picture-in-picture-toggle): hide the component in non-compatible …
amtins May 21, 2023
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
10 changes: 1 addition & 9 deletions src/js/control-bar/control-bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
* @file control-bar.js
*/
import Component from '../component.js';
import document from 'global/document';

// Required children
import './play-toggle.js';
Expand Down Expand Up @@ -73,17 +72,10 @@ ControlBar.prototype.options_ = {
'descriptionsButton',
'subsCapsButton',
'audioTrackButton',
'pictureInPictureToggle',
'fullscreenToggle'
]
};

if ('exitPictureInPicture' in document) {
ControlBar.prototype.options_.children.splice(
ControlBar.prototype.options_.children.length - 1,
0,
'pictureInPictureToggle'
);
}

Component.registerComponent('ControlBar', ControlBar);
export default ControlBar;
54 changes: 38 additions & 16 deletions src/js/control-bar/picture-in-picture-toggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,10 @@ class PictureInPictureToggle extends Button {
*/
constructor(player, options) {
super(player, options);

this.on(player, ['enterpictureinpicture', 'leavepictureinpicture'], (e) => this.handlePictureInPictureChange(e));
this.on(player, ['disablepictureinpicturechanged', 'loadedmetadata'], (e) => this.handlePictureInPictureEnabledChange(e));

this.on(player, ['loadedmetadata', 'audioonlymodechange', 'audiopostermodechange'], () => {
// This audio detection will not detect HLS or DASH audio-only streams because there was no reliable way to detect them at the time
const isSourceAudio = player.currentType().substring(0, 5) === 'audio';

if (isSourceAudio || player.audioPosterMode() || player.audioOnlyMode()) {
if (player.isInPictureInPicture()) {
player.exitPictureInPicture();
}
this.hide();
} else {
this.show();
}

});
this.on(player, ['loadedmetadata', 'audioonlymodechange', 'audiopostermodechange'], () => this.handlePictureInPictureAudioModeChange());

// TODO: Deactivate button on player emptied event.
this.disable();
Expand All @@ -56,7 +43,30 @@ class PictureInPictureToggle extends Button {
* The DOM `className` for this object.
*/
buildCSSClass() {
return `vjs-picture-in-picture-control ${super.buildCSSClass()}`;
return `vjs-picture-in-picture-control vjs-hidden ${super.buildCSSClass()}`;
}

/**
* Displays or hides the button depending on the audio mode detection.
* Exits picture-in-picture if it is enabled when switching to audio mode.
*/
handlePictureInPictureAudioModeChange() {
// This audio detection will not detect HLS or DASH audio-only streams because there was no reliable way to detect them at the time
const isSourceAudio = this.player_.currentType().substring(0, 5) === 'audio';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably update isAudio_ when this matches as the source is set, then use isAudio() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mister-ben it make sens. Your suggestion is to set the audio flag somewhere in

video.js/src/js/player.js

Lines 1582 to 1629 in f1558c6

handleTechSourceset_(event) {
// only update the source cache when the source
// was not updated using the player api
if (!this.changingSrc_) {
let updateSourceCaches = (src) => this.updateSourceCaches_(src);
const playerSrc = this.currentSource().src;
const eventSrc = event.src;
// if we have a playerSrc that is not a blob, and a tech src that is a blob
if (playerSrc && !(/^blob:/).test(playerSrc) && (/^blob:/).test(eventSrc)) {
// if both the tech source and the player source were updated we assume
// something like @videojs/http-streaming did the sourceset and skip updating the source cache.
if (!this.lastSource_ || (this.lastSource_.tech !== eventSrc && this.lastSource_.player !== playerSrc)) {
updateSourceCaches = () => {};
}
}
// update the source to the initial source right away
// in some cases this will be empty string
updateSourceCaches(eventSrc);
// if the `sourceset` `src` was an empty string
// wait for a `loadstart` to update the cache to `currentSrc`.
// If a sourceset happens before a `loadstart`, we reset the state
if (!event.src) {
this.tech_.any(['sourceset', 'loadstart'], (e) => {
// if a sourceset happens before a `loadstart` there
// is nothing to do as this `handleTechSourceset_`
// will be called again and this will be handled there.
if (e.type === 'sourceset') {
return;
}
const techSrc = this.techGet('currentSrc');
this.lastSource_.tech = techSrc;
this.updateSourceCaches_(techSrc);
});
}
}
this.lastSource_ = {player: this.currentSource().src, tech: event.src};
this.trigger({
src: event.src,
type: 'sourceset'
});
}

Do you recommend doing it in another PR? If yes with what kind of scope? feat, refactor or fix?

I would tend to create another PR to keep modification more atomic. This would allow to try to better handle the HLS and Dash audio.

Copy link
Contributor

@mister-ben mister-ben May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, should be a separate PR. Digging a bit more I think isAudio() and its purpose need a bit of a rethink and discussion, so this PR shouldn't wait for that.

const isAudioMode =
isSourceAudio || this.player_.audioPosterMode() || this.player_.audioOnlyMode();

if (!isAudioMode) {
this.show();

return;
}

if (this.player_.isInPictureInPicture()) {
this.player_.exitPictureInPicture();
}

this.hide();
}

/**
Expand Down Expand Up @@ -117,6 +127,18 @@ class PictureInPictureToggle extends Button {
}
}

/**
* Show the `Component`s element if it is hidden by removing the
* 'vjs-hidden' class name from it only in browsers that support the Picture-in-Picture API.
*/
show() {
// Does not allow to display the pictureInPictureToggle in browsers that do not support the Picture-in-Picture API, e.g. Firefox.
if (typeof document.exitPictureInPicture !== 'function') {
return;
}

super.show();
}
}

/**
Expand Down
30 changes: 28 additions & 2 deletions test/unit/controls.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,11 @@ QUnit.test('Picture-in-Picture control is hidden when the source is audio', func
player.src({src: 'example.mp4', type: 'video/mp4'});
player.trigger('loadedmetadata');

assert.notOk(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is not hidden initially');
if (document.exitPictureInPicture) {
assert.notOk(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is not hidden initially');
} else {
assert.ok(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is hidden if PiP is not supported');
}

player.src({src: 'example1.mp3', type: 'audio/mp3'});
player.trigger('loadedmetadata');
Expand All @@ -316,7 +320,11 @@ QUnit.test('Picture-in-Picture control is displayed if docPiP is enabled', funct
player.src({src: 'example.mp4', type: 'video/mp4'});
player.trigger('loadedmetadata');

assert.notOk(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is not hidden');
if (document.exitPictureInPicture) {
assert.notOk(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is not hidden');
} else {
assert.ok(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is hidden if PiP is not supported');
}

player.dispose();
pictureInPictureToggle.dispose();
Expand All @@ -325,6 +333,24 @@ QUnit.test('Picture-in-Picture control is displayed if docPiP is enabled', funct
}
});

QUnit.test('Picture-in-Picture control should only be displayed if the browser supports it', function(assert) {
const player = TestHelpers.makePlayer();
const pictureInPictureToggle = new PictureInPictureToggle(player);

player.trigger('loadedmetadata');

if (document.exitPictureInPicture) {
// Browser that does support PiP
assert.false(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is not hidden');
} else {
// Browser that does not support PiP
assert.true(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is hidden');
}

player.dispose();
pictureInPictureToggle.dispose();
});

QUnit.test('Fullscreen control text should be correct when fullscreenchange is triggered', function(assert) {
const player = TestHelpers.makePlayer({controlBar: false});
const fullscreentoggle = new FullscreenToggle(player);
Expand Down
15 changes: 12 additions & 3 deletions test/unit/player.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3150,15 +3150,20 @@ QUnit.test('audioOnlyMode(true/false) hides/shows video-specific control bar com
controlBar.getChild('ChaptersButton').update();

player.trigger('ready');
player.trigger('loadedmetadata');
player.hasStarted(true);

// Show all control bar children
allChildren.forEach(child => {
const el = controlBar.getChild(child) && controlBar.getChild(child).el_;

if (el) {
// Sanity check that component is showing
assert.notEqual(TestHelpers.getComputedStyle(el, 'display'), 'none', `${child} is initially visible`);
if (!document.exitPictureInPicture && child === 'PictureInPictureToggle') {
assert.equal(TestHelpers.getComputedStyle(el, 'display'), 'none', `${child} is not visible if PiP is not supported`);
} else {
// Sanity check that component is showing
assert.notEqual(TestHelpers.getComputedStyle(el, 'display'), 'none', `${child} is initially visible`);
}
}
});

Expand Down Expand Up @@ -3187,7 +3192,11 @@ QUnit.test('audioOnlyMode(true/false) hides/shows video-specific control bar com
const el = controlBar.getChild(child) && controlBar.getChild(child).el_;

if (el) {
assert.notEqual(TestHelpers.getComputedStyle(el, 'display'), 'none', `${child} is shown`);
if (!document.exitPictureInPicture && child === 'PictureInPictureToggle') {
assert.equal(TestHelpers.getComputedStyle(el, 'display'), 'none', `${child} is not visible if PiP is not supported`);
} else {
assert.notEqual(TestHelpers.getComputedStyle(el, 'display'), 'none', `${child} is shown`);
}
}
});
})
Expand Down