theKashey / react-focus-on

🎯 Solution for WAI ARIA compatible modal dialogs or full-screen tasks, you were looking for
MIT License
340 stars 16 forks source link

React@17 event delegation issue #61

Closed patrykkopycinski closed 1 year ago

patrykkopycinski commented 2 years ago

Hi! πŸ‘‹

Firstly, thanks for your work on this project! πŸ™‚

Today I used patch-package to patch react-focus-on@3.5.4 for the project I'm working on.

Here is the diff that solved my problem:

diff --git a/node_modules/react-focus-on/dist/es2015/Effect.js b/node_modules/react-focus-on/dist/es2015/Effect.js
index 06fe55c..1994a65 100644
--- a/node_modules/react-focus-on/dist/es2015/Effect.js
+++ b/node_modules/react-focus-on/dist/es2015/Effect.js
@@ -50,15 +50,15 @@ export function Effect(_a) {
             mouseTouches.current = event.touches.length;
         };
         if (activeNode) {
-            document.addEventListener('keydown', onKeyDown);
-            document.addEventListener('mousedown', onMouseDown);
-            document.addEventListener('touchstart', onTouchStart);
-            document.addEventListener('touchend', onTouchEnd);
+            document.addEventListener('keydown', onKeyDown, { capture: true });
+            document.addEventListener('mousedown', onMouseDown, { capture: true });
+            document.addEventListener('touchstart', onTouchStart, { capture: true });
+            document.addEventListener('touchend', onTouchEnd, { capture: true });
             return function () {
-                document.removeEventListener('keydown', onKeyDown);
-                document.removeEventListener('mousedown', onMouseDown);
-                document.removeEventListener('touchstart', onTouchStart);
-                document.removeEventListener('touchend', onTouchEnd);
+                document.removeEventListener('keydown', onKeyDown, { capture: true });
+                document.removeEventListener('mousedown', onMouseDown, { capture: true });
+                document.removeEventListener('touchstart', onTouchStart, { capture: true });
+                document.removeEventListener('touchend', onTouchEnd, { capture: true });
             };
         }
     }, [activeNode, onClickOutside, onEscapeKey]);

This issue body was partially generated by patch-package.

theKashey commented 2 years ago

Wondering what was the problem you've solved this way. This code is fighting mostly with browser behavior, not react.

danielbeutner commented 2 years ago

This patch works for me as well. I used it for modal and the click on a backdrop went through the underlying element and "tapped" it.

danielbeutner commented 2 years ago

Sorry for the confusion but the patch did not work (somehow it worked as I wrote the last comment).

I took a video of the issue to visualize the problem. Screen Recording 2022-08-31 at 14 28 52

I tried to debug the issue but I can't solve it. But I figured out it doesn't work if it's a TouchEvent. On a MouseEvent it works as intended.

Do you have a hint to solve this?

theKashey commented 2 years ago

Ok. So the issue is that you can "click-through" the backdrop. Now we are talking 😼

Just a quick check - is it working on production mode as well? πŸ˜… There are some presently discovered "race conditions" caused by StrictMode/double-rendering.

danielbeutner commented 2 years ago

Ok. So the issue is that you can "click-through" the backdrop. Now we are talking 😼

Exactly.

Just a quick check - is it working on production mode as well?

The issue is still on production and was filed from one of our beloved testers.

https://user-images.githubusercontent.com/531853/188813694-c6a08430-18e3-4787-a523-cef4d3e317a9.mov

There are some presently discovered "race conditions" caused by StrictMode/double-rendering.

Seems plausible. In my case I am not using React.StrictMode in the app.

If you need more info just drop me a line.

danielbeutner commented 1 year ago

Are there any news? Do you need more info on this?

stale[bot] commented 1 year ago

This issue has been marked as "stale" because there has been no activity for 2 months. If you have any new information or would like to continue the discussion, please feel free to do so. If this issue got buried among other tasks, maybe this message will reignite the conversation. Otherwise, this issue will be closed in 7 days. Thank you for your contributions so far.