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.

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

[v5] Popover doesn't close when clicked outside in React 18 StrictMode #6203

Closed
PetrusAsikainen opened this issue Jun 5, 2023 · 11 comments · Fixed by #6458
Closed

[v5] Popover doesn't close when clicked outside in React 18 StrictMode #6203

PetrusAsikainen opened this issue Jun 5, 2023 · 11 comments · Fixed by #6458

Comments

@PetrusAsikainen
Copy link
Contributor

PetrusAsikainen commented Jun 5, 2023

tl;dr: Popovers with interactionKind="click" in v5 don't close when rendered within React 18 StrictMode and the user clicks outside the popover. This includes anything depending on Popover, like Select.

This is an upstream bug in react-popper. Already filed a PR, but I'm also filing it here because I don't want anyone else to have to debug this. Only an uncommon use case of react-popper (Reference with innerRef) is affected but that breaks BlueprintJS's usage.

floating-ui/react-popper#459

v4 and popover2 are unaffected because they use findDOMNode instead of innerRef.

Environment

  • Package version(s): @blueprintjs/core@5.0.0-alpha.6
  • Operating System: Windows 10
  • Browser name and version: Firefox 113

Code Sandbox

https://codesandbox.io/p/sandbox/tender-brown-wryqy4

Steps to reproduce

  1. Click on popover target button
  2. Click outside popover

Actual behavior

The popover inside StrictMode doesn't close when you click outside the popover

Expected behavior

...it does?

@adidahiya adidahiya added this to the 5.x milestone Jun 22, 2023
@bioc-druzgami
Copy link

bioc-druzgami commented Oct 6, 2023

@adidahiya With datetime dependency issue resolved - this one is the only one blocking me from fully migrating to blueprintjs v5. Any news/ETA regarding that one?

@adidahiya
Copy link
Contributor

@bioc-druzgami I'm not sure, it's an upstream bug in popper.js and it looks like the maintainer has not taken a look at the bugfix PR submitted by @PetrusAsikainen.

If we don't get that PR merged, the other options for us are to either:

  • fork popper.js, or
  • migrate to floating-ui

The latter option sounds like quite a big breaking change that I'm hesitant to take on any time soon given how painful it was to migrate Popover -> Popover2 in Blueprint v4.x for our very large code base.

@adidahiya
Copy link
Contributor

Actually, the third option which is our best path forward: remove our usage of findDOMNode via #3979. So this issue is essentially a duplicate of that one.

@adidahiya
Copy link
Contributor

I forked the original sandbox in this thread and updated to @blueprintjs/core v5.3.3, the issue is still present: https://codesandbox.io/p/sandbox/jovial-framework-75l4w4?file=%2Fpackage.json%3A16%2C5

@PetrusAsikainen
Copy link
Contributor Author

Actually, the third option which is our best path forward: remove our usage of findDOMNode via #3979. So this issue is essentially a duplicate of that one.

Based on my testing the issue was in <Reference innerRef=...>, not related to any usage of findDOMNode. The change that started using the broken react-popper code was cfdcbd3.

@PetrusAsikainen
Copy link
Contributor Author

Perhaps this could be fixed by replacing <Reference innerRef=...> with a mergeRefs call again? I don't see why the code before cfdcbd3 would be a findDOMNode issue.

@dylangrandmont
Copy link
Contributor

dylangrandmont commented Oct 11, 2023

I don't mean to spam this thread but merely as feedback for the library: this is also the only bug I am tracking which blocks my adoption of v5 (which I would very much like to use!). Are other users simply not using strict mode in development?

@bdenhollander
Copy link
Contributor

Since this was the last blocker for a project that was being upgraded to v5, I temporarily switched back to React 17 to workaround the issue.

@adidahiya
Copy link
Contributor

Thanks for the feedback folks, I've bumped the priority on this issue and I believe I've found a workaround that reverts Popover to its behavior before cfdcbd3 while still avoiding findDOMNode elsewhere in the codebase.

I think I switched to using react-popper's innerRef prop in #6137 because I believed React ref callbacks were being deprecated and I was trying to move away from them. But the latest React docs suggest this is not the case and it is safe to keep using them.

Bugfix PR here: #6458. Should land in a release this week if there are no issues in testing the change.

@adidahiya
Copy link
Contributor

Good news: I've verified that the bug is fixed in @blueprintjs/core v5.5.0 using the repro in @PetrusAsikainen's original code sandbox. Here's my forked sandbox: https://codesandbox.io/p/sandbox/gifted-ritchie-rmstyp?file=%2Fsrc%2Fmain.tsx%3A4%2C53

@adidahiya
Copy link
Contributor

Note that React will still print console warnings in strict mode, since we still use findDOMNode in the Overlay component (see #3979):

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants