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

fix(useFullscreen)!: better cross-platform compatibility #2915

Merged
merged 3 commits into from Apr 12, 2023
Merged

fix(useFullscreen)!: better cross-platform compatibility #2915

merged 3 commits into from Apr 12, 2023

Conversation

ferferga
Copy link
Contributor

@ferferga ferferga commented Mar 28, 2023

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Description

  • Functions are fully reactive (even after a target change, which wasn't handled before as stated here)
  • Function mapping is much simpler
  • Fix 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) to webkitEnterFullscreen (notice the S) as it was stated in Apple docs that the one without the case was the one to be used. However, Safari iOS only had webkitEnterFullScreen function defined, rendering my PR completely useless (thanks Apple!)

While investigating and testing everything again, I found out further modifications needed to be done:

  • Everything had to be targeted to the media element, not document, while the composable only handled the document in some places.
  • The event handlers for iOS's Safari also had to be for the target, not the document
  • I wanted to ensure maximum compatibility and have all the functions I found in the various doc pieces of browser vendors. The previous way to map functions, besides not being the best for reactivity, were really strict: we had to provide a fixed 1:1 set. With this new approach, we can have, for example, more known enter functions (7 right now) than exit (6) and we don't force. Before, if we wanted to support webkitEnterFullScreen and webkitEnterFullscreen, 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 supported webkitEnterFullScreen to also had a webkitExitFullscreen. What if the browser had webkitEnterFullScreen as an enter function but webkitCancelFullScreen 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.
  • The fullscreen element properties was unused, bloating bundle size.

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

  • Rewrite useFullscreen function to use computed properties, feature detection and event listeners (link)
  • Import computed, watchEffect and Fn modules from vue-demi and @vueuse/shared (link)
  • Remove unused functionsMap array (link)
  • Add eventHandlers array to store fullscreen change events (link)
  • Modify enter function to use requestMethod and isElementFullScreen (link)
  • Modify toggle function to use ternary operator, handlerCallback and watchEffect (link)
  • Add comment to usage section of documentation to warn about iOS Safari limitations (link)

@ferferga ferferga changed the title refactor(useFullscren): better cross-platform compatibility refactor(useFullscren): better cross-platform compatibility and remove dead code Mar 28, 2023
@ferferga ferferga changed the title refactor(useFullscren): better cross-platform compatibility and remove dead code refactor(useFullscreen): better cross-platform compatibility and remove dead code Mar 28, 2023
if (targetHandlerCleanup)
targetHandlerCleanup()

targetHandlerCleanup = useEventListener(target, eventHandlers, handlerCallback, false)
Copy link
Contributor Author

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?

@Alfred-Skyblue
Copy link
Member

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.

@ferferga
Copy link
Contributor Author

ferferga commented Mar 29, 2023

@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.

@Alfred-Skyblue
Copy link
Member

It seems so

@ferferga
Copy link
Contributor Author

@Alfred-Skyblue What are you referring to specifically? 😅

@antfu antfu changed the title refactor(useFullscreen): better cross-platform compatibility and remove dead code fix(useFullscreen)!: better cross-platform compatibility Apr 12, 2023
@antfu antfu enabled auto-merge (squash) April 12, 2023 13:28
@antfu antfu merged commit 2e46781 into vueuse:main Apr 12, 2023
3 checks passed

const fullscreenEnabled = computed<'fullscreenEnabled' | undefined>(() => {
return [
'fullScreen',

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. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants