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

@japacible
Copy link

@japacible 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
Copy link
Collaborator

@beaufortfrancois 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
Copy link
Author

@japacible 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
Copy link
Collaborator

@beaufortfrancois 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.

@japacible
Copy link
Author

@japacible japacible commented May 28, 2018

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

explainer.md Outdated
@@ -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);
Copy link
Collaborator

@beaufortfrancois beaufortfrancois May 29, 2018

Choose a reason for hiding this comment

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

Nit: s/action/pipControls

Copy link
Author

@japacible japacible May 29, 2018

Choose a reason for hiding this comment

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

Done.

explainer.md Outdated
// On the fullscreen API, they live on the Document.
attribute EventHandler onenterpictureinpicture;
attribute EventHandler onleavepictureinpicture;
attribute EventHandler onpictureinpicturecontrolsclick;
Copy link
Collaborator

@beaufortfrancois beaufortfrancois May 29, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

@japacible japacible Jun 1, 2018

Choose a reason for hiding this comment

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

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

@mounirlamouri , thoughts?

Copy link
Collaborator

@beaufortfrancois beaufortfrancois Jun 5, 2018

Choose a reason for hiding this comment

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

(gentle ping) @mounirlamouri

Copy link
Member

@mounirlamouri mounirlamouri Jun 8, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

@japacible japacible Jun 8, 2018

Choose a reason for hiding this comment

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

Thanks!

explainer.md Outdated
@@ -67,6 +72,18 @@ interface PictureInPictureWindow {
attribute EventHandler onresize;
};
interface CustomActionMetadata {
Copy link
Collaborator

@beaufortfrancois beaufortfrancois May 29, 2018

Choose a reason for hiding this comment

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

Nit: s/CustomActionMetadata/PictureInPictureControl/g

Copy link
Author

@japacible japacible May 29, 2018

Choose a reason for hiding this comment

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

Done.

explainer.md Outdated
interface CustomActionMetadata {
attribute DOMString id;
attribute DOMString label; // Description of the action.
attribute FrozenArray<MediaImage> artwork;
Copy link
Collaborator

@beaufortfrancois beaufortfrancois May 29, 2018

Choose a reason for hiding this comment

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

Nit: s/artwork/icons

Copy link
Author

@japacible japacible May 29, 2018

Choose a reason for hiding this comment

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

Done.

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

@mounirlamouri mounirlamouri Jun 8, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

@japacible japacible Jun 12, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@beaufortfrancois beaufortfrancois Jun 12, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

@japacible japacible Jun 12, 2018

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

@beaufortfrancois beaufortfrancois Jun 18, 2018

Choose a reason for hiding this comment

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

Update: s/onpictureinpicturecontrolsclick/onpictureinpicturecontrolclick/g

explainer.md Outdated
// On the fullscreen API, they live on the Document.
attribute EventHandler onenterpictureinpicture;
attribute EventHandler onleavepictureinpicture;
attribute PictureInPictureEventHandler onpictureinpicturecontrolsclick;
Copy link
Collaborator

@beaufortfrancois beaufortfrancois Jun 13, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

@japacible japacible Jun 13, 2018

Choose a reason for hiding this comment

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

Updated.

@beaufortfrancois
Copy link
Collaborator

@beaufortfrancois 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;
Copy link
Collaborator

@beaufortfrancois beaufortfrancois Jun 18, 2018

Choose a reason for hiding this comment

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

Update: s/onpictureinpicturecontrolsclick/onpictureinpicturecontrolclick/g

DOMString type = "";
};
interface PictureInPictureControlsEventHandler : EventHandler {
Copy link
Collaborator

@beaufortfrancois beaufortfrancois Jun 25, 2018

Choose a reason for hiding this comment

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

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

dictionary PictureInPictureControlEventInit : EventInit {
    required DOMString id;
};

@jernoble
Copy link

@jernoble 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
Copy link
Collaborator

@beaufortfrancois 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
Copy link
Collaborator

@beaufortfrancois beaufortfrancois commented Jul 16, 2018

@jernoble (gentle ping)

@jernoble
Copy link

@jernoble 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
Copy link

@zouhir 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
Copy link
Member

@mounirlamouri 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
Copy link

@zouhir 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
Copy link
Collaborator

@beaufortfrancois 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
Copy link
Member

@mounirlamouri mounirlamouri commented Oct 24, 2018

Yes, sgtm

@zouhir
Copy link

@zouhir 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 issue Feb 18, 2019
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.

w3c/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}
SergioOpenPeer pushed a commit to webrtc-uwp/chromium-testing that referenced this issue May 14, 2019
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.

w3c/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-Original-Commit-Position: refs/heads/master@{#633051}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: f333c2fb554cfcf60aaa7b1a2c6e76bd25a4ce5f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants