facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.04k stars 46.53k forks source link

createPortal: support option to stop propagation of events in React tree #11387

Open kib357 opened 6 years ago

kib357 commented 6 years ago

Do you want to request a feature or report a bug? Feature, but also a bug cause new API breaks old unstable_rendersubtreeintocontainer

What is the current behavior? We cannot stop all events propagation from portal to its React tree ancestors. Our layers mechanism with modals/popovers completely broken. For example, we have a dropdown button. When we click on it, click opens popover. We also want to close this popover when clicking on same button. With createPortal, click inside popover fires click on button, and it's closing. We can use stopPropagation in this simple case. But we have tons of such cases, and we need use stopPropagation for all of them. Also, we cannot stop all events.

What is the expected behavior? createPortal should have an option to stop synthetic events propagation through React tree without manually stopping every event. What do you think?

kib357 commented 6 years ago

Also, propagation of mouseOver/Leave looks completely unexpected. image

gaearon commented 6 years ago

Can you move portal outside of the button?

e.g.

return [
  <div key="main">
    <p>Hello! This is first step.</p>
    <Button key="button" />
  </div>,
  <Portal key="portal" />
];

Then it won't bubble through the button.

kib357 commented 6 years ago

It was my first thought, but!) Imagine, that we have mouseEnter handler in such component container:

image

With unstable_rendersubtreeintocontainer i need nothing to do with events in ButtonWithPopover component – mouseEnter simply works when mouse really enters div and button DOM element, and not fired when mouse is over popover. With portal, event fires when mouse over popover – and actually NOT over div in this moment. So, i need to stop propagation. If i do it in ButtonWithPopover component, i will break event firing when mouse is over button. If i do it in popover and i'm using some common popover component for this application, i also can break logic in other app parts.

I really don't understand purpose of bubbling through React tree. If i need events from portal component – i simply can pass handlers through props. We were do it with unstable_rendersubtreeintocontainer and it worked perfect.

If i will open a modal window from some button deep in react tree, i will receive unexpected firing of events under modal. stopPropagation will also stop propagation in DOM, and i will not get events that i really expect to be fired(

craigkovatch commented 6 years ago

@gaearon I would suggest this is more of a bug than a feature request. We have a number of new bugs caused by mouse events bubbling up through portals (where we were previously using unstable_rendersubtreeintocontainer). Some of these can't be fixed even with an extra div layer to filter mouse events because e.g. we rely on mousemove events propagating up to the document to implement draggable dialogs.

Is there a way to workaround this before this is addressed in a future release?

jquense commented 6 years ago

I think it's being called a feature request, because the current bubble behavior of portals is both expected and intended. The goal is that subtree act like real child of their parents.

What would be helpful is additional use cases or situations (like the ones you're seeing) that you don't feel are served by the current implementation, or are difficult to workaround

craigkovatch commented 6 years ago

I understand that this behavior is intended, but I think it's a significant bug that it's not disable-able.

neytema commented 6 years ago

In my mind library working with DOM should preserve DOM implementation behavior not break it.

For example:

class Container extends React.Component {
  shouldComponentUpdate = () => false;
  render = () => (
    <div
      ref={this.props.containerRef}
      // Event propagation on this element not working
      onMouseEnter={() => { console.log('handle mouse enter'); }}
      onClick={() => { console.log('handle click'); }}
    />
  )
}

class Root extends React.PureComponent {
  state = { container: null };
  handleContainer = (container) => { this.setState({ container }); }

  render = () => (
    <div>
      <div
        // Event propagation on this element not working also
        onMouseEnter={() => { console.log('handle mouse enter'); }}
        onClick={() => { console.log('handle click'); }}
      >
        <Container containerRef={this.handleContainer} />
      </div>
      {this.state.container && ReactDOM.createPortal(
        <div>Portal</div>,
        this.state.container
      )}
    </div>
  );
}

When I work with DOM, I expect to receive events like DOM implementation does it. In my example events are propagated through Portal, working around it's DOM parents, and this can be considered as a bug.

jquense commented 6 years ago

Folks thanks for the discussion, however I don't think it's all that helpful to argue whether something is a bug or not. Instead i'd be more productive to discuss the use cases and examples that are not met by the current behavior, so we can better understand if the current way is the best way for the future.

In general we want the API to handle a diverse set of use-cases while hopefully not overly limiting others. I can't speak for the core team, but I'd imagine that making it configurable is not a likely solution. Generally React leans for a consistent api over configurable ones.

I also understand that this behavior is not how the DOM works, but i don't think that's in itself a good reason to say it shouldn't be that way. Lots of react-dom's behavior is different from how the DOM works, may events are already different from the native version. onChange for instance is completely unlike the native change event, and all react events bubble regardless of type, unlike the DOM.

craigkovatch commented 6 years ago

Instead i'd be more productive to discuss the use cases and examples that are not met by the current behavior

Here's two examples that are broken for us in our migration to React 16.

First, we have a draggable dialog which is launched by a button. I attempted to add a "filtering" element on our Portal use which called StopPropagation on any mouse an key events. However, we rely on being able to bind a mousemove event to the document in order to implement the dragging functionality -- this is common because if the user moves the mouse at any significant rate, the cursor leaves the bounds of the dialog and you need to be able to capture the mouse movement at a higher level. Filtering these events breaks this functionality. But with Portals, the mouse and key events are bubbling up from inside the dialog to the button that launched it, causing it to display different visual effects and even dismiss the dialog. I don't think it's realistic to expect every component that will be launched via a Portal to bind 10-20 event handlers to stop this event propagation.

Second, we have a popup context menu which can be launched by either a primary- or secondary-mouse click. One of the internal consumers of our library has mouse handlers attached to the element that launches this menu, and of course the menu also has click handlers for handling item selection. The menu is now reappearing on every click as the mousedown/mousedown events are bubbling back up to the button that launches the menu.

I can't speak for the core team, but I'd imagine that making it configurable is not a likely solution. Generally React leans for a consistent api over configurable ones.

I implore you (and the team) to reconsider this position in this particular case. I think event bubbling will be interesting for certain use cases (although I can't think of any offhand). But I think it will be crippling in others, and it introduces significant inconsistency in the API. While unstable_rendersubtreeintocontainer was never super-supported, it was what everyone used to render outside of the immediate tree, and it didn't work this way. It was officially deprecated in favor of Portals, but Portals break the functionality in this critical way, and there doesn't seem to be an easy workaround. I think this can be fairly described as quite inconsistent.

I also understand that this behavior is not how the DOM works, but i don't think that's in itself a good reason to say it shouldn't be that way.

I understand where you're coming from here, but I think in this case (a) it's a fundamental behavior which (b) currently has no workaround, so I think "the DOM doesn't work this way" is a strong argument, if not a completely convincing one.

craigkovatch commented 6 years ago

And to be clear: my request that this be considered a bug is mostly so that it gets prioritized for a fix sooner rather than later.

sebmarkbage commented 6 years ago

My mental model of a Portal is that it behaves as if it is on the same place in the tree, but avoids problems such as "overflow: hidden" and avoids scrolling for drawing/layout purposes.

There are many similar "popup" solutions that happen inline without a Portal. E.g. a button that expands a box right next to it.

Take as an example the "Pick your reaction" dialog here on GitHub. That is implemented as a div right next to the button. That works fine now. However, if it wants to have a different z-index, or be lifted out of an overflow: scroll area that contains these comments, then it will need to change DOM position. That change is not safe to do unless other things like event bubbling is also preserved.

Both styles of "popups" or "pop outs" are legit. So how would you solve the same problem when the component is inline in the layout as opposed to floating outside of it?

methyl commented 6 years ago

The workaround that worked for me is calling stopPropagation directly under portal rendering:

return createPortal(
      <div onClick={e => e.stopPropagation()}>{this.props.children}</div>,
      this.el
    )

That works great for me since I have single abstraction component that uses portals, otherwise you will need to fix up all your createPortal calls.

craigkovatch commented 6 years ago

@methyl this assumes you know every event that you need to block from bubbling up the tree. And in the case I mentioned with draggable dialogs, we need mousemove to bubble up to document, but not to bubble up the render tree.

craigkovatch commented 6 years ago

Both styles of "popups" or "pop outs" are legit. So how would you solve the same problem when the component is inline in the layout as opposed to floating outside of it?

@sebmarkbage I'm not sure this question makes sense. If I had this problem inlining the component, I wouldn't inline it.

jquense commented 6 years ago

I think some of problem here is the some use cases of renderSubtreeIntoContainer are being ported to createPortal when the two methods are doing conceptually different things. The concept of Portal was being overloaded I think.

I agree that in the Modal dialog case, you almost never want the modal to act like a child of the button that opened it. The trigger component is only rendering it because it controls the open state. I think tho it's a mistake to say that the portal implementation is therefore wrong, instead of saying that createPortal in the button is not the right tool for this. In this case the Modal is not a child of the trigger, and shouldn't be rendered as if it were. One possible solution is to keep using renderSubtreeIntoContainer, another user-land option is to have a ModalProvider near the app root that handles rendering modals, and passes down (via context) a method to render an arbitrary modal element need to the root

craigkovatch commented 6 years ago

renderSubtreeIntoContainer can't be called from inside of render or lifecycle methods in React 16, which pretty much precludes its use for the cases I've been discussing (in fact, all our components which were doing this completely broke in the migration to 16). Portals are the official recommendation: https://reactjs.org/blog/2017/09/26/react-v16.0.html#breaking-changes

I agree that the concept of Portals might have ended up overloaded. Not sure I love the solution of a global component and context for it, though. It seems like this could be easily solved by a flag on createPortal specifying whether events should bubble through. It would be an opt-in flag which would preserve API compatibility with 16+.

kib357 commented 6 years ago

I will try to clarify our use-case of the portals and why we would love to see an option for stopping events propagation. In ManyChat app, we are using portals to create a 'layers'. We have the layer system for the whole app that used by several types of components: popovers, dropdowns, menus, modals. Every layer can expose a new layer, e.g. button on a second level of menu can trigger the modal window where the other button can open the popover. In most cases layer is the new branch of UX that solves it's own task. And when new layer is open, the user should interact with this new layer, not with others in bottom. So, for this system, we've created a common component for rendering to layer:

class RenderToLayer extends Component {
  ...
  stop = e => e.stopPropagation()

  render() {
    const { open, layerClassName, useLayerForClickAway, render: renderLayer } = this.props

    if (!open) { return null }

    return createPortal(
      <div
        ref={this.handleLayer}
        style={useLayerForClickAway ? clickAwayStyle : null}
        className={layerClassName}
        onClick={this.stop}
        onContextMenu={this.stop}
        onDoubleClick={this.stop}
        onDrag={this.stop}
        onDragEnd={this.stop}
        onDragEnter={this.stop}
        onDragExit={this.stop}
        onDragLeave={this.stop}
        onDragOver={this.stop}
        onDragStart={this.stop}
        onDrop={this.stop}
        onMouseDown={this.stop}
        onMouseEnter={this.stop}
        onMouseLeave={this.stop}
        onMouseMove={this.stop}
        onMouseOver={this.stop}
        onMouseOut={this.stop}
        onMouseUp={this.stop}

        onKeyDown={this.stop}
        onKeyPress={this.stop}
        onKeyUp={this.stop}

        onFocus={this.stop}
        onBlur={this.stop}

        onChange={this.stop}
        onInput={this.stop}
        onInvalid={this.stop}
        onSubmit={this.stop}
      >
        {renderLayer()}
      </div>, document.body)
  }
  ...
}

This component stops propagation for all event types from React docs, and it allowed us to update to React 16.

jacobp100 commented 6 years ago

Does this need to be tied to portals? Rather than sandboxing portals, what if there was just a (for example) <React.Sandbox>...</React.Sandbox>?

craigkovatch commented 6 years ago

Even that seems needlessly complex to me. Why not simply add an optional boolean flag to createPortal allowing the bubbling behavior to be blocked?

craigkovatch commented 6 years ago

@gaearon this is a pretty unfortunate situation for a certain slice of us -- could you or someone dear to you have a look at this? :)

jquense commented 6 years ago

I'd add that my current thinking is that both use cases should be supported. There are really use cases where you need context to flow from the current parent to the subtree but to not have that subtree act as a logical child in terms of the DOM. Complex modals are the best example, you just almost never want the events from a form in a modal window to propagate up to the trigger button, but almost certainly need the context passed through (i18n, themes, etc)

I will say that that usecase could be mostly solved with a ModalProvider closer to the app root that renders via createPortal high enough that event propagation doesn't affect anything, but that starts to feel like a workaround as opposed to a well designed architecture. It also makes library provided modals more annoying for users since they are no longer self contained.

jquense commented 6 years ago

i would add tho in terms of API i don't think createPortal should do both, the modal case really wants to just use ReactDOM.render (old skool) because it's pretty close to a distinct tree except that context propagation is often needed

craigkovatch commented 6 years ago

We just had to fix an extremely difficult-to-diagnose bug in our outer application's focus management code as a result of using the workaround that @kib357 posted.

Specifically: calling stopPropagation on the synthetic focus event to prevent it from bubbling out of the portal causes stopPropagation to also be called on the native focus event in React's captured handler on #document, which meant it did not make it to another captured handler on <body>. We fixed by moving our handler up to #document, but we had specifically avoided doing that in the past so as not to step on React's toes.

The new bubbling behavior in Portals really feels like the minority case to me. Be that opinion or truth, could we please get some traction on this issue? Maybe @gaearon? It's four months old and causing real pain. I think this could fairly be described as a bug given that it's a breaking API change in React 16 with no completely-safe workaround.

sebmarkbage commented 6 years ago

@craigkovatch I'm still curious how you would solve my inline example. Let's say the popup is pushing the size of the box down. Inlining something is important since it is pushing something down in the layout given its size. It can't just hover over.

You could potentially measure the popover, insert a blank placeholder with the same size and try to align it on top but that's not what people do.

So if your popover need to expand the content in place, like right next to the button, how would you solve it? I suspect that the pattern that works there, will work in both cases and we should just recommend the one pattern.

sebmarkbage commented 6 years ago

I think in general this is the pattern that works in both scenarios:

class Foo extends React.Component {
  state = {
    highlight: false,
    showFlyout: false,
  };

  mouseEnter() {
    this.setState({ highlight: true });
  }

  mouseLeave() {
    this.setState({ highlight: false });
  }

  showFlyout() {
    this.setState({ showFlyout: true });
  }

  hideFlyout() {
    this.setState({ showFlyout: false });
  }

  render() {
    return <>
      <div onMouseEnter={this.mouseEnter} onMouseLeave={this.mouseLeave} className={this.state.highlight ? 'highlight' : null}>
        Hello
        <Button onClick={this.showFlyout} />
      </div>
      {this.state.showFlyout ? <Flyout onHide={this.hideFlyout} /> : null}
    </>;
  }
}

If the Flyout is a portal, then it works and it doesn't get any mouse over events when hovering over the portal. But more importantly, it also works if it is NOT a portal, and it needs to be an inline flyout. No stopPropagation needed.

So what is it about this pattern that doesn't work for your use case?

craigkovatch commented 6 years ago

@sebmarkbage we are using Portals in a completely different fashion, rendering into a container mounted as the final child of <body> which is then positioned, sometimes with a z-index. The React documentation suggests this is closer to the design intention; i.e. rendering into a totally different place in the DOM. It doesn't seem to me that our use cases are similar enough for discussion to belong on this thread. But if you want to brainstorm/troubleshoot together, I'd be more than happy to discuss further in another forum.

sebmarkbage commented 6 years ago

No my use case is both. Sometimes one and sometimes the other. That's why it is relevant.

The <Flyout /> can choose to render into the final child of body or not but as long as you hoist the portal itself out to a sibling of the hovered component rather than a child of it, your scenario works.

sebmarkbage commented 6 years ago

I think there's a plausible scenario where that is inconvenient and you want a way to teleport things from deeply nested components but in that scenario you're probably fine with the context being the context from the intermediate point. But I think of those as two separate issues.

Maybe we need a slots API for that.

class Foo extends React.Component {
  state = {
    showFlyout: false,
  };

  showFlyout() {
    this.setState({ showFlyout: true });
  }

  hideFlyout() {
    this.setState({ showFlyout: false });
  }

  render() {
    return <>
      Hello
      <Button onClick={this.showFlyout} />
      <SlotContent name="flyout">
        {this.state.showFlyout ? <Flyout onHide={this.hideFlyout} /> : null}
      </SlotContent>
    </>;
  }
}

class Bar extends React.Component {
  state = {
    highlight: false,
  };

  mouseEnter() {
    this.setState({ highlight: true });
  }

  mouseLeave() {
    this.setState({ highlight: false });
  }

  render() {
    return <>
      <div onMouseEnter={this.mouseEnter} onMouseLeave={this.mouseLeave} className={this.state.highlight ? 'highlight' : null}>
        <SomeContext>
          <DeepComponent />
        </SomeContext>
      </div>
      <Slot name="flyout" />
    </>;
  }
}

The portal would then get the context of Bar, not DeepComponent. Context and event bubbling then still share the same tree path.

jquense commented 6 years ago

@sebmarkbage the modal case usually does require context from the point it's rendered. It's slightly unique of a case I think, the component is a logical child of the thing that rendered it but not a structural one (for lack of a better word), e.g. You usually want things like form context (relay, formik, redux form, whatever) but not DOM events to pass through. One also ends up rendering such modals fairly deep in trees, next to their triggers, so they stay componenty and reusable, more so than because they belong there structurally

I think this case is generally different to the flyout/dropdown case createPortal serves. Tbc I think the bubbling behavior of portals is good, but not for modals. I also think this could handled with Context and some sort of ModalProvider reasonably well, but that's kinda annoying especially for libraries.

craigkovatch commented 6 years ago

as long as you hoist the portal itself out to a sibling of the hovered component rather than a child of it, your scenario works.

Not sure I follow. There's still the problem of e.g. keyDown events bubbling through an unexpected DOM tree.

sebmarkbage commented 6 years ago

@jquense Note that in my example the slot is still within the Bar component so it would get its context from the form in something like <Form><Bar /></Form>.

Even if the portal rendered into the document body.

So it’s like two indirections (portalings): deep -> sibling of Bar -> document body.

So the context of the portal is still the context of the form, and so is the event bubbling chain, but neither is in the context of the hovered thing.

jquense commented 6 years ago

Yes sorry missed that 😳 If I'm reading it right tho, you'd still have the bubbling at <Slot> up though? That's definitely better, though I do think that in the Modal dialog case one probably doesn't want any bubbling. Like thinking in terms of a screen reader, you want everything outside the modal to be invert, while it's up. I don't know, I think for that case the bubbling is a gotcha, no one would expect a click inside a dialog to bubble through anywhere.

jquense commented 6 years ago

Maybe the problem here isn't portals, but there isn't a good way to share Context across trees? A part from the context thing ReactDOM.render is really fine for modals, and maybe more "correct" way of thinking about it anyway...

sebmarkbage commented 6 years ago

My thinking here is that there is some bubbling because it still goes from the modal to the div to the body to the document to the window. And conceptually beyond the frame out to the containing window and so on.

That's not theoretical in something like ART or GL rendered content (and to some extent React Native) where there might not be an existing backing tree to get those semantics from. So there needs to be a way to say that this is where it bubbles.

In some apps there are modals in modals. E.g. at FB there is chat window that might be above a modal or a modal might be part of the chat window. So even a modal has some context as to where in the tree it belongs. It is never completely standalone.

That's not to say we can't have two different semantics for event bubbling and context. That are both explicit about this and you can portal one without the other etc.

Having the guarantee that they both follow the same path is really powerful though since it means that event bubbling can be fully implemented for user space events the same as in the browser.

For example this happens with various Redux contexts today. Imagine this.context.dispatch("Hover") is a user space event bubbling. We could even implement React events as part of context. It seams reasonable to think that I can use this the same way, and in every way right now, you can. I think if we did fork these two contexts, we'd probably end up with another user space context API that follows the DOM structure in parallel with normal context - if they are truly so different.

So that's kind of why I'm pushing against it a bit to see if the slots thing might be sufficient, since a) you need to be explicit about which context bubbling happens anyway. b) it can avoid forking the world and having two entire context systems.

kib357 commented 6 years ago

Specifically: calling stopPropagation on the synthetic focus event to prevent it from bubbling out of the portal causes stopPropagation to also be called on the native focus event in React's captured handler on #document, which meant it did not make it to another captured handler on . We fixed by moving our handler up to #document, but we had specifically avoided doing that in the past so as not to step on React's toes.

@craigkovatch , did you used onFocusCapture event on document? In my workaround captured events shouldn't be stopped. Can you provide more detailed example of how it was and what you have done to resolve your issue? Also, i think my code has an issue with stopping blur event – it shouldn't be stopped. So, i'll investigate this question deeper and will try to found a more reliable solution.

craigkovatch commented 6 years ago

@kib357 I'm not suggesting there's an issue in your workaround, I think there's a separate bug in React there (i.e. shouldn't cancel propagation of native focus events in capture phase when calling stopPropagation on synthetic focus events in bubbling phase).

The code in question uses a native capture event listener, i.e. document.body.addEventListener('focus', handler, true)

kib357 commented 6 years ago

@craigkovatch sounds interesting, given the fact that you used captured handler. However, i have not any thoughts why this happens.

So, guys, we have a two different scenarios for using portal rendering:

  1. To prevent CSS issues like overflow:hidden and etc in simple widgets, like a dropdown buttons or one-level menus
  2. To create a new UX layer for more powerful cases like:
    • modals
    • nested menus
    • popovers-with-forms-with-dropdowns-... – all cases, when layers are combined

I think current createPortal API satisfies only the first scenario. Suggestion to use a new React.render for second is unusable – it's very poor to create a separate app with all it providers for every layer. What additional info we can provide to help resolve this issue? What disadvantages of suggested param in the createPortal API?

webdesserts commented 6 years ago

@sebmarkbage My immediate question with the slots API is: would I be able to insert multiple SlotContents into one Slot at the same time? It's not uncommon in our interface to have multiple "popups" or "modals" open simultaneously. In my perfect world a Popup API would look something like this:

import { App } from './app'
import { PopupSlot } from './popups'

let root = (
  <div>
    <App />
    <PopupSlot />
  </div>
)

ReactDOM.render(root, document.querySelector('#root'))
// some dark corner of our app

import { Popup } from './popups'

export function SoManyPopups () {
  return <>
    <Popup>My Entire</Popup>
    <Popup>Interface</Popup>
    <Popup>Is Popups</Popup>
  </>
}
craigkovatch commented 6 years ago

We have a new issue with this that I have been completely unable to find a workaround for. Using the "event trap" approach suggested above, only React Synthetic events are blocked from bubbling out of the portal. The native events still bubble, and since our React code is hosted inside of a mostly-jQuery application, the global jQuery keyDown handler on <body> still gets the event.

I attempted to add an event.stopPropagation listener to the native container element inside the Portal via a ref like this, but this completely neuters all Synthetic events within the portal -- I incorrectly assumed React's top-level listener was watching capture phase.

Not sure what can possibly be done here, other than changes to React.

const allTheEvents: string[] = 'click contextmenu doubleclick drag dragend dragenter dragexit dragleave dragover dragstart drop mousedown mouseenter mouseleave mousemove mouseover mouseout mouseup keydown keypress keyup focus blur change input invalid submit'.split(' ');
const stop = (e: React.SyntheticEvent<HTMLElement>): void => { e.stopPropagation(); };
const nativeStop = (e: Event): void => e.stopPropagation();
const handleRef = (ref: HTMLDivElement | null): void => {
  if (!ref) { return; }
  allTheEvents.forEach(eventName => ref.addEventListener(eventName, nativeStop));
};

/** Prevents https://reactjs.org/docs/portals.html#event-bubbling-through-portals */
export function PortalEventTrap(children: React.ReactNode): JSX.Element {
  return <div
      onClick={stop}
      ...

      ref={handleRef}
    >
      {children}
    </div>;
}
Jessidhia commented 6 years ago

That depends on the order that ReactDOM and JQuery are initialized. If JQuery initializes first, JQuery's top level event handlers will be installed first, and so they will run before ReactDOM's synthetic handlers get to run.

Both ReactDOM and JQuery prefer to only have a single top level listener that then simulates bubbling internally, unless there's some event that the browser won't bubble such as scroll.

craigkovatch commented 6 years ago

@Kovensky my understanding was that jQuery did not do "synthetic bubbling" the way React does, and thus does not have a single top-level listener. My DOM inspector doesn't reveal one, either. Would love to see what you're referencing if I'm mistaken.

Jessidhia commented 6 years ago

That will be the case for delegated events. For example, $(document.body).on('click', '.my-selector', e => e.stopPropagation()).

sebmarkbage commented 6 years ago

Look, this can be solved in React, if someone just convinces me that this can’t be solved with my proposed design above which requires some restructuring of your code. But I haven’t seen any reason that can’t be done other than just trying to find a quick fix workaround.

craigkovatch commented 6 years ago

@sebmarkbage your proposal only solves the case of the events propagating to the immediate owner. What about the rest of the tree?

jquense commented 6 years ago

Here is a use case I think can't be solved well with Slots or createPortal

<Form defaultValue={fromValue}>
   <more-fancy-markup />
   <div>
     <Field name="faz"/>
     <ComplexFieldModal>
       <Field name="foo.bar"/>
       <Field name="foo.baz"/>
     </ComplexFieldModal>
  </div>
</Form>

And here is a gif with a similar but slightly different setup, where i'm using createPortal for a responsive site, to move a form field to the app toolbar (much higher up the tree). In this case as well I really don't want events bubbling back to the page content, but i definitely want the Form context to go with it. My implementation btw is some Slot-esque thing using context...

large gif 640x320

JasonGore commented 6 years ago

@sebmarkbage unstable_renderSubtreeIntoContainer allowed direct access to the top of the hierarchy regardless of a component's position, either within the hierarchy or as part of a separate packaged framework.

Comparatively, I see a few problems with the Slot solution:

slorber commented 6 years ago

I also have a usecase (probably similar to ones already mentionned).

I have a surface where users can select things with his mouse with a "lasso". This is basically 100% width/height and is at the root of my app and use onMouseDown event. In this surface there are also buttons that open portals like modals and dropdowns. A mouseDown event inside the portal is actually intercepted by the lasso selection component at the root of the app.

I see many to fix the problem:

For now my solution is to filter out the events.

const appRootNode = document.getElementById('root');

const isInPortal = element => isNodeInParent(element, appRootNode);

    handleMouseDown = e => {
      if (!isInPortal(e.target)) {
        return;
      }
      ...
    };

This will clearly not be the best solution for all of us and will not be very nice if you have nested portals, but for my current usecase (which is the only one currently) it works. I don't want to add a new context lib or do a complex refactor to solve this. Just wanted to share my solution.

JasonGore commented 6 years ago

I've been able to accomplish blocking event bubbling as noted elsewhere in this thread.

But another seemingly thornier issue I'm running into is the onMouseEnter SyntheticEvent, which does not bubble up. Rather it traverses from the common parent of the from component to the to component as described here. This means that if the mouse pointer enters from outside of the browser window, every onMouseEnter handler from the top of the DOM down to the component in createPortal will be triggered in that order, causing all sorts of events to fire that never did with unstable_renderSubtreeIntoContainer. Since onMouseEnter doesn't bubble up, it can't be blocked at the Portal level. (This did not seem to an issue with unstable_renderSubtreeIntoContainer as the onMouseEnter event did not respect virtual hierarchy and did not sequence through the body content, rather descended directly into the subtree.)

If anyone has any ideas on how to prevent onMouseEnter events from propagating from the top of the DOM hierarchy or divert directly into the portal subtree, please let me know.

hegelstad commented 5 years ago

@JasonGore I've also noticed this behavior.

For instance.

I have a context menu that is rendered when a div triggers onMouseOver, then I open a Modal with createPortal by clicking one of the items in the menu. When I bring the mouse out of the browser window, the onMouseLeave event propagates up to the context menu, closing the context menu (and thus the Modal)...

jnsandrew commented 5 years ago

Had the same issue where I had a list item that I wanted the entirety to be clickable (as a link), but wanted a delete button on labels underneath the name which would open a modal to confirm.

screenshot 2018-10-31 at 11 42 47

My only solution was to prevent bubbling on the modal div like so:

// components/Modal.js

onClick(e) {
    e.stopPropagation();
}

return createPortal(
        <div onClick={this.onClick} ...
            ...

It'll prevent bubbling on every modal yes, but I have no case where I would want that to happen yet so it works for me.

Are there potential issues with this approach?