Add simulateEventDispatch to test ReactDOMEventListener #28079

rickhanlonii · 2024-01-25T04:38:36Z

Overview

For events, the browser will yield to microtasks between calling event handers, allowing time to flush work inbetween. For example, in the browser, this code will log the flushes between events:

<body onclick="console.log('body'); Promise.resolve().then(() => console.log('flush body'));">
  <div onclick="console.log('div'); Promise.resolve().then(() => console.log('flush div'));">
    hi
  </div>
</body>

// Logs
div 
flush div 
body 
flush body

Sandbox

The problem is, dispatchEvent (either in the browser, or JSDOM) does not yield to microtasks. Which means, this code will log the flushes after the events:

const target = document.getElementsByTagName("div")[0];
const nativeEvent = document.createEvent("Event");

nativeEvent.initEvent("click", true, true);
target.dispatchEvent(nativeEvent);

// Logs
div
body
flush div
flush body

The problem

This mostly isn't a problem because React attaches event handler at the root, and calls the event handlers on components via the synthetic event system. We handle flushing between calling event handlers as needed.

However, if you're mixing capture and bubbling events, or using multiple roots, then the problem of not flushing microtasks between events can come into play. This was found when converting a test to createRoot in #28050 (comment), and that test is an example of where this is an issue with nested roots.

Here's a sandox for discrete and continuous events, showing how the test should behave. The existing test, when switched to createRoot matches the browser behavior for continuous events, but not discrete. Continuous events should be batched, and discrete should flush individually.

The fix

This PR implements the fix suggested by @sebmarkbage, to manually traverse the path up from the element and dispatch events, yielding between each call.

react-sizebot · 2024-01-25T04:44:21Z

Comparing: 952aa74...b9ed282

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	176.37 kB	176.37 kB	=	54.96 kB	54.96 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	178.37 kB	178.37 kB	=	55.54 kB	55.54 kB
facebook-www/ReactDOM-prod.classic.js	=	590.79 kB	590.79 kB	=	104.26 kB	104.26 kB
facebook-www/ReactDOM-prod.modern.js	=	574.55 kB	574.55 kB	=	101.36 kB	101.36 kB
test_utils/ReactAllWarnings.js	Deleted	67.02 kB	0.00 kB	Deleted	16.42 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	67.02 kB	0.00 kB	Deleted	16.42 kB	0.00 kB

Generated by 🚫 dangerJS against b9ed282

sophiebits · 2024-01-25T16:40:58Z

(can we call it dispatch or simulate or something, as it does more than just wait?)

rickhanlonii · 2024-01-25T17:21:57Z

@sophiebits renamed to dispatchAndWaitForDiscrete which mirrors the waitForDiscrete function that exists. I figured @acdlite would have opinions on naming as well.

rickhanlonii · 2024-01-25T17:24:20Z

I think the ideal implementation of this would accept an array of array of logs, with the logs we expect between each yield, and asserting that they're logged. Calling waitForDiscrete for each, instead of waitForMicrotasks, but that could be a lot of empty items to specify and seems like too much.

rickhanlonii · 2024-01-25T17:32:07Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

-        // By the time we leave the handler, the second update is flushed.
+        //
+        // Since this is a discrete event, the previous update is already done.
+        if (gate(flags => flags.enableLegacyFBSupport)) {


I don't think this was the right fix, I think the issue is we're not dispatching up to the document, because the other tests are failing for the same reason. I'll look deeper and figure out the fix

I think the issue here isn't the document.

I'm not sure how this works exactly if it's two listeners at the document or just one. I think it's actually two listeners at the document. So we should flush between them.

The issue is that dispatchAndWaitForDiscrete waits between nodes but it doesn't wait between events on the same node.

I wonder if there's a way to get to the internal list of events in JSDOM. Otherwise, you might have to patch EventTarget.prototype.addEventListener so you can gather your own list and iterate through it and wait between each one.

Looks like you can get access to internals through the symbol:

const impl = Object.getOwnPropertySymbols(x)[0]; x[impl]._eventListeners

They should've called it DO_NOT_USE.

You could potentially fork https://github.com/jsdom/jsdom/blob/2f8a7302a43fff92f244d5f3426367a8eb2b8896/lib/jsdom/living/events/EventTarget-impl.js#L114 and make it an async function.

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

sebmarkbage · 2024-01-25T17:46:38Z

packages/internal-test-utils/ReactInternalTestUtils.js

+  // Bubbling phase
+  // Walk the path from the element to the document and dispatch
+  // the event on each node, flushing microtasks in between.
+  for (let current = node; current; current = current.parentNode) {


Technically if you remove or move a node during this phase it'll get the events.

So really this should create a snapshot of all parent nodes into an array and then dispatch to each of those. That way if they change the DOM during the bubbling phase, all events still happen (on disconnected nodes).

I wish I knew less about the DOM.

Also, if this doesn't end up with a parentNode that is in a document, it shouldn't dispatch anything because it means the target was detached before dispatching.

Fixed and added a test.

sebmarkbage · 2024-01-25T17:54:19Z

packages/internal-test-utils/ReactInternalTestUtils.js

+    Object.defineProperty(customEvent, 'eventPhase', {
+      // Avoid going through the capture/bubbling phases,
+      // since we're doing it manually.
+      value: Event.AT_TARGET,


Why do you need to set this if you're dispatching to each target without bubbling anyway? Shouldn't this always be AT_TARGET anyway?

Ideally it should only be AT_TARGET when node === current and otherwise the phase. You could potentially set it to CAPTURING_PHASE and add an event listener in the end of the capturing phase and then in that listener switch to BUBBLING_PHASE.

Not sure this matters though.

The phases will be wrong anyway since it'll be capture/bubble at each node instead of all capture and then all bubbles.

sebmarkbage · 2024-01-25T18:02:20Z

packages/internal-test-utils/__tests__/ReactInternalTestUtilsDOM-test.js

+        );
+      }
+
+      const container = document.createElement('div');


This container is not added to the document body so it shouldn't get any events dispatched to it.

The test probably intended to add it to the body like the others.

Fixed and added a test.

rickhanlonii · 2024-02-06T04:10:00Z

@sebmarkbage updated to fork dispatchEvent, which fixed the test I was gating for the document root for FB. It ain't pretty but it works, I hope lol

This also means we don't need to manually dispatch events up the tree, so the capture phase isn't duplicated. I've renamed the function simulateEventDispatch, and added more tests.

@sebmarkbage

## Overview For events, the browser will yield to microtasks between calling event handers, allowing time to flush work inbetween. For example, in the browser, this code will log the flushes between events: ```js <body onclick="console.log('body'); Promise.resolve().then(() => console.log('flush body'));"> <div onclick="console.log('div'); Promise.resolve().then(() => console.log('flush div'));"> hi </div> </body> // Logs div flush div body flush body ``` [Sandbox](https://codesandbox.io/s/eloquent-noether-mw2cjg?file=/index.html) The problem is, `dispatchEvent` (either in the browser, or JSDOM) does not yield to microtasks. Which means, this code will log the flushes after the events: ```js const target = document.getElementsByTagName("div")[0]; const nativeEvent = document.createEvent("Event"); nativeEvent.initEvent("click", true, true); target.dispatchEvent(nativeEvent); // Logs div body flush div flush body ``` ## The problem This mostly isn't a problem because React attaches event handler at the root, and calls the event handlers on components via the synthetic event system. We handle flushing between calling event handlers as needed. However, if you're mixing capture and bubbling events, or using multiple roots, then the problem of not flushing microtasks between events can come into play. This was found when converting a test to `createRoot` in facebook#28050 (comment), and that test is an example of where this is an issue with nested roots. Here's a sandox for [discrete](https://codesandbox.io/p/sandbox/red-http-2wg8k5) and [continuous](https://codesandbox.io/p/sandbox/gracious-voice-6r7tsc?file=%2Fsrc%2Findex.js%3A25%2C28) events, showing how the test should behave. The existing test, when switched to `createRoot` matches the browser behavior for continuous events, but not discrete. Continuous events should be batched, and discrete should flush individually. ## The fix This PR implements the fix suggested by @sebmarkbage, to manually traverse the path up from the element and dispatch events, yielding between each call.

@sebmarkbage

## Overview For events, the browser will yield to microtasks between calling event handers, allowing time to flush work inbetween. For example, in the browser, this code will log the flushes between events: ```js <body onclick="console.log('body'); Promise.resolve().then(() => console.log('flush body'));"> <div onclick="console.log('div'); Promise.resolve().then(() => console.log('flush div'));"> hi </div> </body> // Logs div flush div body flush body ``` [Sandbox](https://codesandbox.io/s/eloquent-noether-mw2cjg?file=/index.html) The problem is, `dispatchEvent` (either in the browser, or JSDOM) does not yield to microtasks. Which means, this code will log the flushes after the events: ```js const target = document.getElementsByTagName("div")[0]; const nativeEvent = document.createEvent("Event"); nativeEvent.initEvent("click", true, true); target.dispatchEvent(nativeEvent); // Logs div body flush div flush body ``` ## The problem This mostly isn't a problem because React attaches event handler at the root, and calls the event handlers on components via the synthetic event system. We handle flushing between calling event handlers as needed. However, if you're mixing capture and bubbling events, or using multiple roots, then the problem of not flushing microtasks between events can come into play. This was found when converting a test to `createRoot` in #28050 (comment), and that test is an example of where this is an issue with nested roots. Here's a sandox for [discrete](https://codesandbox.io/p/sandbox/red-http-2wg8k5) and [continuous](https://codesandbox.io/p/sandbox/gracious-voice-6r7tsc?file=%2Fsrc%2Findex.js%3A25%2C28) events, showing how the test should behave. The existing test, when switched to `createRoot` matches the browser behavior for continuous events, but not discrete. Continuous events should be batched, and discrete should flush individually. ## The fix This PR implements the fix suggested by @sebmarkbage, to manually traverse the path up from the element and dispatch events, yielding between each call. DiffTrain build for commit cd63ef7.

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 25, 2024

rickhanlonii mentioned this pull request Jan 25, 2024

Convert ReactDOMEventListener to createRoot #28050

Merged

rickhanlonii changed the title ~~Add waitForEvents to test ReactDOMEventListener~~ Add dispatchAndWaitForDiscrete to test ReactDOMEventListener Jan 25, 2024

rickhanlonii commented Jan 25, 2024

View reviewed changes

sebmarkbage reviewed Jan 25, 2024

View reviewed changes

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js Outdated Show resolved Hide resolved

sebmarkbage reviewed Jan 25, 2024

View reviewed changes

rickhanlonii added 8 commits February 5, 2024 16:28

Convert ReactDOMEventListener to createRoot

8d30643

Fix innerRef and ReactDOMX

1fcc459

Revert batching test back, will follow up

ca7c9f6

Fix lint

d2eae5b

Add waitForEvents to test ReactDOMEventListener

1e5f3ce

Legacy FB support

d0f2165

Rename to dispatchAndWaitForDiscrete.

e6e144c

Fork dispatchEvent, fix and add tests

6b73d5d

rickhanlonii force-pushed the rh/test-event-listener-2 branch from 2ae431c to 6b73d5d Compare February 6, 2024 04:05

rickhanlonii added 2 commits February 5, 2024 23:10

Merge branch 'main' into rh/test-event-listener-2

137a093

Fix lint

b9ed282

rickhanlonii marked this pull request as ready for review February 6, 2024 04:24

rickhanlonii changed the title ~~Add dispatchAndWaitForDiscrete to test ReactDOMEventListener~~ Add simulateEventDispatch to test ReactDOMEventListener Feb 6, 2024

rickhanlonii requested a review from sebmarkbage February 7, 2024 02:58

sebmarkbage approved these changes Feb 8, 2024

View reviewed changes

rickhanlonii merged commit cd63ef7 into facebook:main Feb 8, 2024
36 checks passed

rickhanlonii deleted the rh/test-event-listener-2 branch February 8, 2024 21:06

ztanner mentioned this pull request Feb 9, 2024

Update React from 2bc7d336a to ba5e6a832 vercel/next.js#61837

Merged

ztanner mentioned this pull request Mar 19, 2024

(backport) Update React from 60a927d04 to 14898b6a9 vercel/next.js#63442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simulateEventDispatch to test ReactDOMEventListener #28079

Add simulateEventDispatch to test ReactDOMEventListener #28079

rickhanlonii commented Jan 25, 2024 •

edited

Loading

react-sizebot commented Jan 25, 2024 •

edited

Loading

sophiebits commented Jan 25, 2024

rickhanlonii commented Jan 25, 2024

rickhanlonii commented Jan 25, 2024 •

edited

Loading

rickhanlonii Jan 25, 2024

sebmarkbage Jan 25, 2024 •

edited

Loading

sebmarkbage Jan 25, 2024

sebmarkbage Jan 25, 2024 •

edited

Loading

sebmarkbage Jan 25, 2024

rickhanlonii Feb 6, 2024

sebmarkbage Jan 25, 2024

sebmarkbage Jan 25, 2024

rickhanlonii Feb 6, 2024

rickhanlonii commented Feb 6, 2024

Add simulateEventDispatch to test ReactDOMEventListener #28079

Add simulateEventDispatch to test ReactDOMEventListener #28079

Conversation

rickhanlonii commented Jan 25, 2024 • edited Loading

Overview

The problem

The fix

react-sizebot commented Jan 25, 2024 • edited Loading

Critical size changes

Significant size changes

sophiebits commented Jan 25, 2024

rickhanlonii commented Jan 25, 2024

rickhanlonii commented Jan 25, 2024 • edited Loading

rickhanlonii Jan 25, 2024

Choose a reason for hiding this comment

sebmarkbage Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

sebmarkbage Jan 25, 2024

Choose a reason for hiding this comment

sebmarkbage Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

sebmarkbage Jan 25, 2024

Choose a reason for hiding this comment

rickhanlonii Feb 6, 2024

Choose a reason for hiding this comment

sebmarkbage Jan 25, 2024

Choose a reason for hiding this comment

sebmarkbage Jan 25, 2024

Choose a reason for hiding this comment

rickhanlonii Feb 6, 2024

Choose a reason for hiding this comment

rickhanlonii commented Feb 6, 2024

rickhanlonii commented Jan 25, 2024 •

edited

Loading

react-sizebot commented Jan 25, 2024 •

edited

Loading

rickhanlonii commented Jan 25, 2024 •

edited

Loading

sebmarkbage Jan 25, 2024 •

edited

Loading

sebmarkbage Jan 25, 2024 •

edited

Loading