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

feat: Add document picture-in-picture support #8113

Merged
merged 21 commits into from
Apr 4, 2023

Conversation

mister-ben
Copy link
Contributor

@mister-ben mister-ben commented Feb 8, 2023

Description

Implements Document Picture in Picture API, fixes #8108

Preview at https://deploy-preview-8113--videojs-preview.netlify.app/sandbox/docpip.html

Specific Changes proposed

Adds a enableDocumentPictureInPicture player option, to use the doc PIP API where available.
Inserts an el with the player el's classes as a placeholder. Adds poster and titlebar to the placeholder if present.

To do / to discuss

  • Tests
  • Docs - feat: Add enablePictureInPicture option videojs.com#168
  • Aspect ratio not respected by PIP window Tracked in Chromium https://crbug.com/1407629
  • Events
  • Fullscreen button from PIP window - can we trigger fuullscreen from here, otherwise disable
  • Responsive layout styles needed to resize plaeholder text on small players?
  • Exit PIP button needed in PIP window, as the window UI already has two buttons
  • User expectations that disablePictureInPicture will also disable this?
  • Translations

This is implemented so that the current API / control triggers either window.documentPictureInPicture (docPiP) or video.requestPictureInPicure (elPiP). The same events will fire. Promises will be returned when entering and exiting.

  • docPiP is opt-in with the enableDocumentPictureInPicture option.
  • elPiP remains opt-out with the disablePictureInPicture option.
  • if docPiP is enabled and available, and elPiP is not disabled but available, docPiP is used.

The assumption is an integrator will generally prefer docPiP, so controls, ads, etc can be used. Therefore disablePictureInPicture does not disable docPiP, even if that may initially seem unintuitive.

@mister-ben mister-ben marked this pull request as draft February 8, 2023 09:00
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #8113 (3b17e70) into main (35c539d) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8113      +/-   ##
==========================================
+ Coverage   82.11%   82.23%   +0.12%     
==========================================
  Files         112      112              
  Lines        7402     7430      +28     
  Branches     1785     1791       +6     
==========================================
+ Hits         6078     6110      +32     
+ Misses       1324     1320       -4     
Impacted Files Coverage Δ
src/js/control-bar/picture-in-picture-toggle.js 83.87% <100.00%> (+4.56%) ⬆️
src/js/player.js 90.16% <100.00%> (+0.36%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/js/player.js Outdated Show resolved Hide resolved
sandbox/docpip.html.example Outdated Show resolved Hide resolved
src/js/control-bar/picture-in-picture-toggle.js Outdated Show resolved Hide resolved
src/js/player.js Show resolved Hide resolved
src/js/player.js Show resolved Hide resolved
src/js/player.js Outdated Show resolved Hide resolved
src/js/player.js Outdated Show resolved Hide resolved
@mister-ben mister-ben marked this pull request as ready for review March 7, 2023 16:59
src/js/control-bar/picture-in-picture-toggle.js Outdated Show resolved Hide resolved
src/js/control-bar/picture-in-picture-toggle.js Outdated Show resolved Hide resolved
src/js/player.js Outdated Show resolved Hide resolved
src/js/player.js Outdated Show resolved Hide resolved
src/js/player.js Show resolved Hide resolved
@beaufortfrancois
Copy link
Contributor

It seems like all TODOs have been checked. Is there anything left before merging this PR?

@amtins
Copy link
Contributor

amtins commented Mar 30, 2023

@mister-ben this feature is absolutely amazing 🥇

I was testing the feature and I came across a case that does not seem to be supported.

With Apple's bipbop the audio menu is cut off since there is no longer a full screen button next to the audio or subtitles button.
Screenshot from 2023-03-30 13-42-07

When the player is not in picture-in-picture document it is not an issue because the menu is able to overflow.
Screenshot from 2023-03-30 13-43-12

left: -3em; // (Width of vjs-menu - width of button) / 2

Comment on lines +12 to +15
.vjs-pip-window .vjs-menu-button-popup .vjs-menu {
left: unset;
right: 1em; // Extra offset for last menu button in pip window, as fullscreen button not present
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider this temporary. This needs a better solution longer term that only modifies the last visible menu, but this situation could apply in situations other than PiP so deserves tackling separately to thi PR.

@misteroneill misteroneill merged commit 0c72805 into videojs:main Apr 4, 2023
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Use Document Picture-in-Picture Web API
4 participants