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

dialog: focus-trap blocks interactive elements rendered using portal #83

Closed
kirillku opened this issue Oct 15, 2018 · 13 comments
Closed
Labels
Status: Unconfirmed Bug reports without a repro, not yet confirmed

Comments

@kirillku
Copy link

Hi, thanks for the Dialog. It works great but I face one issue. You are using focus-trap internally to block events outside the Dialog, which is nice and I understand the logic behind this. I am rendering some interactive elements inside the Dialog and some of them are also using portals for rendering (for example popups), which results them being not a child node of the Dialog and this why not active. They are visible, but not clickable. Can you suggest some solution or workaround? I didn't come up with anything. Maybe it's possible to only block click events on the elements, that are above the Dialog in the DOM tree.

@everdimension
Copy link

Interesting case.

I think the right solution is to make your portal elements render inside the dialog component. It actually makes sense.

The reason for rendering tooltips and popups as a direct child of body is usually either dealing with css specificity or z-index wars. And since you have a modal dialog open, you're already rendering your elements outside the main app root. There should be no need to render popups in a parallel branch.

@kirillku
Copy link
Author

Yep, this is what I would do with my own components to solve the issue, but what about third party ones. For example react-semantic-ui Popup.

Also, the reasoning behind putting something in a portal can be not only z-index, but also overflow positioning.

@everdimension
Copy link

Yep, this is what I would do with my own components to solve the issue, but what about third party ones. For example react-semantic-ui Popup.

I had similar problems with 3rd party libraries and I've came to the conclusion that those libs that do not let you configure the root container they attach to are not very good libraries :) And it is a good idea to open an issue in their repository.

I understand that this is not the answer you were hoping for, but it seems to me to be the "right" way to deal with this.


Also I wonder whether the @reach/dialog allows you to specify a root other than the body 🤔
Seems to me that it doesn't https://github.com/reach/reach-ui/blob/master/packages/portal/src/index.js#L10 🙁

@kirillku
Copy link
Author

What about making disableFocusTrap prop for such cases. I think there is a lot 3rd party libs, that only attach portals as direct children of body. I will do a mr for that.

@navaru
Copy link

navaru commented Oct 24, 2018

This might be relevant, checkout this article: It’s a (focus) Trap!

focus-trap-react (focus-trap) is emulation, but react-focus-trap (a different one!) is not.
...
... and emulation is the worst thing you can do.

@giuseppeg
Copy link

giuseppeg commented Nov 2, 2018

the reasoning behind putting something in a portal can be not only z-index, but also overflow positioning

This is a super valid point. In an article I wrote how we manager layers and focus trap at work.

Basically we activate the focus trap declaratively and the modal module exports a function that we can invoke imperatively to temporarily deactivate the trap for the element that isOnTop. So for example if we have a dropdown menu inside of the Modal we can deactivate the trap when those are opened and move focus to the dropdown menu. On close we reactivate the trap and restore focus inside of the modal.

@theKashey
Copy link
Contributor

react-focus-lock could be a solution here - it uses React event propagation, not DOM, thus focus events, as long as they would bubble from portals, are visible to lock, and are considered as their own.
But first we have to remove focus-trap from reach/dialog.

@ryanflorence
Copy link
Member

ah! this is likely the problem with #64, would love somebody to explore using react-focus-lock and send a PR using it?

@theKashey
Copy link
Contributor

Probably that should be on me.

@Miklet
Copy link
Contributor

Miklet commented Jan 9, 2019

We are facing currently same issue with focus-trap. Wouldn't be better easier to add additional prop on Dialog which will let us configure focus-trap when initializing it with createFocusTrap? But for future it is actually better to use react-focus-lock since it is using React's event mechanism.

@chaance
Copy link
Member

chaance commented Oct 2, 2019

I believe this issue has been resolved but simply not closed, but if folks are still having trouble here please flag me and I'll happily reopen and investigate further.

@chaance chaance closed this as completed Oct 2, 2019
@theKashey
Copy link
Contributor

So it works if you can "click" on portaled elements, and that would properly bubble from react tree, and managed, but in terms of "focus trapping" - there is no way you can "tab" into portaled content.
It's not a big deal to discover them, but probably Flare would be a more safe way to handle it. So - let's wait a bit.

@PaulRBerg
Copy link

PaulRBerg commented Jul 4, 2021

I believe this issue has been resolved but simply not closed, but if folks are still having trouble here please flag me and I'll happily reopen and investigate further.

No, this isn't solved yet. I've encountered the same focus trap issue in v0.15.2. If I assign display: none to the Reach Dialog tag in Chrome, I can interact with the input. Otherwise, I can't.

I'm loading a third-party widget that appends a div child directly to body; the third-party widget doesn't let me choose the root container.

Update: @chaance's solution proposed in this comment, which is to set the unstable_lockFocusAcrossFrames prop to false, did not work.

Update 2: setting dangerouslyBypassFocusLock worked 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed Bug reports without a repro, not yet confirmed
Projects
None yet
Development

No branches or pull requests

9 participants