hotwired / turbo

The speed of a single-page web application without having to write any JavaScript
https://turbo.hotwired.dev
MIT License
6.54k stars 415 forks source link

Fix turbo-confirm for links without a turbo-method #1266

Open tpaulshippy opened 1 month ago

tpaulshippy commented 1 month ago

Resolves https://github.com/hotwired/turbo/issues/1264

It appears that the turbo-confirm feature implementation relies on the link being captured by the FormLinkClickObserver rather than the LinkClickObserver. Without a data-turbo-method (or data-turbo-stream) attribute, the FormLinkClickObserver will ignore the link and the confirm feature will not fire.

This small bug fix adds the data-turbo-confirm attribute to the other two attributes that indicate to the FormLinkClickObserver that it should handle the link.

tpaulshippy commented 1 month ago

Seeing one flaky test failure.

  [firefox] › functional/navigation_tests.js:432:1 › does not fire turbo:load twice after following a redirect 

But it looks like this is happening in the main branch as well.

tpaulshippy commented 1 month ago

https://github.com/hotwired/turbo/pull/379#issuecomment-913811333 Looks like the current behavior was intended and by design. If this is still the case, perhaps we just need an update to https://github.com/hotwired/turbo-site/blob/main/_source/handbook/02_drive.md with this detail.

It does seem odd to me that data-turbo-method="GET" is necessary to make data-turbo-confirm work.

pinzonjulian commented 1 month ago

The original intention of the confirm dialog as I understand it is to alert the user about operations that will modify the state of the system. In the case of the web that's requests with a PUT/PATCH/DELETE verb.

A GET request, which is how links work by default, shouldn't modify your system so a confirmation seems odd.

When you explicitly tell a link to use another method (a technique that's more and more uncommon these days) then you are allowed to ask the user for confirmation.

What's your use case for asking a user for confirmation on a GET request?

tpaulshippy commented 1 month ago

I don't personally have a use case. I'm sure @JaceBayless does.

But the one documented at https://turbo.hotwired.dev/handbook/drive#requiring-confirmation-for-a-visit seems reasonable enough to me: "Do you want to leave this page?"

tpaulshippy commented 1 month ago

Actually a very common use case just happened for me. I started to edit my comment, and then decided to make a new comment. When I clicked "Cancel," Github asked me to confirm that I wanted to discard my changes.

Any Cancel click on a dirty new/edit form will not modify the system at all and could be constructed as a GET, but a confirmation is very appropriate.

pinzonjulian commented 1 month ago

You're right! That's a great use case.

tpaulshippy commented 1 month ago

The more I think about it, the more I think this implementation doesn't really fit that particular use case. For a couple of reasons: 1) You generally want to do a dirty check in these cases which would usually require additional Javascript before deciding to confirm. 2) You would want the confirm to fire on close of the window/tab as well as any navigation away so a beforeunload event would better cover that. Would be nice if Turbo could handle that but I'm not sure how.

There still could be other use cases for a confirm on navigate though. Maybe you have a set of filter controls and the user has built up a set of filters. Then you have a Clear Filters button that just navigates back to the list action without query strings. You might only show that button if query strings exist (the "dirty check"). In that case you probably wouldn't want to confirm on close of the window because there isn't really a potential for data loss.

I'm sure there are others.

JaceBayless commented 1 month ago

In what I was doing, I didn't have a great use case. I was testing some things out and noticed that the documentation didn't match the actual behavior.

FWIW I think one of the main uses for this would be if you're trying to prevent people from navigating to external sites, generally they do this with a warning message.

marcoroth commented 1 month ago

FWIW, this is a duplicate of https://github.com/hotwired/turbo/pull/874

tpaulshippy commented 1 month ago

True. The implementation approach is quite different though. How would you compare them?

marcoroth commented 1 month ago

I'm wondering if your approach also works for form submissions

tpaulshippy commented 1 month ago

Were form submissions broken? I thought it was just links.

tpaulshippy commented 1 month ago

I think my approach just turns the links into form submissions if they have a confirmation.