Bug: DevTools settings dialog no longer opens

[bvaughn]

Click the settings gear in either the inline or extension build and it does not open.

Edit: This was broken by a change to React via f8ef4ff / #21150

[bvaughn]

I found this while testing the build from #21171 (using a version of React built from #15658). It repros on master (with a build of React from master as well).

Unclear to me yet whether this bug is caused by a change in DevTools or a change in React. I suspect the latter.

[bvaughn]

Changes to our sync scripts make bisecting slower. Have to build from source. 😁

This command is pretty fast way to build the minimum set of DevTools dependencies though:

RELEASE_CHANNEL=experimental \
  yarn build \
  react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh \
  --type=NODE_PROD

I've confirmed that building the latest DevTools with an older version of React (09a2c36) fixes the regression. I'll bisect now.

[bvaughn]

git bisect bad                                                                                                                                       
f8ef4ff571db3de73b0bfab566c1ce9d69c6582f is the first bad commit
commit f8ef4ff571db3de73b0bfab566c1ce9d69c6582f
Author: Andrew Clark <git@andrewclark.io>
Date:   Wed Mar 31 12:39:19 2021 -0500

    Flush discrete passive effects before paint (#21150)
    
    If a discrete render results in passive effects, we should flush them
    synchronously at the end of the current task so that the result is
    immediately observable. For example, if a passive effect adds an event
    listener, the listener will be added before the next input.
    
    We don't need to do this for effects that don't have discrete/sync
    priority, because we assume they are not order-dependent and do not
    need to be observed by external systems.
    
    For legacy mode, we will maintain the existing behavior, since it hasn't
    been reported as an issue, and we'd have to do additional work to
    distinguish "legacy default sync" from "discrete sync" to prevent all
    passive effects from being treated this way.

[eps1lon]

Isn't that basically working around #20074. Sounds like #20074 is now happening regardless of whether portals are rendered or not.

So any solution you come up might be interesting to everybody.

In Material-UI we just deferred "activation" with a crude setTimeout(() => addClickListener(), 0). The rationale being that it seems highly unlikely that people click more often than < 100ms and if they do they can always try again (which is better than not being able to open a popup at all).

We didn't use the event.timeStamp solution for the exact reasons @Gaeron mentioned i.e. it's unclear if that field is reliable.

[bvaughn]

I don't think I share Dan's concern about timestamp resolution in this case. See my comment here for rationale: #21173 (comment)

[bvaughn]

Let's talk about it here (on the issue) or there (on the PR) but not both places 😁

[eps1lon]

Oh, I commented already. I thought I forgot to submit.