react-focus-on icon indicating copy to clipboard operation
react-focus-on copied to clipboard

React@17 event delegation issue

Open patrykkopycinski opened this issue 2 years ago • 5 comments

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch [email protected] 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.

patrykkopycinski avatar Jul 04 '22 14:07 patrykkopycinski

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

theKashey avatar Jul 08 '22 07:07 theKashey

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 avatar Aug 30 '22 13:08 danielbeutner

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?

danielbeutner avatar Aug 31 '22 12:08 danielbeutner

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.

theKashey avatar Sep 07 '22 06:09 theKashey

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 avatar Sep 07 '22 07:09 danielbeutner

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

danielbeutner avatar Dec 08 '22 08:12 danielbeutner

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.

stale[bot] avatar Apr 30 '23 17:04 stale[bot]