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

[core] feat(Overlay, Portal): allow portal to stop event propagation #6093

Merged
merged 2 commits into from
May 4, 2023

Conversation

dylanrcooke
Copy link
Contributor

@dylanrcooke dylanrcooke commented Apr 27, 2023

Works around facebook/react#11387

Fixes #6124

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Currently react have a longstanding issue where portals propagate events up the react tree. This PR allows a list of events to be passed in to prevent propagation through the portal on.

Reviewers should focus on:

Whether or not this should be configurable, or if we should always stopPropagation on events on the portal element.

Screenshot

No user facing changes.

* @see https://legacy.reactjs.org/docs/portals.html#event-bubbling-through-portals
* @see https://github.com/palantir/blueprint/issues/6124
*/
portalStopPropagationEvents?: Array<keyof HTMLElementEventMap>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusted the name of this prop based on the other "portal" props on this interface. I admit it's getting a bit unwieldy and a portalProps prop might be better, but this is OK for now...

@@ -128,6 +143,14 @@ export class Portal extends React.Component<PortalProps, IPortalState> {
}
return container;
}

private addStopPropagationListeners(eventNames?: Array<keyof HTMLElementEventMap>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed these functions to be more specific

@adidahiya
Copy link
Contributor

Whether or not this should be configurable, or if we should always stopPropagation on events on the portal element.

This should definitely be configurable; I don't want to enable new behavior by default that might break existing applications.

There's another question of whether the prop should just be a simple boolean which makes Portal capture all events (rather than a specific denylist). Again, I'm opting to be a little conservative here and going with the way you've initially implemented it, which requires users to know the list of events which they might be having problems with on the underlying content. I think this will be the least surprising for users. For example, we don't want to introduce unnecessary issues with drag & drop listeners that might get blocked if we start capturing all events.

@adidahiya adidahiya changed the title Add stop propagation events to portal [core] feat(Overlay, Portal): allow portal to stop event propagation May 4, 2023
@adidahiya adidahiya merged commit 9c1384a into palantir:develop May 4, 2023
adidahiya added a commit that referenced this pull request May 4, 2023
…PropagationEvents

While preserving new feature added in #6093
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 this pull request may close these issues.

Events propagate through Portals in Overlay elements
2 participants