Skip to content

Conversation

mwyrzykowski
Copy link
Contributor

@mwyrzykowski mwyrzykowski commented Jul 6, 2023

d7f28b8

Resizing a fullscreen element doesn't always resize children to the correct size
https://bugs.webkit.org/show_bug.cgi?id=258847
<radar://110880116>

Reviewed by Tim Horton.

During a window resize, [_contentView bounds] will not be up to date during the precommit
handler so it is incorrect to intersect it with the web view's bounds.

Additionally it makes fullscreen resizing look much smoother.

* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView _updateVisibleContentRects]):
Skip intersection if we just resized the WKWebView.

* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/ios/WKWebViewResize.mm: Added.
(-[NSObject swizzled_didUpdateVisibleRect:unobscuredRect:contentInsets:unobscuredRectInScrollViewCoordinates:obscuredInsets:unobscuredSafeAreaInsets:inputViewBounds:scale:minimumScale:viewStability:enclosedInScrollableAncestorView:sendEvenIfUnchanged:]):
(setupWKContentViewSwizzle):
(operator==):
Required for EXPECT_EQ on CGRect.

(webViewHasExpectedBounds):
(TEST):
Add API test which ensures a content view with no other insets will have an
unobscuredRect which matches the enclosing WKWebView's bounds after setFrame: is called.

Tests both resizing the web view larger and smaller.

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

3e94b6d

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

@mwyrzykowski mwyrzykowski requested review from cdumez and rniwa as code owners July 6, 2023 21:09
@mwyrzykowski mwyrzykowski self-assigned this Jul 6, 2023
@mwyrzykowski mwyrzykowski added the Media Bugs related to the HTML 5 Media elements. label Jul 6, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 6, 2023
@mwyrzykowski mwyrzykowski removed the merging-blocked Applied to prevent a change from being merged label Jul 6, 2023
@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from a3fc609 to 9ee1cd1 Compare July 6, 2023 21:15
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 6, 2023
@mwyrzykowski mwyrzykowski removed the merging-blocked Applied to prevent a change from being merged label Jul 6, 2023
@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 9ee1cd1 to 64c7e94 Compare July 6, 2023 21:49
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 6, 2023
@mwyrzykowski mwyrzykowski removed the merging-blocked Applied to prevent a change from being merged label Jul 7, 2023
@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 64c7e94 to 47cbbb4 Compare July 7, 2023 00:03
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "VideoFullscreen" is used more commonly used than "FullscreenVideo", so maybe inFullscreenElementOrVideoFullscreen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll change that.

@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 47cbbb4 to 632a6a3 Compare July 7, 2023 22:52
@mwyrzykowski mwyrzykowski requested a review from mattwoodrow July 8, 2023 02:50
@mwyrzykowski mwyrzykowski requested review from anttijk and nt1m July 8, 2023 02:50
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems odd that stye resolves needs to know anything about fullscreen. Why doesn't window resize have the same issue? I think we should debug further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also fix this by calling scheduleFullStyleRebuild in a few places but it is a slightly larger change. Do you think that would be reasonable? I can try that instead.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, full style rebuilds were originally there because we had the UA styles matching the :-webkit-full-screen-ancestor pseudo class requiring all the ancestors of the fullscreen element to have their styles recomputed.

It is in theory no longer required with the new fullscreen API (for which I did remove full style rebuilds in some places but not all). The remaining ones avoid dirty renderer assertions when pushing to top layer (which we should arguably fix and then remove the full style rebuilds).

Adding more full style rebuilds isn't really the answer here. Fine grained PseudoClassChangeInvalidation should do its job here.

@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 632a6a3 to 55b4e24 Compare July 11, 2023 21:52
@mwyrzykowski mwyrzykowski requested a review from smfr July 11, 2023 21:53
Copy link
Contributor Author

@mwyrzykowski mwyrzykowski Jul 11, 2023

Choose a reason for hiding this comment

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

The changes in RenderVideo.h/cpp are to fix the same issue 265672@main fixed but for video playing in a fullscreen element.

They can be broken out into a separate patch if preferred, the issues are independent.

@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 55b4e24 to e6712a1 Compare July 11, 2023 22:15
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might not be sufficient. Layout is generally but not always occurring after a full style rebuild.

Maybe I want layoutContext().layout(); instead?

@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from e6712a1 to 099af89 Compare July 13, 2023 02:53
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 13, 2023

@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 099af89 to 3904a6b Compare July 13, 2023 16:48
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 13, 2023

@mwyrzykowski mwyrzykowski force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 3904a6b to 3e94b6d Compare July 24, 2023 05:14
@mwyrzykowski
Copy link
Contributor Author

@hortont424 @smfr I added a test covering this behavior, please let me know if you have any further feedback or would like any changes to be made.

I also cloned this issue to track if skipping the intersection is the correct thing to do in all cases when the window is resized. We were effectively already doing something similar when the window was resized smaller, since the content view bounds were still the larger size, so I don't think this change introduces an issue which was not previously present.

@hortont424
Copy link
Contributor

@hortont424 @smfr I added a test covering this behavior, please let me know if you have any further feedback or would like any changes to be made.

I also cloned this issue to track if skipping the intersection is the correct thing to do in all cases when the window is resized. We were effectively already doing something similar when the window was resized smaller, since the content view bounds were still the larger size, so I don't think this change introduces an issue which was not previously present.

This is a good point and makes me content enough that I'll r+

@mwyrzykowski mwyrzykowski added the merge-queue Applied to send a pull request to merge-queue label Jul 24, 2023
…orrect size

https://bugs.webkit.org/show_bug.cgi?id=258847
<radar://110880116>

Reviewed by Tim Horton.

During a window resize, [_contentView bounds] will not be up to date during the precommit
handler so it is incorrect to intersect it with the web view's bounds.

Additionally it makes fullscreen resizing look much smoother.

* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView _updateVisibleContentRects]):
Skip intersection if we just resized the WKWebView.

* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/ios/WKWebViewResize.mm: Added.
(-[NSObject swizzled_didUpdateVisibleRect:unobscuredRect:contentInsets:unobscuredRectInScrollViewCoordinates:obscuredInsets:unobscuredSafeAreaInsets:inputViewBounds:scale:minimumScale:viewStability:enclosedInScrollableAncestorView:sendEvenIfUnchanged:]):
(setupWKContentViewSwizzle):
(operator==):
Required for EXPECT_EQ on CGRect.

(webViewHasExpectedBounds):
(TEST):
Add API test which ensures a content view with no other insets will have an
unobscuredRect which matches the enclosing WKWebView's bounds after setFrame: is called.

Tests both resizing the web view larger and smaller.

Canonical link: https://commits.webkit.org/266257@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Resizing-a-fullscreen-element-doesnt-always-resize-children-to-the-correct-size branch from 3e94b6d to d7f28b8 Compare July 24, 2023 18:10
@webkit-commit-queue
Copy link
Collaborator

Committed 266257@main (d7f28b8): https://commits.webkit.org/266257@main

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

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

Development

Successfully merging this pull request may close these issues.

8 participants