Doesn't seem to work in other documents

[LoganDark]

I appreciate the effort you have put into making the focus trap library work in iframes and child windows, but unfortunately it seems like maybe the React version is not prepared for this?

I'm using the FocusTrap like <FocusTrap active={element !== null} focusTrapOptions={{document}}>, where document is the document of the child window (derived from element.ownerDocument), but the focus trap still isn't trapping focus. When the same component is used in the main document, it works just fine.

Do you know what could be the cause of the issue?

[stefcameron]

You're welcome! 🙂

It's quite possible there are some things the code isn't handling. I had another look at the code, and I found one case where it's not respecting the focusTrapOptions.document setting: /~https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L66

When you say it doesn't work, have you checked your console for errors? If you're hitting the issue above, you might have exceptions in the console from line 68, and that would cause the trap not to function properly.

If not, then something else is going on.

I'm using the FocusTrap like <FocusTrap active={element !== null} focusTrapOptions={{document}}> [...]

Just a thought here, and probably wouldn't change anything, but {{document}} in there gives me the impression you're storing the document you want to give to focus-trap into a variable, like const document = something which means you're shadowing the global document. This probably works, but just because you named it document, maybe it's not the document you think it is? Shot in the dark...

[...] where document is the document of the child window (derived from element.ownerDocument), [...]

So your code is more like <FocusTrap active={element !== null} focusTrapOptions={{document: element.ownerDocument}}>?

Also, what is element in active={element !== null}? Sounds like an HTML Element obtained through a Ref. It would be better to avoid this if possible and use a state variable, for example, to decide if the trap should be active or not.

Another idea is to put the trap in the other document. For example, if you're using this for a modal window of some kind, put the trap in that modal component, and create it only when that modal is created, instead of keeping it "alive" outside of it and trying to activate it only when the modal is visible.

[LoganDark]

When you say it doesn't work, have you checked your console for errors? If you're hitting the issue above, you might have exceptions in the console from line 68, and that would cause the trap not to function properly.

If not, then something else is going on.

No errors.

Just a thought here, and probably wouldn't change anything, but {{document}} in there gives me the impression you're storing the document you want to give to focus-trap into a variable, like const document = something which means you're shadowing the global document. This probably works, but just because you named it document, maybe it's not the document you think it is? Shot in the dark...

Indeed I am storing it in a variable:

	const document = useMemo(() => menu?.ownerDocument, [menu])

menu is a state variable that is updated using callback refs, I can confirm it's set correctly (and document too).

Sounds like an HTML Element obtained through a Ref. It would be better to avoid this if possible and use a state variable, for example, to decide if the trap should be active or not

If it was a ref, it would be element.current !== null, which would be incorrect because changing the ref wouldn't update the component.

Another idea is to put the trap in the other document. For example, if you're using this for a modal window of some kind, put the trap in that modal component, and create it only when that modal is created, instead of keeping it "alive" outside of it and trying to activate it only when the modal is visible.

The trap is in the other document. It wraps a div whose document is the document I'm passing to focusTrapOptions:

	const [menu, setMenu] = useState<HTMLDivElement | null>(null)
	const document = useMemo(() => menu?.ownerDocument, [menu])

	// a small amount of other stuff but you get the point...

	return <FocusTrap active={menu !== null} focusTrapOptions={{document}}>
		<div ref={setMenu} {...otherProps}>
			{children}
		</div>
	</FocusTrap>

The only reason active is false for one state update is because I don't want to activate it before the document option is passed - to have the best chances of document actually working (although it still does not).

[stefcameron]

OK, then what do you mean when you say it's "not working"? Is it that the trap activates, and traps focus within the child document, but you can still click on elements outside (in the host document)? That's a limitation of having the trap work on a child document.

Also, just to check everything, you should be using focus-trap-react v8.8.0 or greater. That's the version that added support for the focus-trap document option.

[LoganDark]

OK, then what do you mean when you say it's "not working"? Is it that the trap activates, and traps focus within the child document, but you can still click on elements outside (in the host document)? That's a limitation of having the trap work on a child document.

No. I can still use tab and shift+tab to navigate through other elements in the focus trap's document. I'm aware of the limitations of different contexts and that you can't trap focus in one window to another window. That's not what I'm referring to.

To be perfectly clear when I say it's "not working" I mean there are no observable effects other than the keyboard focus being fully capable of leaving the focus trap in the document in which it's active. No console errors, nothing like that, it just... doesn't work. I'm aware of how annoying it can be to receive a bug report like this but when I say doesn't work, I quite literally mean that I have no other information than that.

Also, just to check everything, you should be using focus-trap-react v8.8.0 or greater. That's the version that added support for the focus-trap document option.

Just installed it a couple days ago. Using version 8.8.2.

[stefcameron]

Right, not much to go on then... Are you able to debug into focus-trap-react.js to try and see where the problem is? If your node_modules tree isn't read-only, you could at least add console statements in focus-trap-react.js and then see what parts of the code get executed at runtime and which don't. That might shed some light on things.

[LoganDark]

Right, not much to go on then... Are you able to debug into focus-trap-react.js to try and see where the problem is? If your node_modules tree isn't read-only, you could at least add console statements in focus-trap-react.js and then see what parts of the code get executed at runtime and which don't. That might shed some light on things.

The Chrome debugger has code coverage so I can just use that. (It shows timings on the left for lines that got executed)

Could be a couple days though, I'll keep you posted. In the meantime I can provide a codesandbox that reproduces the issue?

[stefcameron]

Take your time. A repro always helps, for sure!

[LoganDark]

https://codesandbox.io/s/focus-trap-react-other-document-6mvjj

[LoganDark]

Huh. Actually it seems specifying document as a focusTrapOption now fixes it in the codesandbox. Weird.

[stefcameron]

I can't repro in either the native window or the popup one:

[LoganDark]

I can't repro in either the native window or the popup one:

Yeah, that's because specifying document seems to have fixed it in the codesandbox for whatever reason. By any chance did your commit make it to npm already?

I've taken my application into the debugger and verified that active is true and the document is correct and that the focus trap is activated via this.focusTrap.activate().

[LoganDark]

Wrong document here.

The issue seems to be that the focus trap is created before active is first set to true, so createFocusTrap is latching onto the wrong document (because document is still undefined, because it takes a state update to stabilize). Then doc gets stored forever and never gets corrected.

The reason the CodeSandbox demo works is because the document variable has already been initialized when the focus trap first gets switched on.

The solution would be to only call setupFocusTrap once the active prop is seen to be true at least once.

[stefcameron]

So something like componentDidMount(), only call this.setupFocusTrap() if this.props.active is true, and then in componentDidUpdate(), check if (!prevProps.active && this.props.active) and call this.setupFocusTrap() (if it doesn't already exist).

[stefcameron]

And there's no way you can change your code to delay the inclusion of the focus trap component to when the document has stabilized? Like don't render the trap until you have the document you need...

[stefcameron]

Because this is kind of a big change to the way it's currently working.

[LoganDark]

And there's no way you can change your code to delay the inclusion of the focus trap component to when the document has stabilized? Like don't render the trap until you have the document you need...

This... is impossible? Like, my component doesn't know where it's mounted until it's been mounted.

Because this is kind of a big change to the way it's currently working.

It's not. It's highly unlikely people are relying on when the document field is read from the options. If anything this fixes a bug, where the focus trap reads the options before they are ready.

[stefcameron]

And there's no way you can change your code to delay the inclusion of the focus trap component to when the document has stabilized? Like don't render the trap until you have the document you need...

This... is impossible? Like, my component doesn't know where it's mounted until it's been mounted.

That's true. Then rendering with <FocusTrap> could cause the div to change, causing the ref to change, and I guess it could be a different document at that point, though that seems unlikely. Still, it wouldn't be totally "clean" to assume it's the same document.

Because this is kind of a big change to the way it's currently working.

It's not. It's highly unlikely people are relying on when the document field is read from the options. If anything this fixes a bug, where the focus trap reads the options before they are ready.

I'll give this some thought. It's a significant change in terms of when the trap gets created. I'll have to check focus-trap to see if creating the trap triggers any events for which you might expect a callback in one of the options, in which case, you wouldn't get the callback at the same time anymore.

If that turns out to be a problem, then another option might be to promote the document option to a prop (instead of nesting it in focusTrapOptions where there's no "notification" if it changes) and then re-create the trap if that prop changes. Then we could maintain existing behavior and only have changes affect changing the document.

[LoganDark]

If that turns out to be a problem, then another option might be to promote the document option to a prop (instead of nesting it in focusTrapOptions where there's no "notification" if it changes) and then re-create the trap if that prop changes. Then we could maintain existing behavior and only have changes affect changing the document.

Either that or make it so that I don't have to specify the document field at all. You have access to the DOM node, so just get it from there maybe? The document field should only be necessary if I'm doing something exotic.

[stefcameron]

@LoganDark Can you check if #546 will fix your issue?

[stefcameron]

Either that or make it so that I don't have to specify the document field at all. You have access to the DOM node, so just get it from there maybe? The document field should only be necessary if I'm doing something exotic.

I did consider this, but if you give the trap more than one container, which doc do we use (I guess they should all be from the same document, but still), and more, if the container elements are updated while the trap is active, we'd have to check for a change in the doc again and then remove/re-add all the listeners... Just seems like too risky when there seems to be a simpler solution, which I hope I have in #546.

[LoganDark]

@LoganDark Can you check if #546 will fix your issue?

Definitely seems like it.

[stefcameron]

OK, I'll merge it and publish it. Hopefully it addresses your issue.

[stefcameron]

It'll be published imminently in 8.9.0.

[stefcameron]

@all-contributors add @LoganDark for bug

[allcontributors]

@stefcameron

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