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

bbc.co.uk: Video goes black with only audio playing when exiting PiP mode #14737

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

jyavenard
Copy link
Member

@jyavenard jyavenard commented Jun 7, 2023

ba39716

bbc.co.uk: Video goes black with only audio playing when exiting PiP mode
https://bugs.webkit.org/show_bug.cgi?id=257711
rdar://108304377

Reviewed by Jer Noble.

Only fire the fullscreen change event once we did enter fullscreen.
This gives time for the UI process to complete exiting PiP in the UI process.

The process for exiting PiP and go back into fullscreen is a complicated
(and unnecessary) dance between the content and the UI process.
didStopPictureInPicture (UI) -> requestRestoreFullScreen (CP) -> EnterFullScreen (UI)
 -> WillEnterFullscreen (CP) -> beganEnterFullScreen (UI) -> didEnterFullScreen (UI)
 -> didEnterFullScreen (CP)

Previously, the events `fullscreenchange` was fired in WillEnterFullscreen.
if a JS event listener was set, and attempted to exit PiP then (as BBC website is doing)
it would have left VideoFullscreenInterfaceAVKit in a broken state
once it received `beganEnterFullScreen` as it's not an handled chained of event.

By letting the UI process complete the exit of PiP back to fullscreen,
we can avoid the problem from occurring alltogether, and this is simply
done by firing the events and resolving the promise in didEnterFullScreen.

We limit this behaviour to BBC.com

This entire code is in serious need of a rewrite.

Manually tested under all possible interface scenarios:
- Entering PiP, Exiting PiP using BBC main player control.
- Entering PiP, Exiting PiP using PiP controller button.
- Entering fullscreen via BBC control, entering PiP using PiP button, exiting PiP using BBC main player control
- As above but using PiP controller button as last step.
- Going into Auto-Pip by swiping home and exiting PiP.
- Entering PiP, quitting PiP (X button)

* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::FullscreenManager::willEnterFullscreen):
(WebCore::FullscreenManager::didEnterFullscreen):
* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::shouldDelayFullscreenEventWhenExitingPictureInPictureQuirk const):
* Source/WebCore/page/Quirks.h:

Canonical link: https://commits.webkit.org/264974@main

93cdf18

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2   πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@jyavenard jyavenard self-assigned this Jun 7, 2023
@jyavenard jyavenard added the Media Bugs related to the HTML 5 Media elements. label Jun 7, 2023
Copy link
Contributor

@jernoble jernoble left a comment

Choose a reason for hiding this comment

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

The problem with this change is it means the fullscreenchange event won't fire until after the transition into fullscreen has started. For sites that use that event to re-layout into a fullscreen mode, it will mean that transition will have flashes of the original layout visible to the user.

Perhaps we can limit this change to only the BBC with a quirk?

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 7, 2023
@jyavenard jyavenard removed the merging-blocked Applied to prevent a change from being merged label Jun 7, 2023
@jyavenard jyavenard added the merge-queue Applied to send a pull request to merge-queue label Jun 8, 2023
…mode

https://bugs.webkit.org/show_bug.cgi?id=257711
rdar://108304377

Reviewed by Jer Noble.

Only fire the fullscreen change event once we did enter fullscreen.
This gives time for the UI process to complete exiting PiP in the UI process.

The process for exiting PiP and go back into fullscreen is a complicated
(and unnecessary) dance between the content and the UI process.
didStopPictureInPicture (UI) -> requestRestoreFullScreen (CP) -> EnterFullScreen (UI)
 -> WillEnterFullscreen (CP) -> beganEnterFullScreen (UI) -> didEnterFullScreen (UI)
 -> didEnterFullScreen (CP)

Previously, the events `fullscreenchange` was fired in WillEnterFullscreen.
if a JS event listener was set, and attempted to exit PiP then (as BBC website is doing)
it would have left VideoFullscreenInterfaceAVKit in a broken state
once it received `beganEnterFullScreen` as it's not an handled chained of event.

By letting the UI process complete the exit of PiP back to fullscreen,
we can avoid the problem from occurring alltogether, and this is simply
done by firing the events and resolving the promise in didEnterFullScreen.

We limit this behaviour to BBC.com

This entire code is in serious need of a rewrite.

Manually tested under all possible interface scenarios:
- Entering PiP, Exiting PiP using BBC main player control.
- Entering PiP, Exiting PiP using PiP controller button.
- Entering fullscreen via BBC control, entering PiP using PiP button, exiting PiP using BBC main player control
- As above but using PiP controller button as last step.
- Going into Auto-Pip by swiping home and exiting PiP.
- Entering PiP, quitting PiP (X button)

* Source/WebCore/dom/FullscreenManager.cpp:
(WebCore::FullscreenManager::willEnterFullscreen):
(WebCore::FullscreenManager::didEnterFullscreen):
* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::shouldDelayFullscreenEventWhenExitingPictureInPictureQuirk const):
* Source/WebCore/page/Quirks.h:

Canonical link: https://commits.webkit.org/264974@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264974@main (ba39716): https://commits.webkit.org/264974@main

Reviewed commits have been landed. Closing PR #14737 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit ba39716 into WebKit:main Jun 8, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
5 participants