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.

Sign up for GitHub

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

fix: Fix exiting fullscreen on Safari #5439

Merged
merged 2 commits into from
Jul 26, 2023
Merged

fix: Fix exiting fullscreen on Safari #5439

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8dc029
fix: Fix screen unlocking on Safari
theodab Jul 25, 2023
cad4ac5
Changed this to be a polyfill change.
theodab Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 34 additions & 7 deletions lib/polyfill/orientation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

goog.provide('shaka.polyfill.Orientation');

goog.require('shaka.log');
goog.require('shaka.util.FakeEvent');
goog.require('shaka.util.FakeEventTarget');
goog.require('shaka.polyfill');
Expand All @@ -23,21 +24,47 @@ shaka.polyfill.Orientation = class {
* @export
*/
static install() {
if (screen.orientation) {
if (screen.orientation && screen.orientation.unlock) {
// Not needed.
shaka.log.info('Using native screen.orientation');
return;
}

// There is no way to check to see if the 'orientationchange' event exists
// on window, which could theoretically lead to this making a
// screen.orientation object that doesn't actually work.
// However, it looks like all platforms that support the deprecated
// window.orientation feature also support that event.
if (window.orientation != undefined) {
if (screen.orientation != undefined) {
// There are some platforms where screen.orientation is defined but
// incomplete (e.g. Safari).
// Install a very simple polyfill in that case.
shaka.polyfill.Orientation.installBasedOnScreenMethods_();
} else if (window.orientation != undefined) {
// There is no way to check to see if the 'orientationchange' event exists
// on window, which could theoretically lead to this making a
// screen.orientation object that doesn't actually work.
// However, it looks like all platforms that support the deprecated
// window.orientation feature also support that event.
shaka.polyfill.Orientation.installBasedOnWindowMethods_();
}
}

/**
* Modifies screen.orientation to add no-ops for missing methods.
* Meant for cases where screen.orientation is defined, but has missing
* methods that cannot be properly polyfilled.
* @private
*/
static installBasedOnScreenMethods_() {
if (screen.orientation.lock === undefined) {
screen.orientation.lock = (orientation) => {
shaka.log.info('screen.orientation.lock is a no-op');
Copy link
Member

Choose a reason for hiding this comment

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

What happens when this no-op is used? How does the app behavior differ from the normal case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this no-op doesn't change anything because we already wrap the lock method in a try-catch to silence errors. I just included it for sake of completeness.

return Promise.resolve();
};
}
if (screen.orientation.unlock === undefined) {
screen.orientation.unlock = () => {
shaka.log.info('screen.orientation.unlock is a no-op');
Copy link
Member

Choose a reason for hiding this comment

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

Same question here.

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 one improves things because, on desktop Safari, neither lock nor unlock are implemented. So we don't need to unlock the screen, because we never locked it in the first place; we just need to prevent the method call from erroring, so that we can move on to the part of the code that calls document.exitFullscreen.
I guess technically this approach, where each methods is filled with a no-op independently, would lead to a weird result if the browser has lock implemented but doesn't have unlock implemented. But I would hope that no browser provides an API for locking the screen without providing an API for unlocking it, that sounds like an accident waiting to happen.

Copy link
Member

Choose a reason for hiding this comment

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

I had the impression from this PR and the comments on it that only unlock() was missing on Safari. Thanks for clarifying.

};
}
}

/**
* Makes a polyfill for orientation, based on window methods.
* Note that some of the features this is based on are deprecated, so this
Expand Down