Bug 203933 – Picture-in-Picture events are not fired if we switch the Picture-in-Picture mode through modern media controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Picture-in-Picture events are not fired when enter/exit the Picture-in-Picture mode through modern media controls

[Radar WebKit Bug Importer]

<rdar://problem/56966334>

[Peng Liu]

Created attachment 382990 [details]
Patch

[Eric Carlson]

Comment on attachment 382990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382990&action=review

> Source/WebCore/Modules/modern-media-controls/media/pip-support.js:55
> -        media.webkitSetPresentationMode(media.webkitPresentationMode === PiPMode ? InlineMode : PiPMode);
> +        if (!document.pictureInPictureElement)
> +            media.requestPictureInPicture();
> +        else
> +            document.exitPictureInPicture();

Is this required, or is the change just modernize the controls code? 

If it is required, does that mean that existing code that uses video.webkitSetPresentationMode (e.g. all WebKit-specific PiP code) won't behave correctly?

[Peng Liu]

Comment on attachment 382990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382990&action=review

>> Source/WebCore/Modules/modern-media-controls/media/pip-support.js:55
>> +            document.exitPictureInPicture();
> 
> Is this required, or is the change just modernize the controls code? 
> 
> If it is required, does that mean that existing code that uses video.webkitSetPresentationMode (e.g. all WebKit-specific PiP code) won't behave correctly?

It is required. The video.webkitSetPresentationMode() will continue working, but if we use it, the video element won't fire the events defined in the picture-in-picture API spec (enterpictureinpicture, leavepictureinpicture) properly.

[Peng Liu]

In Bug 203989, we will let video.webkitSetPresentationMode() fire the events like the interface defined in the picture-in-picture API spec.

[WebKit Commit Bot]

Comment on attachment 382990 [details]
Patch

Clearing flags on attachment: 382990

Committed r252240: <https://trac.webkit.org/changeset/252240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the changes in: https://trac.webkit.org/changeset/252240/webkit

caused two tests to timeout:
media/modern-media-controls/media-controller/media-controller-inline-to-fullscreen-to-pip-to-inline.html 
media/modern-media-controls/pip-support/pip-support-click.html

history:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=media%2Fmodern-media-controls%2Fmedia-controller%2Fmedia-controller-inline-to-fullscreen-to-pip-to-inline.html&test=media%2Fmodern-media-controls%2Fpip-support%2Fpip-support-click.html


Can this be looked at today?

[Peng Liu]

Got it, will look into them now. Thanks!

[Peng Liu]

Reopening to attach new patch.

[Peng Liu]

Created attachment 383302 [details]
Patch

[Peng Liu]

Comment on attachment 383302 [details]
Patch

This patch can fix the timeout failure of media/modern-media-controls/pip-support/pip-support-click.html.

[Peng Liu]

(In reply to Truitt Savell from comment #8)
> It looks like the changes in: https://trac.webkit.org/changeset/252240/webkit
> 
> caused two tests to timeout:
> media/modern-media-controls/media-controller/media-controller-inline-to-
> fullscreen-to-pip-to-inline.html 
For this layout test, can we disable it for mac-wk1 for now? It is related to the test runner updates that need to be done in bug 203723. Also, I need to look into bug 183490 to see the related media control bugs.

[WebKit Commit Bot]

Comment on attachment 383302 [details]
Patch

Clearing flags on attachment: 383302

Committed r252365: <https://trac.webkit.org/changeset/252365>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.