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

Prevent hidden documents from locking the screen orientation #6283

Merged
merged 1 commit into from Nov 25, 2022
Merged

Prevent hidden documents from locking the screen orientation #6283

merged 1 commit into from Nov 25, 2022

Conversation

marcoscaceres
Copy link
Contributor

@marcoscaceres marcoscaceres commented Nov 9, 2022

a68ef76

Prevent hidden documents from locking the screen orientation
https://bugs.webkit.org/show_bug.cgi?id=247248
rdar://102019707

Reviewed by Youenn Fablet.

* LayoutTests/fast/screen-orientation/hidden-document-check-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/resources/testdriver-vendor.js:
(window.test_driver_internal.action_sequence):
(async if):
(window.test_driver_internal.minimize_window):
(window.test_driver_internal.set_window_rect):
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/hidden_document-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/hidden_document.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/page/ScreenOrientation.cpp:
(WebCore::ScreenOrientation::lock):

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

c7728e5

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

@marcoscaceres marcoscaceres self-assigned this Nov 9, 2022
@marcoscaceres marcoscaceres added Other WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). labels Nov 9, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 9, 2022
@marcoscaceres marcoscaceres removed the merging-blocked Applied to prevent a change from being merged label Nov 9, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 10, 2022
@marcoscaceres
Copy link
Contributor Author

That mac-wk1 failure is a bit odd:

PASS SecurityError rejected promise  with InvalidStateError: No browsing context.

@hortont424, would the lack of a browsing context be a wk1 thing (even thought the script is running in page)? Any suggestions?

@marcoscaceres marcoscaceres removed the merging-blocked Applied to prevent a change from being merged label Nov 14, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 14, 2022
Source/WebCore/page/ScreenOrientation.cpp Outdated Show resolved Hide resolved
promise->reject(Exception { SecurityError, "Only visible documents can lock the screen orientation"_s });
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this patch, but we could try to use DOMPromiseDeferred::settle in manager->lock().
Maybe by adding settle(std::optional) to it?

@@ -6041,3 +6041,7 @@ http/tests/loading/oauth.html [ DumpJSConsoleLogInStdErr ]
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html [ DumpJSConsoleLogInStdErr ]
imported/w3c/web-platform-tests/clipboard-apis/feature-policy/clipboard-write/clipboard-write-disabled-by-feature-policy.tentative.https.sub.html [ DumpJSConsoleLogInStdErr ]
imported/w3c/web-platform-tests/html/infrastructure/urls/terminology-0/document-base-url-initiated-grand-parent.https.window.html [ DumpJSConsoleLogInStdErr ]

# Screen orientation
## We don't implment test_driver.minimize_window()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is window.internals.setPageVisibility good enough to implement test_driver.minimize_window?
If so, could we try implementing it this way?

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 had a look. We might be able to use the same semantics ((ab)using page visibility), but I don't think we can use window.internal, as it needs to be done through TestDriver.

Is my understanding correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update LayoutTests/imported/w3c/web-platform-tests/resources/testdriver-vendor.js to use window.internals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed we can, I recently discovered. πŸŽ‰ I’ll patch something up.

@xeenon xeenon removed the Other label Nov 21, 2022
@marcoscaceres marcoscaceres removed the merging-blocked Applied to prevent a change from being merged label Nov 24, 2022
@marcoscaceres marcoscaceres marked this pull request as ready for review November 24, 2022 05:24
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 262d5ec. Live statuses available at the PR page, #6283

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 24, 2022
@marcoscaceres marcoscaceres removed the merging-blocked Applied to prevent a change from being merged label Nov 24, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 24, 2022
@marcoscaceres marcoscaceres removed the merging-blocked Applied to prevent a change from being merged label Nov 24, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 25, 2022
@marcoscaceres marcoscaceres added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Nov 25, 2022
https://bugs.webkit.org/show_bug.cgi?id=247248
rdar://102019707

Reviewed by Youenn Fablet.

* LayoutTests/fast/screen-orientation/hidden-document-check-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/resources/testdriver-vendor.js:
(window.test_driver_internal.action_sequence):
(async if):
(window.test_driver_internal.minimize_window):
(window.test_driver_internal.set_window_rect):
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/hidden_document-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/screen-orientation/hidden_document.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/page/ScreenOrientation.cpp:
(WebCore::ScreenOrientation::lock):

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

Committed 257019@main (a68ef76): https://commits.webkit.org/257019@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 25, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit a68ef76 into WebKit:main Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
7 participants