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

Nested portals should be discoverable #14540

Open
theKashey opened this issue Jan 7, 2019 · 12 comments
Open

Nested portals should be discoverable #14540

theKashey opened this issue Jan 7, 2019 · 12 comments

Comments

@theKashey
Copy link
Contributor

theKashey commented Jan 7, 2019

This is more about a bridge between actual DOM Tree and React Tree.

Do you want to request a feature or report a bug?
feature

What is the current behavior?
You can portal a part of your rendering tree to another place in Dom Tree, and React would handle events, including events on Capture Phase like there were no portals - events could dive through all the react parents, and bubble up through all the react parents.

This is quite useful, as long as portal is an implementation detail, but useful only for normal events; there are more cases around it.

What is the expected behavior?

It's better to explain it by example

  • you have a Modal Dialog and it uses a Focus Lock, ie focus could not leave it.
  • inside Modal you have a Custom Select, with Dropdown menu rendered via a portal.
  • you could not use it, as long as from DOM prospective ModalNode.contains(DropDownNode) is always false, and Focus Lock will prevent focusing.

It's a real issue - reach/reach-ui#83, theKashey/react-focus-lock#19.

Proposed solution:

  • containsNode(domNode):boolean - React-aware version of DOM API node.contains(anotherNode).
  • getHostNodes():Nodes[] - returns a list of all root nodes inside "current component" including direct children and portals. Similar to ReactDom.findDomNode, and (proposed)refs attached to React.Fragment. It just finds all nodes "you are consists of". As a result you will be able to tab from one piece of you to another, making focus management independed of implementation details.

Cons:

  • requires Component to access fiber, DOM node to access fiber thought node, or an new hook to do it in a functional way.
  • does twice dreadfull things than deprecated findDomNode
  • usage scope is very narrow.

Pros:

  • my use case requires momentum access to a rendered tree, and does not suffer async stuff as findDomNode, where underlaying node might not be yet created. Stuff like "does something containsNode right now", or "getHostNodes I consist from right now" are sync, and the question asked about actual DOM tree structure.

Example using react-dom-reflection, which implements required API - https://codesandbox.io/s/1or60v506l

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Never worked

@stale
Copy link

stale bot commented Jan 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@theKashey
Copy link
Contributor Author

With the React.Flare deprecation this issue is still very valuable.

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 12, 2020
@stale
Copy link

stale bot commented Apr 11, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Apr 11, 2020
@diegohaz
Copy link

Bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 11, 2020
@stale
Copy link

stale bot commented Jul 11, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jul 11, 2020
@theKashey
Copy link
Contributor Author

theKashey commented Jul 11, 2020

Sorry StaleBot, but we need some solution.

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 11, 2020
@stale
Copy link

stale bot commented Oct 12, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Oct 12, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Dec 25, 2020
@kalda341
Copy link

kalda341 commented May 1, 2022

Bump. It would be great to have a solution to this.

@eps1lon
Copy link
Collaborator

eps1lon commented May 2, 2022

I think this is still relevant so re-opening.

I've seen the recommendation to use Node.contains a couple of times and this recommendation is problematic since in the DOM we (always?) have the guarantee that a bubbled event means that the event.currentTarget.contains(event.target). But this guarantee is definitely broken if we have a ReactDOM.createPortal in between.

Would be interesting to check if event.currentTarget.contains(event.target) is true for all events (e.g. what happens with submit FormEvent in <input type="submit" form="my-form"><form id="my-form" onSubmit={handleSubmit} />).

Since we don't have the tools to introspect the React tree (and I think it was made clear that we never will have), we currently don't truly now if the currentTarget of a React event contains the target of a React event. This is relevant for focus-trapping but I think so far we've been able to fix it in userland?

So it would be nice to create a comprehensive list of Node.contains use cases, how ReactDOM.createPortal breaks this, and if and how these work around ReactDOM.createPortal existence. Ideally with an eye on how DOM related "portaling" APIs affect these scenarios (e.g. form, aria-owns and others if there are some)

@eps1lon eps1lon reopened this May 2, 2022
@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label May 2, 2022
@eseakin
Copy link

eseakin commented Dec 14, 2022

Bump. We have the same issue and it's really annoying.

Nested portals are a pretty reasonable thing, and there is no way to tell if an event originated from a nested child portal.

For example, a modal (portal) with a dropdown (portal) inside it. If I click outside the modal, both should close. If I click the inner dropdown, the outer modal should stay open. However, the outer modal thinks the inner click is an external dom node, so it closes.

We are solving this by adding a Context wrapper around our portals which passes down a setter that each child portal can call in order to save its node in the parent. It works, but this feels really silly since React KNOWS that these are parent/child in the virtual dom. Being locked out of the virtual dom's knowledge feels really bad here.

Edit: And before anyone says "don't use portals," please trust that we have good reasons for this and portals are the best solution for our use case.

@theKashey
Copy link
Contributor Author

I also ended up managing portals from the user space via ContextApi/subscriptions, but while such solution works perfectly - it works perfectly only for me and cannot be shared with some third-party library as it is "not standard".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants
@theKashey @kalda341 @gaearon @diegohaz @eps1lon @eseakin and others