fix: Fix exiting fullscreen on Safari #5439
Conversation
screen.orientation.unlock is not defined on Safari, even though screen.orientation is. This change checks for the presence of that function before calling it. Fixes shaka-project#5437
theodab
changed the title
|
Incremental code coverage: 31.25% |
|
Hey @theodab With this should we even extend and patch the existing Orientation Polyfill with reference from https://github.com/chmanie/o9n#supported-browsers? |
I guess updating the polyfill would be nicer, you're right. It looks like that polyfill just falls through to a no-op for Safari, so I'm not sure if it will be very relevant here as a reference though. |
|
Yes it's a noop for Safari, Reason for patching the polyfill as on custom apps the developers even noticing that an Orientation Polyfill exists but still it fails to handle it on Safari. |
| static installBasedOnScreenMethods_() { | ||
| if (screen.orientation.lock === undefined) { | ||
| screen.orientation.lock = (orientation) => { | ||
| shaka.log.info('screen.orientation.lock is a no-op'); |
There was a problem hiding this comment.
What happens when this no-op is used? How does the app behavior differ from the normal case?
There was a problem hiding this comment.
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.
| } | ||
| if (screen.orientation.unlock === undefined) { | ||
| screen.orientation.unlock = () => { | ||
| shaka.log.info('screen.orientation.unlock is a no-op'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I had the impression from this PR and the comments on it that only unlock() was missing on Safari. Thanks for clarifying.
`screen.orientation.unlock` is not defined on Safari, even though `screen.orientation` is. This changes our orientation polyfill to add that method to `screen.orientation`. Fixes #5437
`screen.orientation.unlock` is not defined on Safari, even though `screen.orientation` is. This changes our orientation polyfill to add that method to `screen.orientation`. Fixes #5437
screen.orientation.unlockis not defined on Safari, even thoughscreen.orientationis.This changes our orientation polyfill to add that method to
screen.orientation.Fixes #5437