Summary: | Picture-in-Picture events are not fired if we switch the Picture-in-Picture mode through modern media controls | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||
Component: | Media | Assignee: | Peng Liu <peng.liu6> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, ews-watchlist, glenn, jer.noble, joepeck, philipj, sergio, tsavell, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Peng Liu
2019-11-06 17:23:24 PST
Created attachment 382990 [details]
Patch
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? 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. In Bug 203989, we will let video.webkitSetPresentationMode() fire the events like the interface defined in the picture-in-picture API spec. Comment on attachment 382990 [details] Patch Clearing flags on attachment: 382990 Committed r252240: <https://trac.webkit.org/changeset/252240> All reviewed patches have been landed. Closing bug. 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? Got it, will look into them now. Thanks! Reopening to attach new patch. Created attachment 383302 [details]
Patch
Comment on attachment 383302 [details]
Patch
This patch can fix the timeout failure of media/modern-media-controls/pip-support/pip-support-click.html.
(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. Comment on attachment 383302 [details] Patch Clearing flags on attachment: 383302 Committed r252365: <https://trac.webkit.org/changeset/252365> All reviewed patches have been landed. Closing bug. |