Ignore UJS `<a>` clicks and `<form>` submissions #744

seanpdoyle · 2022-09-28T15:57:42Z

Closes #743

The change in behavior can be traced back to hotwired/turbo#412.
When the overlap between the LinkInterceptor and the
LinkClickObserver were unified, the technique used by the
LinkInterceptor to prevent duplicate event handlers from responding to
the same click event was changed in a subtle way.

In its place, this commit changes the LinkClickObserver and
FormSubmitObserver to ignore <a> clicks and <form> submissions if
they're annotated with [data-remote="true"].

To exercise these edge cases, this commit also adds a ujs.html fixture
file along with a ujs_tests.ts module to cover situations when
@rails/ujs and @hotwired/turbo co-exist.

seanpdoyle · 2022-09-28T16:12:00Z

I've converted this to a draft, since I'm not sure about the intended behavior.

While the test coverage ensures that there is only a single request issued, I'm not entirely sure about how @rails/ujs and @hotwired/turbo should co-exist in this scenario.

The test asserts that clicking <a data-remote="true"> would drive the frame. From a turbo-centric perspective, that feels correct, but the fact that the Turbo event listeners respond to the click event before the UJS handlers feels incidental instead of deliberate.

This covers an edge-case scenario for an application in the midst of a migration from UJS to Turbo. Given that rendering an <a data-remote="true"> element requires opt-in, the intention behind the element are likely at odds with driving a <turbo-frame> element.

To quote the original issue opened by @acetinick:

*the [...] turbo request, which is NOT expected resulting in an UnknownFormat exception which trying to render a .html format, but we only have a .js.erb view
the [...] UJS request, which is expected to return JS to open a modal window

Given those expectations, Turbo should lose the race against UJS for handling the event.

Since 7.1.0 supported the expected behavior, finding a way to revert #412 (incorporating changes made since its merge) might be the most backwards-compatible solution.

acetinick · 2022-09-28T18:15:53Z

This amazing, I'll try it out and let you know.

I guess for large applications like ours it's impossible to switch so easily, it's a slow transition screen by screen. We know we need to pull the pin eventually but for a point release of turbo it's hard for us to refactor soo many existing UJS methods, which are in the hundreds.

I think what always was the goal of turbo was move away from UJS which we agree. Baby steps. For a lot of large apps, it's a few years to rid completely. Like a jquery that sticks around

acetinick · 2022-09-29T08:01:30Z

To add to your comment about expectations, I believe it would be safe to assume that any link with data-remote should be ignored from turbo link intercepts completely.

If anyone is using data-remote they will not be expecting turbo to do anything with this request.

I believe same expectation will go for all remote situations of links/buttons and forms.

It's either one or the other.

Closes hotwired#743 The change in behavior can be traced back to [hotwired#412][]. When the overlap between the `LinkInterceptor` and the `LinkClickObserver` were unified, the technique used by the `LinkInterceptor` to prevent duplicate event handlers from responding to the same `click` event was changed in a subtle way. In its place, this commit changes the `LinkClickObserver` and `FormSubmitObserver` to ignore `<a>` clicks and `<form>` submissions if they're annotated with `[data-remote="true"]`. To exercise these edge cases, this commit also adds a `ujs.html` fixture file along with a `ujs_tests.ts` module to cover situations when `@rails/ujs` and `@hotwired/turbo` co-exist. [hotwired#412]: hotwired#412

acetinick · 2022-10-01T08:23:33Z

@dhh when you get chance could you review and if possible do a release on this. It's a blocker for anyone still using data-remote to upgrade to 7.2

kevinmcconnell · 2022-10-04T11:52:51Z

I think the situation is a little more complicated, because in Turbo 7.1, forms marked with data-remote="true" are submitted by Turbo (and not UJS) when they're inside a <turbo-frame>. It seems like an edge case, but it can be easy to run into if using config.action_view.form_with_generates_remote_forms = true in a Rails app. We've run into a handful of occurrences of this already.

In order to not break existing applications when they upgrade, we'd need to restore that behaviour.

Here's a minimal example to reproduce the original behaviour. On this page, the form submits via Turbo, and the link uses UJS.

<!doctype html>
<html>

<head>
  <meta charset="utf-8">
  <title>Test</title>

  <script src="https://cdn.jsdelivr.net/npm/rails-ujs@5.2.8-1/lib/assets/compiled/rails-ujs.min.js"></script>
  <script src="https://cdn.jsdelivr.net/npm/@hotwired/turbo@7.1.0/dist/turbo.es2017-umd.min.js"></script>
</head>

<body>
  <turbo-frame id="frame">
    <form data-remote="true">
      <button>Submit</button>
    </form>

    <a data-remote="true" href="/">Click</a>
  </turbo-frame>
</body>

</html>

When the above code is using Turbo 7.2, both the link and the form issue duplicate requests. With this PR, both link & form will use UJS, which would be a change from 7.1.

jeremy · 2022-10-04T18:57:07Z

When the above code is using Turbo 7.2, both the link and the form issue duplicate requests. With this PR, both link & form will use UJS, which would be a change from 7.1.

True, but… seems worth breaking while it's still undefined behavior? Considering the same markup would then behave differently in & out of a frame.

Curious whether turbo-rails could manage Rails UJS integration. If Turbo offered a hook or delegate call for users to make this determination, then turbo-rails could use it to check for UJS conflict (and emit deprecation warnings, throw errors, etc.)

seanpdoyle · 2022-10-04T19:14:22Z

@jeremy I agree that package is a more appropriate home for UJS-specific code. I've previously explored what that might entail in hotwired/turbo-rails#93 and hotwired/turbo-rails#257.

If Turbo offered a hook or delegate call for users to make this determination

Applications can call event.preventDefault() on turbo:click events to opt-out of Turbo integration. Could a similar mechanism on turbo:submit-start support opting out of Form Submissions?

Then, Turbo Rails could define event listeners like:

// support already exists in Turbo
addEventListener("turbo:click", (event) => {
  const remote = event.target.closest('[data-remote="true"]')

  if (remote) event.preventDefault()
})

// support does not yet exist in Turbo
addEventListener("turbo:submit-start", (event) => {
  const remote = event.target.closest('[data-remote="true"]')

  if (remote) event.preventDefault()
})

kevinmcconnell · 2022-10-04T20:22:01Z

True, but… seems worth breaking while it's still undefined behavior?

The downside to breaking it is that upgrading 7.2 would then require people to update any forms inside <turbo-frame>s that had been marked remote: true, as they would now be handled differently. And as I say, that sounds like it would be an edge case, but given that many people will have been adding remote: true to their forms by default, it's likely to come up quite a bit.

I agree that the inconsistency is not great, but we could choose to preserve existing behaviour while also adding a nicer path for people to resolve the conflict on their own time. As you say, adding things like deprecation warnings would be helpful.

We can prevent the duplicate form submission by adding back the stopImmediatePropagation that was in 7.1.

Applications can call event.preventDefault() on turbo:click events to opt-out of Turbo integration

The ability to do this on click events is what changed in #412, I believe, which results in the double submissions from clicks in frames. The event handlers moving to the frame element itself means cancelling the event on document no longer works. But if we restored that behaviour, we could stop the double submissions without having UJS & Turbo need to be aware of each other at all. I think that's a nicer way to do it than Turbo having knowledge of data-remote. It also means that if there's anyone who has Turbo co-existing with some library other than UJS, it's less likely to break for them.

Personally I think restoring the behaviour of both those things would be the best option, since it'll make the upgrade path easier for folks. But I also agree that turbo-rails should provide help here, whether through deprecation warnings or (opt-in) errors where both UJS & Rails are competing, to help people work through updating their forms to be unambiguously one or the other.

dhh · 2022-10-05T13:15:13Z

Yeah, I'd very much like to see a quick 7.2.1 shipped with a solution to this, so we don't create an expectation of the new timing being something you can rely on, when we're changing it back to ensure compatibility 👍

This reverts commit 9bef653.

[comment]: hotwired#744 (comment) [FormInterceptor]: https://github.com/hotwired/turbo/blob/v7.1.0/src/core/frames/form_interceptor.ts#L31

seanpdoyle · 2022-10-06T20:23:14Z

I've pushed up changes that:

revert Replace LinkInterceptor with LinkClickObserver #412
adjust the restored LinkInterceptor to accommodate tangential changes made since it was removed
Restore behavior for <form data-remote="true"> elements nested within <turbo-frame> from 7.1.0 based on Ignore UJS <a> clicks and <form> submissions #744 (comment) by adding a call to event.stopImmediatePropagation() to mimic the FormInterceptor that existed in 7.1.0

kevinmcconnell · 2022-10-07T14:14:38Z

This looks great @seanpdoyle -- appreciate you adding it 🙏 I've started testing this out in our apps here and so far it certainly seems to fix the regressions we'd seen related to UJS.

seanpdoyle force-pushed the fix-issue-743 branch from c131833 to 1213774 Compare September 28, 2022 15:58

seanpdoyle mentioned this pull request Sep 28, 2022

7.2 data-remote inside turbo-frame produces double requests #743

Closed

seanpdoyle marked this pull request as draft September 28, 2022 16:03

seanpdoyle force-pushed the fix-issue-743 branch from 1213774 to 3db3efd Compare September 30, 2022 18:44

seanpdoyle marked this pull request as ready for review September 30, 2022 18:45

seanpdoyle force-pushed the fix-issue-743 branch from 3db3efd to 5871173 Compare September 30, 2022 18:46

seanpdoyle changed the title ~~Prevent double requests from within turbo-frame~~ Ignore UJS <a> clicks and <form> submissions Sep 30, 2022

Revert "Replace LinkInterceptor with LinkClickObserver"

c212ae7

This reverts commit 9bef653.

seanpdoyle force-pushed the fix-issue-743 branch from 66e33f1 to 10988ec Compare October 6, 2022 19:56

seanpdoyle added 2 commits October 6, 2022 16:00

adjust after reverting hotwired#412

fa20fcd

Restore <form data-remote="true">+<turbo-frame> behavior from 7.1.0

ab5558a

[comment]: hotwired#744 (comment) [FormInterceptor]: https://github.com/hotwired/turbo/blob/v7.1.0/src/core/frames/form_interceptor.ts#L31

seanpdoyle force-pushed the fix-issue-743 branch 2 times, most recently from d324cb9 to ab5558a Compare October 6, 2022 20:05

dhh merged commit 2345af9 into hotwired:main Oct 8, 2022

manuelpuyol mentioned this pull request Nov 4, 2022

Canceling turbo:click does not stop a frame navigation. #610

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore UJS `<a>` clicks and `<form>` submissions #744

Ignore UJS `<a>` clicks and `<form>` submissions #744

seanpdoyle commented Sep 28, 2022 •

edited

Loading

seanpdoyle commented Sep 28, 2022

acetinick commented Sep 28, 2022

acetinick commented Sep 29, 2022

acetinick commented Oct 1, 2022

kevinmcconnell commented Oct 4, 2022

jeremy commented Oct 4, 2022

seanpdoyle commented Oct 4, 2022 •

edited

Loading

kevinmcconnell commented Oct 4, 2022

dhh commented Oct 5, 2022

seanpdoyle commented Oct 6, 2022

kevinmcconnell commented Oct 7, 2022

Ignore UJS <a> clicks and <form> submissions #744

Ignore UJS <a> clicks and <form> submissions #744

Conversation

seanpdoyle commented Sep 28, 2022 • edited Loading

seanpdoyle commented Sep 28, 2022

acetinick commented Sep 28, 2022

acetinick commented Sep 29, 2022

acetinick commented Oct 1, 2022

kevinmcconnell commented Oct 4, 2022

jeremy commented Oct 4, 2022

seanpdoyle commented Oct 4, 2022 • edited Loading

kevinmcconnell commented Oct 4, 2022

dhh commented Oct 5, 2022

seanpdoyle commented Oct 6, 2022

kevinmcconnell commented Oct 7, 2022

Ignore UJS `<a>` clicks and `<form>` submissions #744

Ignore UJS `<a>` clicks and `<form>` submissions #744

seanpdoyle commented Sep 28, 2022 •

edited

Loading

seanpdoyle commented Oct 4, 2022 •

edited

Loading