Picture-in-picture #171
Picture-in-picture #171
Conversation
|
I've requested design help internally. |
| pipButton.addEventListener('click', async () => { | ||
| pipButton.disabled = true; | ||
| try { | ||
| if (this !== document.pictureInPictureElement) { |
There was a problem hiding this comment.
Nit: I'd use this.videoElement to be explicit as it's the <video> sub element that is in Picture-in-Picture. +1 for consistency with this.videoElement.requestPictureInPicture() ;)
There was a problem hiding this comment.
@beaufortfrancois That is not going to work, actually, as pictureInPictureElement can't refer to an element within a shadowRoot tree.
https://w3c.github.io/picture-in-picture/#documentorshadowroot-extension
The browser is supposed to run a retargeting algorithm to define a node that will be used instead. In this case it's the custom element itself.
It's an interesting caveat, I think. And it caught me by surprise, too.
There was a problem hiding this comment.
My bad, you're absolutely right. I forgot about that.
|
Implemented a design update:
Screencast: kino-pip-design.mp4 |
| @@ -345,6 +353,9 @@ export default class extends HTMLElement { | |||
|
|
|||
| this.videoElement.addEventListener('loadedmetadata', setPipButton); | |||
| this.videoElement.addEventListener('emptied', setPipButton); | |||
| this.videoElement.addEventListener('enterpictureinpicture', () => this.classList.add(PIP_CLASSNAME)); | |||
There was a problem hiding this comment.
Nit: It would be great if the PiP floating button would also change shape when video is playing in picture in picture.
There was a problem hiding this comment.
@beaufortfrancois What do you mean by "change shape"? Like change the icon from the current "Enter PiP" icon to some kind of "Exit PiP" icon?
There was a problem hiding this comment.
@beaufortfrancois Gotcha! Added in e484469. Here's a short screencast:
kino-exit-pip-icon.mp4
(This one's on me, our designer has originally designed both icons, but I missed it during implementation. 😐)
There was a problem hiding this comment.
LGTM. Thank you @dero! See #171 (comment) as well
@beaufortfrancois I usually do that every time, but the "Enter PiP" icon seems to have slipped through. Thanks for noticing! Updated in 8e34fda. |
|
This PR looks good! Thank you @dero! |
|
Bug related to this feature was found: #178 |
|
LGTM. PiP displays one video at a time, and if a second video start on PiP, the first one stops. |
Summary
Introduces basic implementation of the programmatic picture-in-picture experience.
kino-basic-pip.mp4
To-dos
† Do we design and implement fully custom video player UI that would allow us to provide users with single controls UI and also allow us to be in control of which buttons are rendered in the UI? Or do we rather keep the PiP button as a control separate from the default video player controls?
Fixes #53