Fix issue with returnFocusOnDeactivate not working when active pr…

[krikienoid]

focus-trap-react can be activated/deactivated by either mounting/unmounting the component, or by passing a boolean value to the active prop. When using the latter method, there is an issue where the focus is not returned on deactivation.

Explicitly setting returnFocusOnDeactivate to true partially fixes the problem, but the issue persists when deactivation is triggered via the ESCAPE key.

I think the cause of the issue is that the component references this.previouslyFocusedElement when mounting/unmounting but not when a prop is updated.

[stefcameron]

@krikienoid Thanks for making an attempt at this, and I'm sorry your PR has gone so long without response. The project went quiet for a while, and I'm trying to take over now.

I did some digging and found the issue. It's related timing, the ESC key handling in focus-trap, and this line: https://github.com/focus-trap/focus-trap/blob/master/index.js#L111

If you test with the "special element" example, you can reproduce this bug by hitting ESC to deactivate the trap instead of clicking on the "deactivate" button. This is the sequence of events that ensues:

1. focus-trap's deactivate() method gets called thanks to its ESC key handler and immediately deactivates the trap at https://github.com/focus-trap/focus-trap/blob/master/index.js#L98

   NOTE: The trap was created with returnFocusOnDeactivate: false thanks to focus-trap-react's special handling of that feature here: https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L28

2. focus-trap-react's componentDidUpdate() handler then gets called, determines the trap should no longer be active because the active prop is now false so it creates the deactivate config with returnFocus: true because returnFocusOnDeactivate: true was provided as initial trap options. It then calls https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L69 but this does nothing because of https://github.com/focus-trap/focus-trap/blob/master/index.js#L90 which determines the trap is no longer active and returns immediately. So returnFocus: true is lost here.

3. Execution returns to https://github.com/focus-trap/focus-trap/blob/master/index.js#L98 and continues to https://github.com/focus-trap/focus-trap/blob/master/index.js#L108-L111 where deactivateOptions === undefined (because this call into deactivate() came from the ESC key handler, NOT componentDidUpdate()...) and we fallback to https://github.com/focus-trap/focus-trap/blob/master/index.js#L111 which determines returnFocusOnDeactivate: false (because of https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L28) and therefore focus is NOT returned to the "activate" button.

That's the series of unfortunate events.

I think the cause of the issue is that the component references this.previouslyFocusedElement when mounting/unmounting but not when a prop is updated.

Your solution, while it may work, attempts to accomplish some work that focus-trap should be handling instead. I'd like to see if there's a way to let focus-trap do its thing. Any ideas?

[JoSuzuki]

Hey, I've just ran into this bug and I am not very familiar with open-source so I apologize if this is not the correct place to comment this, if it is I'd be happy to remove it.

First of all thanks for maintaining this package!

By what I could understand of the comment

focus-trap-react/src/focus-trap-react.js

Lines 21 to 25 in 6a80deb

	// We need to hijack the returnFocusOnDeactivate option,
	// because React can move focus into the element before we arrived at
	// this lifecycle hook (e.g. with autoFocus inputs). So the component
	// captures the previouslyFocusedElement in componentWillMount,
	// then (optionally) returns focus to it in componentWillUnmount.

the focus-trap-react wrapper isn't using the focus-trap intended way of capturing the previously focused element, most likely because of the limitation described there. So it implements a hacky way of dealing with that by saving it on the component and updating it properly on mount/unmount.

Probably the interaction of the active prop slipped through that iteration. This is just a guess, but the implementation suggested in this PR seems to be the most aligned with what could've been implemented at that time.

One alternative I see is adding a function on focus-trap-react that would always pass to focus-trap as an onDeactivate function and this function does something similar the unmounting process does already

So the lines https://github.com/focus-trap/focus-trap-react/blob/6a80debd55ec76cfeaeb8fdea67c4c8952bd75ce/src/focus-trap-react.js#L52-55 the tailoredFocusTrapOptions would look like something along the lines

const newTailoredFocusTrapOptions = {
	...tailoredFocusTrapOptions,
	onDeactivate: () => {
		if(tailoredFocusTrapOptions.onDeactivate) {
			tailoredFocusTrapOptions.onDeactivate()
		}
		if (
	      this.props.focusTrapOptions.returnFocusOnDeactivate !== false &&
	      this.previouslyFocusedElement &&
    	  this.previouslyFocusedElement.focus
	    ) {
    	  	this.previouslyFocusedElement.focus();
    	}
	}
}

Which will probably mean that we can remove the check https://github.com/focus-trap/focus-trap-react/blob/6a80debd55ec76cfeaeb8fdea67c4c8952bd75ce/src/focus-trap-react.js#L66-68 and https://github.com/focus-trap/focus-trap-react/blob/6a80debd55ec76cfeaeb8fdea67c4c8952bd75ce/src/focus-trap-react.js#L83-89

This still doesn't solve the issue that you brought that focus-trap-react is dealing with something that focus-trap should be dealing. To me that could be mapped as a tech-debt (as it already exists in the codebase), but again I'm very inexperienced on how open-source deals with it, unfortunately I also don't have any idea on how to solve this problem 😕 (one thing that crossed my mind would be exposing in focus-trap what the nodeFocusedBeforeActivation is when creating the focusTrap object, still not sure if that would be a good choice either)

[krikienoid]

I'm having trouble understanding what is going on with the code, but it sounds like focus-trap-react needs to call the deactivate method twice in order to work, and the config settings are getting lost between the two calls, to put it simply.

I'm thinking maybe this could be fixed by making sure the settings are always passed in the escape key method. I haven't had a chance to test this out, but I'm thinking of modifying this method to something like this:

https://github.com/focus-trap/focus-trap/blob/master/index.js#L261

  function checkKey(e) {
    if (config.escapeDeactivates !== false && isEscapeEvent(e)) {
      e.preventDefault();
      deactivate({
        returnFocus: config.returnFocusOnDeactivate
      });
      return;
    }
    if (isTabEvent(e)) {
      checkTab(e);
      return;
    }
  }

[stefcameron]

@JoSuzuki @krikienoid Thanks to both of you for your thoughts on this bug! 😄

Since I added my thoughts last Aug 29, focus-trap/focus-trap#103 has surfaced, which seems at least partly related, and is a bug that should be fixed over in focus-trap. I think the fix needed there is simple. @krikienoid I think fixing this issue might essentially equate to what you were suggesting...?

Before determining that the fix to this issue we're discussing here belongs either here or in focus-trap, I think it would be good to see what happens to this issue once focus-trap/focus-trap#103 is fixed. Kind of hard to imagine the effects in my head without tinkering with the code -- unless one of you can process it! 😜

I'm trying to get that one done. Fix is easy, but I have to add tests, etc. That's taking a bit of time (to find time), but it's my current focus. Once I've got the fix in, I'll publish, and then it'll be easier to test over here.

I'm not sure it'll fix the problem, but worth seeing the effects in case it helps. There's also the problem here in focus-trap-react that the ESC key path causes the trap to try to be deactivated twice, which would be nice to avoid somehow.

I do like the idea of leveraging focus-trap's onDeactivate() event. 🤔

[stefcameron]

FYI, focus-trap/focus-trap#103 has been fixed and published as focus-trap@6.1.1

[stefcameron]

@JoSuzuki @krikienoid I did some work on this over on #139 and I think I've fixed it. If one of you could somehow test my changes with your code and let me know, I'd really appreciate it. I did address the behavior I brought-up earlier with the "special element" test and the ESC key, and I also found a couple of other things to fix, all related, along the way.

[stefcameron]

@JoSuzuki @krikienoid I did some work on this over on #139 and I think I've fixed it. If one of you could somehow test my changes with your code and let me know, I'd really appreciate it. I did address the behavior I brought-up earlier with the "special element" test and the ESC key, and I also found a couple of other things to fix, all related, along the way.

@JoSuzuki @krikienoid I'd love for one of you to check out #139 if you have the time. I think I have a good fix, but it's kind of a shot in the dark WRT to your specific issues related to the returnFocusOnDeactivate option. If not, that's OK. I'll merge next weekend if I don't hear back before then.

[krikienoid]

I took a quick look at #139 and it appears to have fixed the issue!

[stefcameron]

I took a quick look at #139 and it appears to have fixed the issue!

@krikienoid Thank you very much for checking it out! I will merge it, then, and publish in the next few days.

[stefcameron]

@all-contributors add @krikienoid for bug

[stefcameron]

@all-contributors add @JoSuzuki for bug

[allcontributors]

@stefcameron

I've put up a pull request to add @krikienoid! 🎉

[allcontributors]

@stefcameron

I've put up a pull request to add @JoSuzuki! 🎉

[stefcameron]

Closing in favor of the implementation/fix in #139