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

Focus jumps to body when updating content inside FocusTrap #962

Closed
kasperg opened this issue Mar 26, 2023 · 13 comments · Fixed by #998
Closed

Focus jumps to body when updating content inside FocusTrap #962

kasperg opened this issue Mar 26, 2023 · 13 comments · Fixed by #998

Comments

@kasperg
Copy link

kasperg commented Mar 26, 2023

Thanks for sharing focus-trap.

I am trying to use it to retain focus inside a <dialog> with a <FocusTrap> inside but when I update content inside that <dialog> clicking on a <button> the active element jumps to the <body> element. When using tab focus moves back within the focus trap but I had expected it to stay within the focus trap during the entire process as it contains another focusable element during the entire proces.

Based on my testing none of the callbacks provided by focus trap are called either.

I have tried to show the process here and in this sandbox:

Monosnap.screencast.2023-03-26.14-51-25.mp4

https://codesandbox.io/s/focus-trap-react-body-focus-jump-khjkr8?file=/src/App.js

I am not sure whether this behavior is a bug or actually to be expected based on the implementation.

@stefcameron
Copy link
Member

@kasperg

Thanks for sharing focus-trap.

You're welcome! 😄

I am trying to use it to retain focus inside a <dialog> with a <FocusTrap> inside but when I update content inside that <dialog> clicking on a <button> the active element jumps to the <body> element. When using tab focus moves back within the focus trap but I had expected it to stay within the focus trap during the entire process as it contains another focusable element during the entire proces.

Based on my testing none of the callbacks provided by focus trap are called either.

All the callback options provided by focus-trap (which is wrapped by focus-trap-react) are all based on trap activation/deactivation, and pointer/keyboard events related to preventing focus escape.

I starred at your recording and your code for a while (thank you; very useful!) and the best explanation I can come up with is that the focus escapes because there is a short gap of time where no trap exists.

Since clicking on the "Go to variant X" button in either variant changes a state variable in the app itself, it should be causing the <Modal> component to re-render because it'll get new children. Since <Modal> renders the <FocusTrap>, I'm guessing the previous instance gets unmounted, focus appears to escape (because there's actually no trap for a brief moment in time), and then the <Modal> is rendered again, with a new <FocusTrap>. And since the trap is active by default, from the user's perspective, the focus remains trapped in the modal, even though programmatically you're seeing it temporarily exit the modal.

But what bothers me in this is that if I'm correct, then why are we not seeing your onActivate() and onDeactivate() handlers get called -- if, indeed, the old trap is unmounted (which should lead to auto-deactivation) and a new one is rendered (which should lead to a new activation)?

The variants of the modal only concern its content, not the entire modal, I would recommend you move that variant state into the modal, inside the trap and flip it there. Whether I'm right or not quite right, I think I'm close, and I'm thinking that rearranging your logic that way would eliminate the issue.

If you're not satisfied by that theory, then what would be helpful is to see if you can replicate this using only https://github.com/focus-trap/focus-trap, eliminating React and focus-trap-react's wrapper from the equation. That would help further isolate the issue.

Note that focus-trap is not impervious to focus escape and does have some logic in it to "pull it back in", so to speak, on the next pointer or key event it sees. Things do get risky when the content of the trap changes, especially when async operations (like React render cycles) are involved -- and in your case, you definitely have some timing gaps where things are changing, including the trap itself (potentially).

The following section of focus-trap code is where most of the focus escape mitigation occurs: https://github.com/focus-trap/focus-trap/blob/master/index.js#L414-L460

@kasperg
Copy link
Author

kasperg commented Mar 28, 2023

@stefcameron Appreciate the thorough followup.

I would recommend you move that variant state into the modal, inside the trap and flip it there.

I have tried to do this in a forked version of the sandbox: https://codesandbox.io/s/focus-trap-react-body-focus-jump-v2-t0m12f?file=/src/App.js. Unfortunately it does not seem to make any difference regarding the problem.

[...] why are we not seeing your onActivate() and onDeactivate() handlers get called -- if, indeed, the old trap is unmounted (which should lead to auto-deactivation) and a new one is rendered (which should lead to a new activation)?

These are my thoughts exactly.

[...] what would be helpful is to see if you can replicate this using only https://github.com/focus-trap/focus-trap, eliminating React and focus-trap-react's wrapper from the equation.

I agree that is a reasonable next step.

@kasperg
Copy link
Author

kasperg commented Mar 30, 2023

I have tried to recreate the setup in pure JavaScript with the core focus-trap project: https://codesandbox.io/s/focus-trap-vanillajs-07it1h. As I see it the problem is the same here and thus it is not React specific.

With that in mind I tried to dig a bit more into the situation and found this:

If the element that is currently focused is removed from the DOM, the focus is moved either to the document's body element or becomes null

Source: https://amberwilson.co.uk/blog/where-did-the-focus-go/

Note that in my example no event is fired when focus changes to the body. With that and the above explanation in mind one approach to mitigate focus escape in this example might be to attach a MutationObserver to the trap and then try to detect if the currently focused element is removed and in that example pull focus back into the trap.

@stefcameron
Copy link
Member

@kasperg Thanks for testing this in focus-trap, and starting that PR over there for an eventual fix, much appreciated!

With that in mind I tried to dig a bit more into the situation and found this:

If the element that is currently focused is removed from the DOM, the focus is moved either to the document's body element or becomes null

Source: https://amberwilson.co.uk/blog/where-did-the-focus-go/

Note that in my example no event is fired when focus changes to the body. With that and the above explanation in mind one approach to mitigate focus escape in this example might be to attach a MutationObserver to the trap and then try to detect if the currently focused element is removed and in that example pull focus back into the trap.

Nice find in that blog post. The MutationObserver idea is interesting though I'm a bit concerned at how relatively "new" it is to Safari (vs Chrome, Edge, FF, Opera). Obviously not new-new, but much newer, at least in terms of version history, than all the others.

But the concept of what you're proposing could work well. Since that blog post is using the focusin event to log the currently-focus element whenever that changes, it seems like there might be an easier fix in focus-trap's focusin handler: https://github.com/focus-trap/focus-trap/blob/master/index.js#L415-L429

I'm not immediately sure which branch of the IF statement we're falling into when the element is removed. I guess it would be the first (true) branch, but then targetContained is likely false and so we do nothing.

Looking at this code (which predates my involvement as a Maintainer; so it's been there for a long time!) again, I'm suddenly intrigued by the fact that if target instanceof Document (which would imply a focus-escape, right!?), we just happily stand by, as it were. 🤔 It's like focus-trap went for a passive approach in that case, waiting for the next click or tab key from the user to then pull focus back in.

Seems like we should treat that as an escape.

Also, in light of your use case (focused element gets removed from the DOM), I think we'll need to add a check for state.mostRecentlyFocusedNode no longer being in the DOM because it won't be null yet. It'll still have a reference to that node...

Something like this inspired by: https://github.com/focus-trap/tabbable/blob/master/src/index.js#L314

    } else {
      // escaped! pull it back in to where it just left
      e.stopImmediatePropagation();
      if (state.mostRecentlyFocusedNode && isFocusable(state.mostRecentlyFocusedNode, config.tabbableOptions)) {
        tryFocus(state.mostRecentlyFocusedNode);
      } else {
        // either we don't have a most-recently-focused node (strange) or it's likely no longer in the DOM
        tryFocus(getInitialFocusNode());
    }

WDYT?

@kasperg
Copy link
Author

kasperg commented Apr 5, 2023

Since that blog post is using the focusin event to log the currently-focus element whenever that changes, it seems like there might be an easier fix in focus-trap's focusin handler

@stefcameron, I agree that this would be a preferable fix as it builds on constructs already used within the project. I do not think it will work though.

A breakpoint placed within the focus-trap's focusin handler is not triggered when removing the focused button in tests. I have also tried to update the vanilla sandbox to log both focus and focusin events and as far as I can tell neither of the listeners are triggered when the currently focused element is removed from the DOM.

I also updated the sandbox with an example MutationObserver for reference. It is triggered.

@stefcameron
Copy link
Member

Since that blog post is using the focusin event to log the currently-focus element whenever that changes, it seems like there might be an easier fix in focus-trap's focusin handler

@stefcameron, I agree that this would be a preferable fix as it builds on constructs already used within the project. I do not think it will work though.

A breakpoint placed within the focus-trap's focusin handler is not triggered when removing the focused button in tests. I have also tried to update the vanilla sandbox to log both focus and focusin events and as far as I can tell neither of the listeners are triggered when the currently focused element is removed from the DOM.

I also updated the sandbox with an example MutationObserver for reference. It is triggered.

@kasperg I see what you mean... I found another blog post about monitoring the element that gets focus, and that one indicated that focus is fired even for non-interactive elements, and that it's possible to capture the focus event on the window.

So I tried console logging the event.target in:

  • doc.addEventListenter('focusin', handler, true) (existing handler)
  • doc.addEventListenter('focus', handler, true)
  • doc.defaultView.addEventListenter('focus', handler, true)
  • doc.body.addEventListenter('focus', handler)

When the button is clicked and removed, nothing fires. 🤯 Going back to the DevTools, document.activeElement reveals <body> as the apparently-focused element.

I'm starting to think that the <body> can't really have focus and that document.activeElement reporting that <body> has focus is like saying "nothing has focus" (curious why document.activeElement doesn't return null in that case, though, since the API docs state the property could be null). In fact, document.body.focus() does nothing. Focus never actually goes there (it remains on the element that has it).

So if we end-up in this situation, has focus really escaped the trap? It seems to be nowhere more than actually being somewhere...

Clicking around in this state will just cause the trap to prevent clicks if they're outside of it (or deactivate the trap if that's configured). Pressing tab will immediately discover focus isn't in the trap (because document.activeElement reports that the body has it, but does it, really...?) and move focus to the first element in the trap.

It all seems pretty seamless. If we add the MutationObserver, what would we do differently other than just setting focus back to the first element in the trap? We shouldn't try to set focus to what would've been the next element in tab order because focus-trap tries to refrain from tab order per se and let the browser handle it, so if you tabbed to the button and pressed the Enter key and it disappeared on you, seems better to send focus back to the first element in the trap.

Does the fact that document.activeElement becomes the <body> have a negative effect on your dialog use case?

@kasperg
Copy link
Author

kasperg commented Apr 13, 2023

Does the fact that document.activeElement becomes the have a negative effect on your dialog use case?

@stefcameron The problem originates in a bug report where a colleague has been checking the accessibility of our application.

They use NVDA and in this case they claim it is a problem referring to WCAG 2.1 success criterium 2.4.3 regarding focus order.

NVDA is only available on Windows and being on a Mac myself it is not easy to verify. I have tried testing the sandbox using NVDA a remote machine and as far as I can tell it is also a problem here.

react-focus-trap-nvda.mov

As I see it:

  1. The user opens the modal by clicking the "Open dialog" button
  2. The NVDA viewer prints the "Switch to variant 2" and "Close dialog" buttons
  3. The user clicks the "Switch to variant 2" button
  4. The currently focused element dissapears and is replaced by "Loading" and then "Another button" button
  5. The NVDA viewer prints the "Open dialog" and "Some button" buttons

I had expected 5) to be that the NVDA viewer prints Loading and then prints the "Another button" and "Close dialog" buttons.

I also tried checking the sandbox using OSX VoiceOver. I find it harder to recreate the results here but as I see it the process is as follows:

  1. The user opens the modal by clicking the "Open dialog" button
  2. VoiceOver says "Switch to variant 2" button
  3. The user clicks the "Switch to variant 2" button
  4. The currently focused element dissapears and is replaced by "Loading" and then "Another button" button
  5. VoiceOver does nothing
  6. The user clicks "Close dialog"
  7. VoiceOver says "Open dialog"

Based on the behavior by VoiceOver in 2) and 7) by immediately saying the name of the currently focused element I had expected VoiceOver to either say "Loading" and then "Another button button", "Close dialog button" or "Another button button".

If we add the MutationObserver, what would we do differently other than just setting focus back to the first element in the trap?

I think the right way to go would be to set focus back to the first element in the trap if the currently focused element is removed. One way to do so might be to reactivate the trap containing the removed element.

@kasperg
Copy link
Author

kasperg commented Apr 13, 2023

The ally.js project has a nice writeup and playground on what happens across browsers when you remove/detach the currently focused element: https://allyjs.io/tutorials/mutating-active-element.html.

@stefcameron
Copy link
Member

The ally.js project has a nice writeup and playground on what happens across browsers when you remove/detach the currently focused element: https://allyjs.io/tutorials/mutating-active-element.html.

Very useful writeup -- as well as all your testing with NVDA remote and macOS Voice Over!

If we add the MutationObserver, what would we do differently other than just setting focus back to the first element in the trap?

I think the right way to go would be to set focus back to the first element in the trap if the currently focused element is removed. One way to do so might be to reactivate the trap containing the removed element.

If it weren't for the screen reader reading "Open dialog.... Some button" as soon as the element is removed, I'd lean more toward saying this isn't technically a focus escape (because a focused <body> is like "no man's land" based on my previous observations; you couldn't programmatically focus the body if you wanted to!).

But from the disabled user's point of view, it is. So I'm back leaning the other way, and I'm not sure how else to address it other than with a MutationObserver.

The new code would have to be careful to check if the API exists before using it (for browser backward compatibility), and upon discovering the mostRecentlyFocusedNode has been removed, I think the consistent thing to do would be to call tryFocus(getInitialFocusNode()) since getInitialFocusNode will execute a series of fallbacks if the option wasn't set, including setting focus to the first node in the first container in the trap.

We would probably have to add a check in getInitialFocusNode() to see if the node is attached to the DOM or not, since it could be the initial node that was removed, in which case we could treat that the same as if the option weren't specified and go into the fallback options.

As for the observer, I think there are two possible approaches there:

  1. It observes each trap container for node removals therein. Any time updateContainerElements is called, the observer is reset and restarts observing the new containers given. The callback checks the removed node against state.mostRecentlyFocusedNode.
  2. It observes only the state.mostRecentlyFocusedNode and would be updated in tryFocus() and checkFocusIn() where that property is updated (or the property is converted into a get()/set() pair and the observer is updated in the setter so it doesn't matter where it gets updated).

I'm not sure how the observer reacts when the very node it's watching gets removed. Approach (1) might be the only way.

WDYT about this solution?

@kasperg
Copy link
Author

kasperg commented Apr 14, 2023

I also think that approach 1 is the way to go.

Your description above along with the test in focus-trap/focus-trap#932 should be helpful when implementing a fix.

@stefcameron
Copy link
Member

@all-contributors add @kasperg for bug

@allcontributors
Copy link
Contributor

@stefcameron

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

@stefcameron
Copy link
Member

Published in focus-trap-react v10.1.4

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 a pull request may close this issue.

2 participants