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

♻️ Refactor Focus Manager Event Handling for Enhanced User Experience ✨ #2672

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sanjaiyan-dev
Copy link
Contributor

As you can see, the focus manager currently listens to both visibilitychange and focus events by default.

I believe it may be beneficial to consider discontinuing the listening to focus events and solely rely on visibilitychange events.

My rationale behind this proposal is that we might be supporting both events primarily for historical reasons. It is worth noting that visibilitychange was not fully supported in older browsers that are no longer relevant (e.g., IE11).

The focus event has proven to have various pitfalls, as it can be triggered in the following scenarios:

  1. Navigating between the application and DevTools.
  2. Closing an alert or confirm dialog.
  3. Interacting with a file upload dialog.
  4. Interacting with an iframe.

These scenarios have the potential to result in a suboptimal user experience.

This pull request is inspired by this reference PR.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 17, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 17d1760:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration
SWR-SSR Configuration

@shuding
Copy link
Member

shuding commented Jun 17, 2023

Like it, although there're some tests that we need to update.

@sanjaiyan-dev
Copy link
Contributor Author

Like it, although there're some tests that we need to update.

Ok, sure! But could you please guide me through it? I apologize for any inconvenience caused.

@promer94
Copy link
Collaborator

Most of tests are based on focus event and fireEvent.focus. I guess you need to replace the focus event with visibilitychange event like what @tanstack/query did

@sanjaiyan-dev
Copy link
Contributor Author

Most of tests are based on focus event and fireEvent.focus. I guess you need to replace the focus event with visibilitychange event like what @tanstack/query did

Okay :)

@sanjaiyan-dev
Copy link
Contributor Author

Hi (@promer94 , @shuding ),

I apologize for the inconvenience, but I'm having trouble updating the test code correctly. I'm not familiar with the process, and I don't want to risk introducing errors or breaking anything. Could someone please lend me a hand and guide me through the correct steps? I'm sorry for any delays this may cause, and I truly appreciate your assistance.

@promer94
Copy link
Collaborator

promer94 commented Jul 1, 2023

https://github.com/TanStack/query/pull/4805/files#diff-3cd8cae55f836a38ea976d6cbf9f1b59e8fdfa4d3218b123cb1f9dccfc6824d2
I am sure what blocks you but i think this diff clearly shows what you need to do here.

@shuding
Copy link
Member

shuding commented Jul 21, 2023

Changes look good to me but this might need to be landed in v3 as it's breaking.

@sanjaiyan-dev
Copy link
Contributor Author

Changes look good to me but this might need to be landed in v3 as it's breaking.

Okay 🙌

And thanks a lot @promer94

@koba04
Copy link
Collaborator

koba04 commented Jul 24, 2023

After this change has landed, we should change the example (Revalidate on Focus) in the docs because some behaviors in the example depend on the focus event.
https://swr.vercel.app/docs/revalidation#revalidate-on-focus

I feel revalidateOnVisible is a more appropriate name for this.

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@AhmedBaset
Copy link

Changes look good to me but this might need to be landed in v3 as it's breaking.

Any updates on how close is the v3? Can we have like future flags or a canary channel?

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

Successfully merging this pull request may close these issues.

5 participants