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

Update explainer to include custom actions. #69

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
5 participants
@japacible
Copy link

japacible commented May 16, 2018

@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

This comment has been minimized.

Copy link
Collaborator

beaufortfrancois commented May 16, 2018

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

This comment has been minimized.

Copy link
Author

japacible commented May 23, 2018

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

This comment has been minimized.

Copy link
Collaborator

beaufortfrancois commented May 23, 2018

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.

@beaufortfrancois beaufortfrancois force-pushed the WICG:master branch from b1e2907 to cee81d2 May 24, 2018

japacible added some commits May 16, 2018

@japacible

This comment has been minimized.

Copy link
Author

japacible commented May 28, 2018

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

@@ -42,10 +45,12 @@ The proposed API is very similar to the Fullscreen API as they have similar prop
```
partial interface HTMLVideoElement {
Promise<PictureInPictureWindow> requestPictureInPicture();
Promise<void> setPictureInPictureControls(FrozenArray<CustomActionMetadata> action);

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois May 29, 2018

Collaborator

Nit: s/action/pipControls

This comment has been minimized.

Copy link
@japacible
// On the fullscreen API, they live on the Document.
attribute EventHandler onenterpictureinpicture;
attribute EventHandler onleavepictureinpicture;
attribute EventHandler onpictureinpicturecontrolsclick;

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois May 29, 2018

Collaborator

I'm not sure click is the right name but I don't have any thing better in mind.
WDYT @mounirlamouri?

This comment has been minimized.

Copy link
@japacible

japacible Jun 1, 2018

Author

I'm trying to find another API that has an EventHandler for a generic 'interaction' for some inspiration.

@mounirlamouri , thoughts?

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Jun 5, 2018

Collaborator

(gentle ping) @mounirlamouri

This comment has been minimized.

Copy link
@mounirlamouri

mounirlamouri Jun 8, 2018

Collaborator

As discussed offline with @beaufortfrancois the name seems good to me. We can always change it later if needed.

This comment has been minimized.

Copy link
@japacible

japacible Jun 8, 2018

Author

Thanks!

@@ -67,6 +72,18 @@ interface PictureInPictureWindow {
attribute EventHandler onresize;
};
interface CustomActionMetadata {

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois May 29, 2018

Collaborator

Nit: s/CustomActionMetadata/PictureInPictureControl/g

This comment has been minimized.

Copy link
@japacible
interface CustomActionMetadata {
attribute DOMString id;
attribute DOMString label; // Description of the action.
attribute FrozenArray<MediaImage> artwork;

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois May 29, 2018

Collaborator

Nit: s/artwork/icons

This comment has been minimized.

Copy link
@japacible
@@ -113,6 +144,13 @@ interface PictureInPictureWindow {
video.addEventListener('leavepictureinpicture', function() {
// Video element left Picture-In-Picture mode.
});
video.addEventListener('pictureinpicturecontrolsclick', event => {
switch (event.id) {

This comment has been minimized.

Copy link
@mounirlamouri

mounirlamouri Jun 8, 2018

Collaborator

If we add the id property to the event, we should probably define the interface for the event too.

This comment has been minimized.

Copy link
@japacible

japacible Jun 12, 2018

Author

Does this mean we'll have to add id to all Events? I'm looking at the Event interface and don't see a field we can similarly use.

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Jun 12, 2018

Collaborator

I think @mounirlamouri meant having a dedicated PictureInPictureControlsEventHandler which inherits from EventHandler and has a id property.

This comment has been minimized.

Copy link
@japacible

japacible Jun 12, 2018

Author

Updated with PictureInPictureControlsEventHandler. I can see the dedicated EventHandler behaviour be determined by the internal blink implementation; is that correct, or should there be something else here? Thanks!

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Jun 18, 2018

Collaborator

Update: s/onpictureinpicturecontrolsclick/onpictureinpicturecontrolclick/g

// On the fullscreen API, they live on the Document.
attribute EventHandler onenterpictureinpicture;
attribute EventHandler onleavepictureinpicture;
attribute PictureInPictureEventHandler onpictureinpicturecontrolsclick;

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Jun 13, 2018

Collaborator

Nit: Should this be PictureInPictureControlsEventHandler to make it clear it won't be the one for onenterpictureinpicture and onleavepictureinpicture?

This comment has been minimized.

Copy link
@japacible

japacible Jun 13, 2018

Author

Updated.

@beaufortfrancois

This comment has been minimized.

Copy link
Collaborator

beaufortfrancois commented Jun 14, 2018

@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': ...
  };
});
// On the fullscreen API, they live on the Document.
attribute EventHandler onenterpictureinpicture;
attribute EventHandler onleavepictureinpicture;
attribute PictureInPictureControlsEventHandler onpictureinpicturecontrolsclick;

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Jun 18, 2018

Collaborator

Update: s/onpictureinpicturecontrolsclick/onpictureinpicturecontrolclick/g

DOMString type = "";
};
interface PictureInPictureControlsEventHandler : EventHandler {

This comment has been minimized.

Copy link
@beaufortfrancois

beaufortfrancois Jun 25, 2018

Collaborator

Please add [Constructor(DOMString type, PictureInPictureControlEventInit eventInitDict)] and

dictionary PictureInPictureControlEventInit : EventInit {
    required DOMString id;
};
@jernoble

This comment has been minimized.

Copy link

jernoble commented Jun 25, 2018

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

@beaufortfrancois

This comment has been minimized.

Copy link
Collaborator

beaufortfrancois commented Jun 28, 2018

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.

  • Media Session API controls are for notifications, media center, etc.
  • Picture-in-Picture API controls are specifically for the overlay window.

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

This comment has been minimized.

Copy link
Collaborator

beaufortfrancois commented Jul 16, 2018

@jernoble (gentle ping)

@jernoble

This comment has been minimized.

Copy link

jernoble commented Jul 16, 2018

@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

This comment has been minimized.

Copy link

zouhir commented Jul 26, 2018

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

This comment has been minimized.

Copy link
Collaborator

mounirlamouri commented Jul 26, 2018

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

This comment has been minimized.

Copy link

zouhir commented Jul 27, 2018

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

This comment has been minimized.

Copy link
Collaborator

beaufortfrancois commented Oct 24, 2018

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

This comment has been minimized.

Copy link
Collaborator

mounirlamouri commented Oct 24, 2018

Yes, sgtm

@zouhir

This comment has been minimized.

Copy link

zouhir commented Oct 24, 2018

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 👍

aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 18, 2019

[Picture-in-Picture] Remove experimental custom controls.
It was decided to use the Media Session API to add controls to the
Picture-in-Picture window. This CL removes all "Custom Controls" code
that was added as an experimental feature.

WICG/picture-in-picture#69

Bug: 926174
Change-Id: If7816bc5aadf2535ad42873f400dcc3bcbcccffb
Reviewed-on: https://chromium-review.googlesource.com/c/1472292
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Auto-Submit: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#633051}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.