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

WebKit Media: Toggling fullscreen fails to work #16336

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

rr-codes
Copy link
Contributor

@rr-codes rr-codes commented Aug 3, 2023

43da1fd

WebKit Media: Toggling fullscreen fails to work
https://bugs.webkit.org/show_bug.cgi?id=259760
rdar://111534888

Reviewed by Aditya Keerthi.

The window resize handler is created when receiving the `will{Enter|Exit}FullScreen` notification,
and is then invalidated as a result of `setFrameSize` being called. However, when transiting to/from
element full screen, `setFrameSize` is called before the notification is posted, instead of after.
This causes the handler to never be invalidated.

Fix by creating the handler directly inside `setFrameSize`, and not rely on the full screen
notifications at all. This is ok since `_holdResizeSnapshotWithReason` only returns a non-nil block
when entering or exiting full screen, so it has no effect in other cases.

No new tests, since all tests in `FullscreenVideoTextRecognition` already test this behavior in some
configurations.

* Source/WebKit/UIProcess/API/mac/WKView.mm:
(-[WKView _web_windowWillEnterFullScreen]): Deleted.
(-[WKView _web_windowWillExitFullScreen]): Deleted.
* Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:
(-[WKWebView _holdWindowResizeSnapshotWithReason:]):
(-[WKWebView setFrameSize:]):
(-[WKWebView _web_windowWillEnterFullScreen]): Deleted.
(-[WKWebView _web_windowWillExitFullScreen]): Deleted.
* Source/WebKit/UIProcess/mac/WebViewImpl.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(-[WKWindowVisibilityObserver startObserving:]):
(-[WKWindowVisibilityObserver stopObserving:]):
(-[WKWindowVisibilityObserver _windowWillEnterFullScreen:]): Deleted.
(-[WKWindowVisibilityObserver _windowWillExitFullScreen:]): Deleted.
(WebKit::WebViewImpl::windowWillEnterFullScreen): Deleted.
(WebKit::WebViewImpl::windowWillExitFullScreen): Deleted.

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

f680e43

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

@rr-codes rr-codes requested a review from cdumez as a code owner August 3, 2023 02:24
@rr-codes rr-codes self-assigned this Aug 3, 2023
@rr-codes rr-codes added the Media Bugs related to the HTML 5 Media elements. label Aug 3, 2023
pxlcoder
pxlcoder previously approved these changes Aug 3, 2023
Copy link
Member

@pxlcoder pxlcoder left a comment

Choose a reason for hiding this comment

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

I'm assuming no tests since we have existing tests running on a different configuration, but it would be good if you could try to find a test that was failing locally prior to this change.

Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm Outdated Show resolved Hide resolved
@pxlcoder
Copy link
Member

pxlcoder commented Aug 3, 2023

PR description could also be a bit more detailed, you gave me a nice explanation elsewhere πŸ™‚

@pxlcoder
Copy link
Member

pxlcoder commented Aug 3, 2023

Dismissing review, as there’s a case this doesn’t fix:

When someone tries to enter app fullscreen, but does not resize the web view.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 3, 2023
@rr-codes rr-codes removed the merging-blocked Applied to prevent a change from being merged label Aug 3, 2023
@rr-codes rr-codes added the merge-queue Applied to send a pull request to merge-queue label Aug 3, 2023
@webkit-commit-queue webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Aug 3, 2023
@rr-codes rr-codes removed the merging-blocked Applied to prevent a change from being merged label Aug 3, 2023
@rr-codes rr-codes added the merge-queue Applied to send a pull request to merge-queue label Aug 3, 2023
https://bugs.webkit.org/show_bug.cgi?id=259760
rdar://111534888

Reviewed by Aditya Keerthi.

The window resize handler is created when receiving the `will{Enter|Exit}FullScreen` notification,
and is then invalidated as a result of `setFrameSize` being called. However, when transiting to/from
element full screen, `setFrameSize` is called before the notification is posted, instead of after.
This causes the handler to never be invalidated.

Fix by creating the handler directly inside `setFrameSize`, and not rely on the full screen
notifications at all. This is ok since `_holdResizeSnapshotWithReason` only returns a non-nil block
when entering or exiting full screen, so it has no effect in other cases.

No new tests, since all tests in `FullscreenVideoTextRecognition` already test this behavior in some
configurations.

* Source/WebKit/UIProcess/API/mac/WKView.mm:
(-[WKView _web_windowWillEnterFullScreen]): Deleted.
(-[WKView _web_windowWillExitFullScreen]): Deleted.
* Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:
(-[WKWebView _holdWindowResizeSnapshotWithReason:]):
(-[WKWebView setFrameSize:]):
(-[WKWebView _web_windowWillEnterFullScreen]): Deleted.
(-[WKWebView _web_windowWillExitFullScreen]): Deleted.
* Source/WebKit/UIProcess/mac/WebViewImpl.h:
* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(-[WKWindowVisibilityObserver startObserving:]):
(-[WKWindowVisibilityObserver stopObserving:]):
(-[WKWindowVisibilityObserver _windowWillEnterFullScreen:]): Deleted.
(-[WKWindowVisibilityObserver _windowWillExitFullScreen:]): Deleted.
(WebKit::WebViewImpl::windowWillEnterFullScreen): Deleted.
(WebKit::WebViewImpl::windowWillExitFullScreen): Deleted.

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

Committed 266558@main (43da1fd): https://commits.webkit.org/266558@main

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

@webkit-commit-queue webkit-commit-queue merged commit 43da1fd into WebKit:main Aug 3, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 3, 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