-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(useFullscreen)!: better cross-platform compatibility #2915
Conversation
…mapping for unsupported browsers
packages/core/useFullscreen/index.ts
Outdated
if (targetHandlerCleanup) | ||
targetHandlerCleanup() | ||
|
||
targetHandlerCleanup = useEventListener(target, eventHandlers, handlerCallback, false) |
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.
Just wondering: Being this inside a watchEffect
, perhaps I need to pass targetHandlerCleanup
to tryOnScopeDispose
so the event handlers are cleared?
I always think that using compatibility attributes as computed properties is a bit inappropriate because the browser's support for those methods is determined at render time and does not change due to certain factors. Therefore, I believe that we should not calculate them every time we call them, but rather calculate them during initialization and use the result as a constant. This can reduce unnecessary calculations and improve performance. |
@Alfred-Skyblue That's not the case here, since target can be a ref and we need to check if the method exists both in document and in target, since some browsers (like Safari, the only I know) only support them in media elements. I'd like some input though in why no more composables do this (have state outside of the function) though: some composables like useWindowScroll and useIntersectionObserver would benefit from having global state regardless where they're called: window scroll will be the same everywhere and for intersection observer we can simply observe and unobserve elements, instead of recreating an intersection observer in every composable call. But all of that it's for another PRs for sure. I'm curious on an explanation though and if such changes would be welcome. |
It seems so |
@Alfred-Skyblue What are you referring to specifically? 😅 |
|
||
const fullscreenEnabled = computed<'fullscreenEnabled' | undefined>(() => { | ||
return [ | ||
'fullScreen', |
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.
Hello, I'd like to confirm if this property name is really correct and expected. I didn't see the fullScreen
property but see the fullscreen
property from the MDN documentation. 🤔
Before submitting the PR, please make sure you do the following
fixes #123
).Description
Document is not active
errors in older Chrome-based versions.Additional context
The merge of #2822 attempted to add compatibility to Safari iOS by introducing a new entry in the function map array. However, in one of the rebases I performed to keep the branch up to date, I changed the case from
webkitEnterFullScreen
(which is the one used in my original PR description, as you can see) towebkitEnterFullscreen
(notice theS
) as it was stated in Apple docs that the one without the case was the one to be used. However, Safari iOS only hadwebkitEnterFullScreen
function defined, rendering my PR completely useless (thanks Apple!)While investigating and testing everything again, I found out further modifications needed to be done:
webkitEnterFullScreen
andwebkitEnterFullscreen
, we had to define an entire array with the rest of the functions defined (just the first entry would change), bloating bundle size. We also forced a browser that supportedwebkitEnterFullScreen
to also had awebkitExitFullscreen
. What if the browser hadwebkitEnterFullScreen
as an enter function butwebkitCancelFullScreen
as the exit one?. Current approach doesn't force anything on browsers, simply checks at runtime and takes the first available method to work with.All the cross-browser testing of the current pushed code has been conducted by adding the composable to my project and testing through BlueStack to ensure everything's working perfectly in all the devices, so no more problems arise. Sorry for all the inconvenience 😅.
🤖 Generated by Copilot at 075e5c7
This pull request improves the
useFullscreen
function and its documentation. It refactors the code to use more modern and compatible features, and adds a comment to the docs to inform users about platform-specific limitations.🤖 Generated by Copilot at 075e5c7
useFullscreen
function to use computed properties, feature detection and event listeners (link)computed
,watchEffect
andFn
modules fromvue-demi
and@vueuse/shared
(link)functionsMap
array (link)eventHandlers
array to store fullscreen change events (link)enter
function to userequestMethod
andisElementFullScreen
(link)toggle
function to use ternary operator,handlerCallback
andwatchEffect
(link)