Skip to content

Add iframe support and fix nested act #1275

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

snowystinger
Copy link
Contributor

@snowystinger snowystinger commented Feb 13, 2025

What

This adds iframe support (next to the already existing shadow dom support) to the active element so that userEvent can fire keyboard events on the correct element. This is particularly important because the event needs to stay within the iframe, it shouldn't ever appear outside of it.

It also removes an extra act which was causing us issues. This may be more of a bug in React, we're a little unsure, but we think this is still more correct here.

Why

We ran into these while developing a feature in adobe/react-spectrum#7561

How

Checklist

  • Documentation
  • Tests
  • Ready to be merged

Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@mgmolisani
Copy link

mgmolisani commented Apr 13, 2025

We actually just ran into this bug as well with the extra act. Since the nested sync act call in the change is flushed along with already wrapping call to act with blur, the act queue gets fully drained in the nested scope as that update is flushed and the act queue is torn down prematurely. This makes some future state updates added from subsequent effects cause the Warning: An update to %s inside a test was not wrapped in act(...). error. Since not all libraries trigger state updates in effects after focus events, the failure is uncommon. React seems to handle nested scopes for async calls to act but not sync which this is. Seems like an RTL bug.

Further, I think any of these code paths calling dispatchDOMEvent aren't behaving like a user (or the DOM) would. The solution in the PR (assuming the call to change is required) seems correct omit the eventWrapper. In user code, we would not be able to wrap our imperative event dispatches in act so it seems correct that we would not do so here either. In all the cases where dispatchDOMEvent is being called, we should already be in an act scope that triggered the focus/blur.

I don't actually fully understand what this code path is even trying to achieve TBH. I went back through history and couldn't find why it was added. Can someone explain why we would want to trigger a change event if the value the input was first intercepted with doesn't match the final value? It doesn't seem like this is fixing any missing JSOM focus/blur features or anything. In our codebase with over 5000 tests, simply deleting this line had no impact, all continued passing. Never mind, it does look like JSDOM is not conforming to spec. It does not trigger a change event on blur which explains this line.

I would suggest splitting this PR to address that issue separately. I think the solution can simply stop internally wrapping events in dispatchDOMEvent with eventWrapper and that should fix all cases (also passed all 5000 of our tests). This PR might go through multiple revisions as I think this implementation is creating the event in the wrong abstraction layer (see event/createEvent and will need more thought to trigger the right document constructors.

I can work on a repro but I'm hoping that there is already enough evidence to make this change to stop wrapping it. Im also hoping for an answer to the question of if this call is even necessary.

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