Pass useCapture to @rails/ujs event listener #42512
Conversation
React 17 changed how it attaches events. This change broke @rails/ujs when used within React components. https://reactjs.org/blog/2020/08/10/react-v17-rc.html#fixing-potential-issues suggests using useCapture to resolve this. I've confirmed that this does indeed resolve the issue in my codebase.
| @@ -66,3 +66,4 @@ Rails.delegate = (element, selector, eventType, handler) -> | |||
| if target instanceof Element and handler.call(target, e) == false | |||
| e.preventDefault() | |||
| e.stopPropagation() | |||
| , true | |||
There was a problem hiding this comment.
the linked blog post suggests { capture: true }, is there a difference between that and just true here?
There was a problem hiding this comment.
These docs indicate that they're the same.
There was a problem hiding this comment.
Cool, thanks for confirming.
So if I understand correctly, this change will allow events fired by React 17 to be handled by Rails UJs, same as they were in previous versions of React. And that would apply for the all the events here.
Are there any risks for non-React users?
There was a problem hiding this comment.
Yes, that's correct. As far as risks for non-React users I'm noticing that there are some failing tests surrounding event bubbling. I started to look into them but I'm not all that familiar with @rails/ujs and/or the test suite.
There was a problem hiding this comment.
With a fair bit of tinkering, I got the test results. Will have a think about how to get them to show in CI. For now, here is the qunit output: https://gist.github.com/ghiculescu/02b72bf2826615201db8811a0e04f521
So I think these are the failing tests:
- data-confirm: clicking on the children of a disabled button should not trigger a confirm. (1, 0, 1)
- data-remote: form's submit bindings in browsers that don't support submit bubbling (2, 2, 4)
- data-remote: returning false in form's submit bindings in non-submit-bubbling browsers (1, 0, 1)
There was a problem hiding this comment.
For data-confirm the functionality is working but the test is not. Since capture propagates events from the outermost element in when the event on the disabled button fires we call stopImmediatePropagation which stops the click event from firing on the child as well.
I would imagine the other specs are failing for a similar reason but I haven't been able to narrow it down.
There was a problem hiding this comment.
So I think the other specs are also failing because @rails/ujs is calling stopImmediatePropagation which is causing the events in the spec to not be called. I can't tell if this is actually breaking functionality or if the specs are testing too close to the implementation.
There was a problem hiding this comment.
Is there a way to write the specs better to match this new behaviour?
There was a problem hiding this comment.
Hmm I tried seeing if the behavior was actually broken by testing some of the scenarios in a real application. It appears that things are indeed not quite working as intended. I think the issue is @rails/ujs calls stopPropagation in various places, this was fine with bubbling but with capture it's having some unintended consequences. I feel like in order to enable useCapture a bit of a larger refactor might be in order.
Summary
React 17 changed how it attaches events. This change broke @rails/ujs when used within React components. https://reactjs.org/blog/2020/08/10/react-v17-rc.html#fixing-potential-issues suggests using useCapture to resolve this. I've confirmed that this does indeed resolve the issue in my codebase.