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

feat: event leak demo #7376

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

nwidynski
Copy link
Contributor

@nwidynski nwidynski commented Nov 15, 2024

Closes comment

**NOT INTENDED FOR MERGE ** - I only needed access to Storybook examples for demo purposes.

TLDR; I did not test for every possible interaction for now and also limited the assertions to grid interaction keys only. It's very likely that additional components will also fail when tested more throughly. Point of this demo was to hightlight that many components, not only nested collections, will cause problems when used inside grids.

Also related are #7370 and my latest comment on #7321.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. Open localhost:9003
  2. Navigate to Event Leak Grid story
  3. Watch the Playwright test

🧢 Your Project:

@nwidynski
Copy link
Contributor Author

It would be great if someone could issue a build for this 👍

@LFDanLu
Copy link
Member

LFDanLu commented Nov 22, 2024

Just chiming in, I believe I actually just ran into this event leak behavior myself in Autocomplete since I was dispatching fake keyboard events to the wrapped collection which bubbled up to the dialog and closed it prematurely. I'll make a duplicate of this branch since security dinged us on running builds on fork PRs and see about getting the team to take a look as soon as possible

@LFDanLu
Copy link
Member

LFDanLu commented Dec 6, 2024

@nwidynski Apologies for the delay, the team discussed this some more yesterday and moving forward we'll want to be more strict/consistent about stopping propagation on keyboard events. This usually happens by default when calling useKeyboard but as you've discovered we haven't been the best about consistently using that hook or making sure that a base level onKeyDown gets provided to the hook if none exists. We'll need to do an audit of all our components and make sure that keyboard events that get handled by a component have their propagation stopped and that keyboard events that aren't handled are still propagated.

This change will hopefully make handing interactive components within a grid much easier as well as possibly allowing the typeahead listener to become a non-capturing listener.

@nwidynski
Copy link
Contributor Author

nwidynski commented Dec 6, 2024

@LFDanLu Got it, I generally very much agree with the sentiment to have atomic building blocks. Let's hope this doesn't cause too many behavior issues in user applications 🙏

As this effort will likely take a while, we will scratch our immediate need by beginning work on open-sourcing a custom grid virtualizer, which prevents these leaks from reaching the collection 👍

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.

2 participants