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

Use event.composedPath for getting event target (keydown shadow DOM fix) #2416

Merged
merged 1 commit into from Sep 14, 2022

Conversation

jwm0
Copy link
Contributor

@jwm0 jwm0 commented Sep 8, 2022

This PR fixes a problem where keystrokes are not being registered when originating from shadow DOM inputs. Current react-flow implementation attaches a global event listener for keydown event and calls preventDefault() on it if the following 2 conditions are met:

  1. The key name is registered (using props like deleteKeyCode, multiSelectionKeyCode etc.)
  2. isInputDOMNode returns true

This article explains why the current implementation of isInputDOMNode doesn't work as well as shows a solution which is used in this PR

Reproducible demo sandbox: https://codesandbox.io/s/react-flow-with-shadow-dom-lymfwm?file=/index.js

@moklick
Copy link
Member

moklick commented Sep 12, 2022

Thanks! I didn't know about this. I will have a look.

@moklick moklick self-assigned this Sep 12, 2022
@moklick moklick merged commit d22bf9e into xyflow:main Sep 14, 2022
@moklick
Copy link
Member

moklick commented Sep 14, 2022

released in v10.3.17

@jwm0
Copy link
Contributor Author

jwm0 commented Sep 21, 2022

@moklick would you be up to release this fix in v9 as well if I prepare a ported PR for that? Our team is currently on v9 and it would be great if we didn't have to patch this ourselves locally

@moklick
Copy link
Member

moklick commented Sep 21, 2022

We don't do fixes for v9. v10 was released six month ago and we are moving to v11 next week. Can't you upgrade to v10 or better v11 ?

@jwm0
Copy link
Contributor Author

jwm0 commented Sep 22, 2022

It's in the pipeline but not sure how much longer it'll take. Anyway, we can leave without it for now and hopefully can migrate sooner than we need this functionality. Thanks

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.

None yet

2 participants