DevTools: useModalDismissSignal bugfix

[bvaughn]

Make useModalDismissSignal's manually added click/keyboard events more robust to sync flushed passive effects. (Don't let the same click event that shows a modal dialog also dismiss it.)

Fixes #21172

[sizebot]

Comparing: b4f119c...8f1087e

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	=	122.62 kB	122.62 kB	=	39.34 kB	39.34 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	129.18 kB	129.18 kB	=	41.42 kB	41.42 kB
facebook-www/ReactDOM-prod.classic.js	=	405.84 kB	405.84 kB	=	75.37 kB	75.37 kB
facebook-www/ReactDOM-prod.modern.js	=	394.10 kB	394.10 kB	=	73.51 kB	73.51 kB
facebook-www/ReactDOMForked-prod.classic.js	=	405.84 kB	405.84 kB	=	75.37 kB	75.37 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 8f1087e

[gaearon]

Hmm. It’s not obvious to me that relying on a timestamp is the correct solution. What happens if some code in the middle just happens to spend enough CPU time? Does this affect the logic anyhow?

[bvaughn]

Hmm. It’s not obvious to me that relying on a timestamp is the correct solution. What happens if some code in the middle just happens to spend enough CPU time? Does this affect the logic anyhow?

@gaearon I'm not sure I understand the scenario you're describing.

The specific issue I'm fixing here is tis:

1. A click event causes a modal to be shown.
2. The modal has a passive effect which adds a click handler to the document.
3. React now flushes this passive effect synchronously.
4. The newly added click event handler also picks up on the original click event due to bubbling.

So my fix is to check if the click event was trigger before the passive effect that added the listener. This fix should not be affected by code running in the middle. (That would just make the delta between the event and effect time larger.)

An alternative fix would be to add a small delay before attaching my event handler in the effect, but this approach seemed more obvious/direct.

[gaearon]

Do we expect any problems due to limited precision? Eg Firefox seems to have a privacy setting for this. https://developer.mozilla.org/en-US/docs/Web/API/Event/timeStamp

[bvaughn]

Do we expect any problems due to limited precision? Eg Firefox seems to have a privacy setting for this. developer.mozilla.org/en-US/docs/Web/API/Event/timeStamp

Even for older implementations, I don't think it seems that likely for React to render an update, commit it, and flush passive effects in < 1 ms. And because I return only if timeOfEffect > event.timeStamp, I wouldn't early return if the time between the effect running and a subsequent click event was < 1ms.

I also don't think it's very common for React DevTools to be running in older browser versions. For instance, Chrome switched to high res timestamps in version 49 (released over 5 years ago). Seems likely that people using DevTools would also be using versions of Chrome released more recently than 5 years ago.

[bvaughn]

We didn't use event.timeStamp for the same reasons @gaearon mentioned. We just don't know how reliable event.timeStamp is in reality.

As mentioned in the comment above yours, I don't think this concern seems too likely. 😄

In Material-UI we basically just deferred "activation" of the click handler with setTimeout(() => activateClickHandler, 0). The rationale being that people are unlikely to click more often than every ~15ms and if they do they can always repeat the action (compared to not being able to open at all).

Simply delaying the event listener with setTimeout(() => activateClickHandler, 0) wouldn't be sufficient, because if another nested update (synchronously) removed the component you could end up leaking.

Something like this should be fine though:

useEffect(() => {
  function handler(event) {
    // ...
  }

  let timeoutID = setTimeout(() => {
    timeoutID = null;
    window.addEventListener("click", handler);
  }, 0);

  return () => {
    if (timeoutID !== null) {
      clearTimeout(timeoutID);
    }

    window.removeEventListener("click", handler);
  };
});

I think I slightly prefer the timeout approach in this PR though because the intent seems clearer to me.

[eps1lon]

Simply delaying the event listener with setTimeout(() => activateClickHandler, 0) wouldn't be sufficient, because if another nested update (synchronously) removed the component you could end up leaking.

Yes, we're actually doing that. Just was a bit lazy with my comment.

[bvaughn]

I'm still tempted to try this fix because I prefer the clarity of it vs the timestamp and I don't think it's problematic (especially in the DevTools case) but if we're concerned about setting a highly visible precedent for this case that may be copied elsewhere, I won't choose to die on this hill.

[gaearon]

Maybe it would be good to discuss with the team what's the canonical fix for this. Like you said, it's not so much about this case but just what do we actually recommend other people do.

[bvaughn]

Sounds good. Added it to our sync topics.

[bvaughn]

Btw, @eps1lon, I took the time to write up my thoughts/preferences about this at a little more length. In case you're interested:

https://gist.github.com/bvaughn/fc1c3f27f1aab91c7378f2264f7c3aa1