-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[expo-av][web] Fix Video Fullscreen APIs for Safari #12258
Conversation
fe9d5d0
to
3fef87b
Compare
@bbarthec @IjzerenHein I believe this PR is ready, the failing check seems to be related to a problem with github's issue API that's beyond me... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! This looks great! I verified locally and can confirm it works on all browsers now.
A couple things still need attention:
- Please remove any changes from the the root CHANGELOG.md and put them in
expo-av/CHANGELOG.md
- Please rename the file
WebFullscreenUtils.ts
->FullscreenUtils.web.ts
Awesome work! 👍
277bf00
to
f12782c
Compare
|
@IjzerenHein Thanks for the feedback! All has been addressed. I think the blocked merge is due to the same GitHub action configuration issue that I experienced earlier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the updates Eliot! There is one small thing, the changelog entry was added to an existing version but should be added to Unpublished
.
f12782c
to
695e5d2
Compare
**Problem** The Video.onFullscreenUpdate,videoRef.presentFullscreenPlayer, and videoRef.dismissFullscreenPlayer APIs are broken in Safari on both iOS and macOS. The videoRef.presentFullscreenPlayer API throws the error Unhandled Rejection (TypeError): element.requestFullscreen is not a function. (In 'element.requestFullscreen()', 'element.requestFullscreen' is undefined). The other APIs are difficult to test without that API working, but after a little experimentation I was able to determine that they also are broken (onFullscreenUpdate is never called even when a video is switched to fullscreen and dismissFullscreenPlayer throws an error similar to presentFullscreenPlayer). **Solution** I extracted the code that interacts with the browser's fullscreen APIs into a separate FullscreenUtils.web.ts module. I added some feature-detection code to these APIs to switch between the standard fullscreen API (the previous solution) and the non-standard webkit fullscreen API (my addition to fix fullscreen usage in Safari). **Testing** For each of the browsers listed below I manually: 1. Loaded the Video component page in the bare-expo test app. 2. Confirmed that clicking "Open full screen" switched the video into fullscreen and logged `onFullscreenUpdate - {fullscreenUpdate: 1}` to the console. 3. Confirmed that clicking the native close fullscreen control switched the video out of fullscreen and logged `onFullscreenUpdate - {fullscreenUpdate: 3}` to the console. Browsers: * Chrome 89.0.4385.0 on Android 11 (emulated) * Chrome 89.0.4389.90 on macOS * Firefox 86.0.1 on macOS * Safari 14.0.3 on macOS * Safari on iOS 14.4 (simulator + iPhone SE) For Safari (iOS and macOS) I manually exectued the webkitExitFullScreen API from console to confirm that it works as expected (there's no control in the test app to trigger that API). For IE11 I just manually tested the DOM APIs/events that I selected. I couldn't find an easy way to test this PR specifically in IE11 because Expo doesn't work in IE11 by default. I was tempted to just drop the IE11 fullscreen code I came across because it seemed to be broken anyway, but Chesterton's fence prevented me. I did not write unit tests for the new WebFullscreenUtils as that would basically mean just testing the internal implementation details of the utilities which is not very helpful. I could not figure out how to get the `Video` e2e tests running for the web application - I got strange errors when I tried that. As this is a cross browser issue I'm not really sure running tests in puppeteer would tell us much though. Ultimately something like BrowserStack is what's needed to test this change, but that's beyond the scope of this PR. fixes expo#12257
695e5d2
to
a567043
Compare
This PR will break the |
|
Problem
The
Video.onFullscreenUpdate
,videoRef.presentFullscreenPlayer
, andvideoRef.dismissFullscreenPlayer
APIs are broken in Safari on both iOS and macOS. The
videoRef.presentFullscreenPlayer
API throws theerror
Unhandled Rejection (TypeError): element.requestFullscreen is not a function. (In 'element.requestFullscreen()', 'element.requestFullscreen' is undefined)
. The other APIs are difficultto test without that API working, but after a little experimentation I was able to determine that they
also are broken (
onFullscreenUpdate
is never called even when a video is switched to fullscreen with the working native control anddismissFullscreenPlayer
throws an error similar topresentFullscreenPlayer
).Solution
I extracted the code that interacts with the browser's fullscreen APIs into a separate
WebFullscreenUtils.ts module (named similarly to other web-only utility modules like WebCameraUtils.ts and
WebCapabilityUtils.ts). I added some feature-detection code to these APIs to switch between the standard
fullscreen API (the previous solution) and the non-standard webkit fullscreen API (my addition to fix
fullscreen usage in Safari).
Testing
For each of the browsers listed below I manually:
and logged
onFullscreenUpdate - {fullscreenUpdate: 1}
to the console.video out of fullscreen and logged
onFullscreenUpdate - {fullscreenUpdate: 3}
to the console.Browsers:
For Safari (iOS and macOS) I manually executed the webkitExitFullScreen API from
console to confirm that it works as expected (there's no control in the test
app to trigger that API).
For IE11 I just used the dev console to manually test that the underlying DOM APIs/events that I used work as expected. I couldn't find an easy way to test my actual PR in IE11 because Expo doesn't work in that browser by default. I was
tempted to just drop the IE11 fullscreen code I came across entirely because a) Expo doesn't support IE11 out of the box and b) that IE11 code seemed to be broken anyway, but Chesterton's fence prevented me.
I did not write unit tests for the new WebFullscreenUtils module as that would
basically mean just testing the internal implementation details of the utilities
which is not very helpful.
I could not figure out how to get the
Video
e2e tests running for the webapplication - I got strange errors when I tried that. As this is a cross browser
issue I'm not really sure running tests in puppeteer would tell us much anyway.
Ultimately something like BrowserStack is what's needed to test this change, but
that's beyond the scope of this PR.
fixes #12257