Skip to content

Commit

Permalink
fix: Disable PIP if tech doesn't support it (videojs#6678)
Browse files Browse the repository at this point in the history
The requestPictureInPicture API and button currently assume thta if the browser supports the PIP API, the tech supports it. This results in a broken button with certain techs, such as youtube or HTML5 with an audio el.

Checks if disablePictureInPicture is exactly false. If true it's disabled and if undefined the tech does not support it.

Fixes videojs#6398.
  • Loading branch information
mister-ben authored and edirub committed Jun 6, 2023
1 parent 383a179 commit da54e92
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 8 deletions.
6 changes: 3 additions & 3 deletions src/js/control-bar/picture-in-picture-toggle.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ class PictureInPictureToggle extends Button {
* or on value returned by player.disablePictureInPicture() method.
*/
handlePictureInPictureEnabledChange() {
if (!document.pictureInPictureEnabled || this.player_.disablePictureInPicture()) {
this.disable();
} else {
if (document.pictureInPictureEnabled && this.player_.disablePictureInPicture() === false) {
this.enable();
} else {
this.disable();
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -3035,7 +3035,7 @@ class Player extends Component {
* A promise with a Picture-in-Picture window.
*/
requestPictureInPicture() {
if ('pictureInPictureEnabled' in document) {
if ('pictureInPictureEnabled' in document && this.disablePictureInPicture() === false) {
/**
* This event fires when the player enters picture in picture mode
*
Expand Down
5 changes: 3 additions & 2 deletions src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -789,12 +789,13 @@ class Tech extends Component {
}

/**
* A method to check for the presence of the 'disablePictureInPicture' <video> property.
* A method to check for the value of the 'disablePictureInPicture' <video> property.
* Defaults to true, as it should be considered disabled if the tech does not support pip
*
* @abstract
*/
disablePictureInPicture() {
return false;
return true;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions test/unit/controls.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ QUnit.test('Picture-in-Picture control enabled property value should be correct

assert.equal(pictureInPictureToggle.enabled_, false, 'pictureInPictureToggle button should be disabled after creation');

if ('pictureInPictureEnabled' in document) {
if ('pictureInPictureEnabled' in document && player.disablePictureInPicture() === false) {
player.isInPictureInPicture(true);
player.trigger('enterpictureinpicture');
assert.equal(pictureInPictureToggle.enabled_, true, 'pictureInPictureToggle button should be enabled after triggering an enterpictureinpicture event');
Expand Down Expand Up @@ -203,7 +203,7 @@ QUnit.test('Picture-in-Picture control enabled property value should be correct

assert.equal(pictureInPictureToggle.enabled_, false, 'pictureInPictureToggle button should be disabled after creation');

if ('pictureInPictureEnabled' in document) {
if ('pictureInPictureEnabled' in document && player.disablePictureInPicture() === false) {
player.trigger('loadedmetadata');
assert.equal(pictureInPictureToggle.enabled_, true, 'pictureInPictureToggle button should be enabled after triggering an loadedmetadata event');
} else {
Expand Down
31 changes: 31 additions & 0 deletions test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2258,3 +2258,34 @@ QUnit.test('Should accept multiple calls to currentTime after player initializat
assert.ok(spyCurrentTime.calledAfter(spyInitTime), 'currentTime was called on canplay event listener');
assert.equal(player.currentTime(), 800, 'The last value passed is stored as the currentTime value');
});

QUnit.test('Should only allow requestfullscreen if the tech supports it', function(assert) {
const player = TestHelpers.makePlayer({});
let count = 0;

player.tech_.el_ = {
disablePictureInPicture: false,
requestPictureInPicture() {
count++;
}
};

player.tech_.requestPictureInPicture = function() {
return player.tech_.el_.requestPictureInPicture();
};
player.tech_.disablePictureInPicture = function() {
return this.el_.disablePictureInPicture;
};

player.requestPictureInPicture();
assert.equal(count, 1, 'requestPictureInPicture passed through to supporting tech');

player.tech_.el_.disablePictureInPicture = true;
player.requestPictureInPicture();
assert.equal(count, 1, 'requestPictureInPicture not passed through when disabled on tech');

delete player.tech_.el_.disablePictureInPicture;
player.requestPictureInPicture();
assert.equal(count, 1, 'requestPictureInPicture not passed through when tech does not support');

});

0 comments on commit da54e92

Please sign in to comment.