bvaughn / react-resizable-panels

https://react-resizable-panels.vercel.app/
MIT License
3.99k stars 144 forks source link

Stacking Order Error for Unrelated events #409

Closed exoRift closed 1 month ago

exoRift commented 1 month ago

After looking at the source code, it seems like for handle calculation, the library attaches a pointerup listener to the window rather than the handle. This causes an issue with clicking on disappearing elements (for instance, ShadCN UI's select element) where every so often, clicking on a disappearing element will cause an unhandled runtime error because the nonexistent element and the registered panels don't share an ancestor.

To reproduce, have an existing panel group and use the ShadCN Select component (click on a few items a bunch of times, this error doesn't happen every time)

"react-resizable-panels": "^2.0.23",

bvaughn commented 1 month ago

Is this an issue with ShadCN? I'm not really interested in trying to support random third party libraries.

exoRift commented 1 month ago

This is not an issue with ShadCN. It's simply the way I found the bug which occurs on element-disappearing behavior. I don't know how to specifically reproduce it in isolation which is why I bring up ShadCN as a way to see it for yourself.

(I also have no way to catch this error and ignore it due to the way it's being thrown)

Might I ask why you're listening on the global pointerUp event instead of scoping it to the handle element, perchance?

bvaughn commented 1 month ago

I see. Bug repros are more useful when all of the non-essential bits are removed. So if ShadCN isn't required, it's probably best not to mention it in the repro. (If it was required, there would seem to be a good chance that it caused or contributed to the bug.)

Might I ask why you're listening on the global pointerUp event instead of scoping it to the handle element, perchance?

This library supports a kind of invisible gutter where a pointer event that's near to a resize handle, but not exactly on top of it, will still be handled. This enables better interactions on touch devices (IMO) and it better allows for multiple intersecting handles to be dragged together (see #268).

That's one of the reasons why pointer events aren't attached to the handles themselves.

exoRift commented 1 month ago

I tried replicating it without ShadCN but unfortunately could not achieve the error. I did some thinking, however, and I have a theory that it could possibly be the way that RadixUI handles animations which is (I think) to move the element (or a clone of the element) to toplevel.

I'm not super immersed in the inner workings of your library or Radix, however, unfortunately. I wish I could be of more help

I can't let this go, though, since this bug has happened in production and there's no way to ignore the error as it causes an unhandled rejection that cannot be caught in NextJS

bvaughn commented 1 month ago

I'm fairly suspicious of ShadCN being the cause of this, as I've gotten other bug reports in the past that are related to that library.

I understand that you need to fix this, but I'm not going to be able to help. I don't have the time.

exoRift commented 1 month ago

Is there any chance you could release a patch that adds a skipAssertions option that will just return instead of throwing, perchance?

bvaughn commented 1 month ago

I don't really know what you're describing but I'm probably not interested in releasing workarounds like this, sorry.

bvaughn commented 1 month ago

Honestly you might be better off to just fork this library for your use case :) It's MIT licensed so you can do that!

exoRift commented 1 month ago

I understand. However, I'd still like to shed context on this as I discover more.

So the specific circumstance that this happens under is when I have the select in a dialog (allowing its dropdown to overlap the handle).

If you position your cursor on an option (but over where the handle is) and click on the option, the option disappears (and is no longer under #document) and the pointerup event runs.

A band-aid solution that I found is to assign { capture: true } to the pointerup listener which causes it to run before the element is deleted. This is a bad practice, though

bvaughn commented 1 month ago

I appreciate you adding the extra context here for others in the future.

I'm going to close this issue for now since I don't plan to make any changes in response to it. 🙇🏼