Bug: [email protected] event delegation issue

[linde12]

This is pretty much a duplicate of TanStack/table#3002

React version: 17.x

Bug description
As i was developing i forgot to memoize some mocked data before passing it to react-table's useTable hook and encountered an interesting bug, which led me down a rabbit hole instead of actually seeing the problem (missed useMemo on my data)

It only happens when you use useTable combined with useSortBy, and only with [email protected] (event delegation rewrite release), e.g [email protected] works fine.

It is hard to put to words, but i have attached a minimal example (codesandbox) and will try my best to explain. In the codesandbox i have three components:

• Modal which has a useEffect that adds a click event listener to document. This listener acts like a backdrop (where if i click the backdrop the "modal" should close. Rendered by App.
• Table which is the component that calls useTable (but doesn't use anything it returns) - also rendered by App.
• App which is the main application and controls the state that decides whether Modal is rendered or not. Here i actively "forgot" to use useMemo on my data.

What happens is that when you click "Open Modal" using [email protected] the callback to document.addEventListener('click', ..) is called, but this element shouldn't have been rendered yet at the point when the click event from the button was fired. The result is that you never, or very briefly, see the "modal". Doing the same using [email protected] (you can change in the sandbox) results in the expected behavior of the "modal" actually showing.

Codesandbox example
https://codesandbox.io/s/react-table-react-17-5nw4d?file=/src/App.js

Steps To Reproduce
Steps to reproduce the behavior:

1. Go to https://codesandbox.io/s/react-table-react-17-5nw4d?file=/src/App.js
2. See react-dom version is 17.0.1
3. Press "Open Modal" button
4. Nothing happens
5. Change version of react-dom to 16.14.0 for example.
6. Press "Open Modal" button
7. See the modal get opened as expected.

Expected behavior
Obviously i can't expect the react-table library to work properly, but i would not expect it to have the weird observed undefined behavior. I also would not expect this to have different behaviors across react-dom versions, even if it is a major release.

Additional context
I am very curious to what is actually going on and whether this is a bug of react-dom or react-table or a combination of both. I've been debugging to try and find the root cause, but unfortunately i'm not very familiar with the code of either repos and i believe there are things happening asynchronously which is very hard for me to follow.

It would be nice if anyone wanted to help me debug this issue, so we can get to the root of the problem and perhaps have a more minimal example.

[gaearon]

I don't think there's a bug here. You can reproduce the same behavior with vanilla DOM:

https://codesandbox.io/s/immutable-wildflower-w82nx?file=/src/index.js

document.getElementById("app").innerHTML = `
<button id="btn">
  click me
</button>
`;
document.getElementById("btn").onclick = function () {
  document.addEventListener("click", function () {
    alert("document click fired");
  });
};

If you add a document listener during a button click, then that listener will fire (as a part of that click propagating upwards).

This didn't happen in React <17 because React used to register its events at the document level. So React handlers would run too late (when the event has already bubbled to the document level), and thus registering another document-level listener would not have a consequence. But React 17 behaves closer to how regular DOM works.

[gaearon]

Have you considered using something like https://reach.tech/dialog instead of rolling your own?

[linde12]

Hi @gaearon and thanks for taking your time to help me figure this out. Yes, i saw in the release notes that the listener was moved to the app root rather than keeping it at the document level, so i would expect things to bubble (and i know i can work around this by stopping propagation), but the weird thing to me is that, in my head, the effect in Modal shouldn't even have been run yet. And this behavior only happens when useSortBy is passed to useTable, otherwise the propagation works as i would expect.

To me it looks like:

1. On line 33 setOpen is called, the click handler returns immediately after setOpen, i then expect the click event to continue bubbling up and first after this process has finished should any other tasks in the event loop be handled.
2. The setOpen of step 1 queued a re-render of the App component, so it is run again and now open is actually set to true
3. At this point Modal will be mounted and renders a div with the contents "my component"
4. The useEffect of Modal is then run, calling document.addEventListener
5. By this stage the click-event, in my eyes, is long gone and already handled and propagated, but my handler for document.addEventListener('click', ...) is called, causing handleClick to be run which in turn does setOpen(false) and hides the modal immediately

Maybe there is something asynchronous going on which i am missing. I'm using the Chrome debugger and seeing that the handler of my addEventListener call is being called immediately leaves me to think that event propagation was delayed somehow?

To continue on your vanilla example: (run here: https://codesandbox.io/s/falling-tdd-0tthx?file=/src/index.tsx):

document.getElementById("app").innerHTML = `
<button id="btn">
  click me
</button>
`;
document.getElementById("btn").onclick = function () {
  setTimeout(() => { // mimic async behavior of setOpen
    document.addEventListener("click", function () {
      alert("document click fired");
    });
  }, 0);
};

If we add a setTimeout to the onclick, mimicing the asynchronous operation react does when using the setter of useState, the event listener will not fire immediately, because the event bubbling will finish before next task of the event loop is picked up. Again, i'm a bit on thin ice here and maybe i'm making some wrong assumptions.

Regarding the library i don't think i need it in my case (i'm using a small little useBackdrop hook) If i just add a event.stopPropagation() to my hook i'm all good, but my curiosity is getting the better of me - in my mind this shouldn't be needed in the first place.

EDIT: Of course, this is all based on that setState from const [state, setState] = useState(...) is asynchronous, which i guess with fiber and all might not always be true(? again on thin ice) But even so it it is interesting why it happens only when i pass useSortBy. I tested adding useFilters instead, and same is true there so maybe it's not the actual "plugin", but whatever happens when a plugin is added to useTable

[linde12]

Here is a fork of the original codesandbox without passing useSortBy to the useTable hook (but really the entire Table component could be removed now as it serves no purpose) - as you can see it works perfectly fine. So it'd be interesting to see whatever in useSortBy causes the propagation of the click event to be delayed.

https://codesandbox.io/s/react-table-react-17-forked-212ou?file=/src/App.js

[linde12]

Was there any changes to how fiber renders in 17.x?

Maybe it all boils down to synchronous vs asynchronous rendering after setOpen - if in React 16 it was async, but in 17.x it is optimized to be sync then that would explain the difference in behavior. I can't find any release notes about this, so i'm not sure whether this side effect is something that was considered or if it is actually some unintended side effect.

[sub-zero1]

okay thanks

…

On Mon, Jan 25, 2021 at 11:29 AM Oscar Linde ***@***.***> wrote: Was there any changes to how fiber renders in 17.x? Maybe it all boils down to synchronous vs asynchronous rendering after setOpen - if in React 16 it was async, but in 17.x it is optimized to be sync then that would explain the difference in behavior. I can't find any release notes about this, so i'm not sure whether this side effect is something that was considered or if it is actually some unintended side effect. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#20636 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APYPGPVLSTRTWRTURPQVYVDS3VI2NANCNFSM4WNIAS2A> .

[maksimr]

We faced similar issue(s) after migrating to [email protected].
Does it expect behaviour(feature) in react@17, or it's a bug? In my mental concept useEffect should be called asynchronously when we have changed dependency or just install it but in reality it's called synchronously sometimes that lead to such nasty problem.

[eps1lon]

In my mental concept useEffect should be called asynchronously when we have changed dependency or just install it but in reality it's called synchronously sometimes that lead to such nasty problem.

Related: #20863 (comment)

[zidian257]

React 17 changes the root of event delegation target from document to AppRoot, still I can reproduce this issue with this codesandbox.

imo the event bubbling here in React Component has some inconsistency to DOM implementation, it should be noted when user tries to do delicate event bubbling manipulation.

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[github-actions]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!