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.

Sign up for GitHub

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 createRoot in ReactEventIndependence-test #28052

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Use createRoot in ReactEventIndependence-test #28052

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afe3351
Use createRoot in ReactEventIndependence-test
Jan 22, 2024
5145a9e
Add unmounted tree test case back in
Jan 23, 2024
a9b85a8
lint fix
Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 32 additions & 23 deletions packages/react-dom/src/__tests__/ReactEventIndependence-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
'use strict';

let React;
let ReactDOMClient;
let act;
let ReactDOM;

describe('ReactEventIndependence', () => {
Expand All @@ -18,60 +20,67 @@ describe('ReactEventIndependence', () => {

React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
act = require('internal-test-utils').act;
});

it('does not crash with other react inside', () => {
it('does not crash with other react inside', async () => {
let clicks = 0;
const container = document.createElement('div');
document.body.appendChild(container);
const root = ReactDOMClient.createRoot(container);
try {
const div = ReactDOM.render(
<div
onClick={() => clicks++}
dangerouslySetInnerHTML={{
__html: '<button data-reactid=".z">click me</div>',
}}
/>,
container,
);
await act(() => {
root.render(
<div
onClick={() => clicks++}
dangerouslySetInnerHTML={{
__html: '<button data-reactid=".z">click me</div>',
}}
/>,
);
});

div.firstChild.click();
container.firstElementChild.click();
expect(clicks).toBe(1);
} finally {
document.body.removeChild(container);
}
});

it('does not crash with other react outside', () => {
it('does not crash with other react outside', async () => {
let clicks = 0;
const outer = document.createElement('div');
document.body.appendChild(outer);
const root = ReactDOMClient.createRoot(outer);
try {
outer.setAttribute('data-reactid', '.z');
const inner = ReactDOM.render(
<button onClick={() => clicks++}>click me</button>,
outer,
);
inner.click();
await act(() => {
root.render(<button onClick={() => clicks++}>click me</button>);
});
outer.firstElementChild.click();
expect(clicks).toBe(1);
} finally {
document.body.removeChild(outer);
}
});

it('does not when event fired on unmounted tree', () => {
it('does not when event fired on unmounted tree', async () => {
let clicks = 0;
const container = document.createElement('div');
document.body.appendChild(container);
try {
const button = ReactDOM.render(
<button onClick={() => clicks++}>click me</button>,
container,
);
const root = ReactDOMClient.createRoot(container);

await act(() => {
root.render(<button onClick={() => clicks++}>click me</button>);
});

const button = container.firstChild;

// Now we unmount the component, as if caused by a non-React event handler
// for the same click we're about to simulate, like closing a layer:
ReactDOM.unmountComponentAtNode(container);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since unmountComponentAtNode is deprecated and root.unmount() completely removes the button element, I'm not sure there's anything else to test here so I've removed the test case. Any reason to continue testing events on unmounted trees like this?

Copy link
Member

Choose a reason for hiding this comment

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

@jackpope root.unmount() doesn't remove the container, which is where the event listeners are attached so the test is needed to confirm the event handlers on the container are not fired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying node#remove because I don't think we have any non-deprecated APIs that can remove the container?

If I remove the container from the DOM, the click handler still fires.

container.remove();
button.click();
expect(clicks).toBe(0); // assertion fails

If I remove the button from the DOM, the event handler does not fire

button.remove();
button.click();
expect(clicks).toBe(0);

Is this expected?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm confused, why not use root.unmount()? That behaves the same as unmountComponentAtNode which also removes the button element?

Copy link
Member

Choose a reason for hiding this comment

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

It does seem silly though to confirm that when you remove an element from the DOM, clicking doesn't bubble up to the element it was attached to. The test is really old, @sophiebits do you know what this is testing for and if it's still relevant?

Copy link
Member

@rickhanlonii rickhanlonii Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, maybe this is from when we still delegated events to the document, since detached elements still fire events on the document? Which is interesting because that would make it not relevant post-17, but will be relevant again after the event changes we want to make.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can read the original context here: #4865 #3790

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rickhanlonii root.unmount() nullifies any reference I've tried to store for the button element. So I haven't been able to click on anything. Unlike unmountComponentAtNode which clears the container in the same way but preserves the button element reference.

Copy link
Member

Choose a reason for hiding this comment

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

@jackpope this works:

it('does not when event fired on unmounted tree', async () => {
    let clicks = 0;
    const container = document.createElement('div');
    document.body.appendChild(container);
    try {
      const root = ReactDOMClient.createRoot(container);

      await act(async () => {
        root.render(<button onClick={() => clicks++}>click me</button>);
      });

      const button = container.firstChild;

      // Now we unmount the component, as if caused by a non-React event handler
      // for the same click we're about to simulate, like closing a layer:
      root.unmount();
      button.click();

      // Since the tree is unmounted, we don't dispatch the click event.
      expect(clicks).toBe(0);
    } finally {
      document.body.removeChild(container);
    }
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah so I was overcomplicating it. Thanks!

root.unmount();
button.click();

// Since the tree is unmounted, we don't dispatch the click event.
Expand Down