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

feat: if not active, don't try focusing #101

Closed
wants to merge 1 commit into from

Conversation

cgood92
Copy link
Contributor

@cgood92 cgood92 commented Nov 12, 2019

Fixing issue where deactivation of focus trap could happen before attempting to focus something again on a delay.

This will fix a problem we are having. Here is a replication of that problem: https://codesandbox.io/s/upbeat-mendel-kbdzw .

Fixing issue where deactivation of focus trap could happen before attempting to focus something again on a delay.
@stefcameron
Copy link
Member

@cgood92 Thanks for this PR. Sorry it's taken so (very) long to have someone look at it. The repo fell out of maintenance for the better part of the last 12 months, and I've just taken over a couple of months ago, and trying to catch-up.

Thanks for the sandbox. I forked it at https://codesandbox.io/s/hopeful-field-2i9fr in order to update the focus-trap-react and focus-trap dependencies to the latest versions to make sure this was still an issue.

As best I can tell, after staring at this for a long while, is that it's actually related to what focus-trap/focus-trap-react#54 is trying to fix, which is essentially a double-deactivation loop with the focus trap that happens when clickOutsideDeactivates=true, because with that option enabled, focus-trap immediately deactivates the trap when you click on the green square (in the sandbox), and then the <FocusTrap/> component unmounts and attempts to deactivate it again, and things to bad, similar to the issue described in that PR.

Unfortunately, the fix to returnFocusOnDeactivate in focus-trap@6.1.1 didn't help.

@@ -301,6 +301,7 @@ function focusTrap(element, userOptions) {
}

function tryFocus(node) {
if (!state.active) return;
Copy link
Member

Choose a reason for hiding this comment

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

The fix you're proposing here seems right, but I don't like that we're checking for the activate state in tryFocus(). I think the fix belongs more in the timeout related to the delayed focus on activation, at line 145, just before calling tryFocus().

tryFocus() itself doesn't need to be concerned about state.

And with newer options in focus-trap, I thought the new delayInitialFocus option set to false might help since your description makes it sound like that's basically the problem (because you say the trap is deactivated by the time the initial focus is set), but that didn't help either. Maybe I misinterpreted something there.

Copy link
Member

Choose a reason for hiding this comment

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

@cgood92 Since you're around... any thoughts on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, there was a reason for this, but I honestly can't remember a thing about it. I'll close this PR, as it may be irrelevant.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks!

@stefcameron stefcameron added the waiting Waiting for response from community label Oct 3, 2020
@cgood92 cgood92 closed this Nov 16, 2020
@stefcameron stefcameron removed the waiting Waiting for response from community label Nov 16, 2020
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.

2 participants