w3c / picture-in-picture

Picture-in-Picture (PiP)
https://w3c.github.io/picture-in-picture
Other
311 stars 39 forks source link

Update explainer to include custom actions. #69

Closed japacible closed 5 years ago

japacible commented 6 years ago

@beaufortfrancois @mounirlamouri PTAL, thanks!

This change adds custom actions to the Picture-in-Picture explainer.

Currently, each action is added and handler set individually. Alternatively, we could set all the actions together, including the handlers. How flexible would we want for action properties (e.g. handler functions) to be updated?

Another option could be bubbling an ‘onpictureinpictureaction’ event and setting an id per action, which means there will be no callback handler set explicitly per action.

Note: MediaImage is reused from the Media Session API.

beaufortfrancois commented 6 years ago

I'd personally be in favor of having an array of Picture-in-Picture controls and simply have a specific event listener on the video element as you've suggested.

This would allow me to reset Picture-in-Picture controls more easily while having one event listener unchanged.

What do you think of this?

const pipControls = [
  {
    id: 'thumbs-up',
    label: 'Thumbs up',
    icons: [ { src: 'http://dummyimage.com/96x96', sizes: '96x96', type: 'image/png' }, …]
  }, {
    id: 'thumbs-down',
    label: 'Thumbs down',
    icons: [ { src: 'http://dummyimage.com/96x96', sizes: '96x96', type: 'image/png' }, …]
  }
];

video.addEventListener('pictureInPictureControlsClick', event => {
  switch (event.id) {
    'thumbs-up': …
    'thumbs-down': …
  };
});

await video.setPictureInPictureControls(pipControls);
const pipWindow = await video.requestPictureInPicture();

Extra thought: Shall we expose read-only pipControls in pipWindow?

japacible commented 6 years ago

An array of controls and a single event listener SGTM. @mounirlamouri , thoughts?

What would be the use case of the proposed read only pipControls?

beaufortfrancois commented 6 years ago

I was thinking this would allow web developers to provide more or less Picture-in-Picture custom controls based on the size of the Picture-in-Picture window. But this is NOT needed actually, they can already do it in the resize event listener.

japacible commented 6 years ago

PTAL, thanks! I updated to reflect using a single array for setting the actions and updated the EventHandler.

beaufortfrancois commented 6 years ago

@jernoble We're thinking about adding custom controls to the Picture-in-Picture window. What do you think of this proposal?

const pipControls = [
  {
    id: 'thumbs-up',
    label: 'Thumbs up',
    icons: [ { src: 'http://dummyimage.com/96x96', sizes: '96x96', type: 'image/png' }, ...]
  }, {
    id: 'thumbs-down',
    label: 'Thumbs down',
    icons: [ { src: 'http://dummyimage.com/96x96', sizes: '96x96', type: 'image/png' }, ...]
  }
];

await video.setPictureInPictureControls(pipControls);

video.addEventListener('pictureinpicturecontrolsclick', event => {
  switch (event.id) {
    'thumbs-up': ...
    'thumbs-down': ...
  };
});
jernoble commented 6 years ago

Isn’t this the bailiwick of the Media Session API? It would be a shame if these custom controls in two places.

beaufortfrancois commented 6 years ago

Isn’t this the bailiwick of the Media Session API? It would be a shame if these custom controls in two places.

@jernoble I also happened to think of the Media Session API as the bailiff for Picture-in-Picture custom controls and realized it wasn't a perfect fit for some situations.

It is true there is some overlap but it's not always the case. You may not want Media Session API controls to show up in media notifications and the Picture-in-Picture window at the same time.

For example, a WebRTC video may decide to omit playback controls in media notifications while providing some custom controls (e.g. "hangup") specifically for the Picture-in-Picture window. Or a music video could provide same playback controls in media notifications and Picture-in-Picture window but may decide to add some extra ones (e.g. "like" and "dislike") specifically for the Picture-in-Picture window.

If a control has to be shown in a media notification and a Picture-in-Picture window it looks fine to me:

// Shows built-in ⏭ button in notification.
navigator.mediaSession.setActionHandler('nexttrack', function() {
  handleNextTrack();
});

const pipControls = [
  {
    id: 'nexttrack',
    label: 'Next Track',
    icons: [ { src: 'http://dummyimage.com/96x96', sizes: '96x96', type: 'image/png' }, ...]
  }
];

// Shows custom ⏭ button in window.
await video.setPictureInPictureControls(pipControls);

video.addEventListener('pictureinpicturecontrolsclick', event => {
  if (event.id === 'nexttrack')
    handleNextTrack();
});

Do you think it would be better to rename video.setPictureInPictureControls to video.setPictureInPictureWindowControls to be more explicit?

beaufortfrancois commented 6 years ago

@jernoble (gentle ping)

jernoble commented 6 years ago

@beaufortfrancois I still don't agree that we need separate implementations for custom media controls on a "per UI" basis. We shouldn't have a "Touch Bar Media Controls" API, a "Headphone Mic Button Media Controls" API, a separate "PiP Window Media Controls" API, etc.

A User Agent should be able to figure out, just on the basis of what controls are exposed by the Media Session, what controls should appear in the PiP window (and all the other places where media controls appear). If there's not enough context available in the Media Session, then IMHO that's on the Media Session spec to address. A WebRTC page should be able to configure their Media Session in such a way that a "hang up" button appears in the Touch Bar, a Notification, and the PiP window without specific, custom controls (defined in different specs) for each.

zouhir commented 6 years ago

Just thinking out loud here, what is going to happen to setPictureInPictureControls([...]); if you allow a document fragment to be displayed in Picture in Picture mode? I believe they are not going to be needed since the developer can pass their custom controls.

I apologise if that's too early but since you stated "Later versions of this specification may allow PIP-ing arbitrary HTML content" you think that'd be probably worth thinking of?

mounirlamouri commented 6 years ago

I think that's a great point Zouhir. Even if we allow arbitrary HTML content, one possibility is that it wouldn't be interactive so custom controls would still make sense. Would you have a proposal for an API shape that could handle both video and arbitrary HTML content?

zouhir commented 6 years ago

I actually don't have a good proposal, but I will be finding time to think about that during the next couple days, don't want to hold any activity on this PR, if you decided to merge, I can open an issue early next week with an idea or a suggestion. Thanks @mounirlamouri .

beaufortfrancois commented 5 years ago

After discussing with @jernoble and @mounirlamouri at TPAC Lyon 2018 about Picture-in-Picture custom controls, we may want to go instead with some Media Session actions. Shall we close this PR and update the Media Session API spec if needed?

mounirlamouri commented 5 years ago

Yes, sgtm

zouhir commented 5 years ago

I don't disagree with that having our only use case for now is video playback in PiP. Since the spec document has implied intentions to support DOM fragment in the future in multiple areas I feel it's totally worths keeping that on the back of our heads while we progress with existing video only scenario and possibly revisit if we have a customer or use case who wanting custom actions. LGTM so far 👍