hotwired / turbo

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

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

Open manuelpuyol opened 2 years ago

manuelpuyol commented 2 years ago

Looking at the turbo:click docs:

Cancel this event to let the click fall through to the browser as normal navigation.

Though if your link triggers a frame navigation, running preventDefault() doesn't cancel it.


document.addEventListener('turbo:click', (e) => e.preventDefault()) on a Visit:

https://user-images.githubusercontent.com/11280312/175405211-c6b112ea-1fd7-4afe-93b8-04cde5e910fa.mov


document.addEventListener('turbo:click', (e) => e.preventDefault()) on a frame:

https://user-images.githubusercontent.com/11280312/175405308-19dcfbe7-de8f-4d88-8d08-72d3af4b943c.mov

manuelpuyol commented 2 years ago

@dhh can we reopen this? #611 doesn't fix the canceling behavior

manuelpuyol commented 1 year ago

this was solved by https://github.com/hotwired/turbo/pull/729

manuelpuyol commented 1 year ago

This was reintroduced by https://github.com/hotwired/turbo/pull/744

@seanpdoyle any reason why the test about this issue was removed? https://github.com/hotwired/turbo/pull/744/files#diff-ba1c7f863678da4856a147f82730481b725f848e033969fe679fc27f1e3263b4L459-L465

seanpdoyle commented 1 year ago

@manuelpuyol that test was introduced between the last stable release, but complicated the process of rolling back the LinkClickObserver changes in https://github.com/hotwired/turbo/pull/744.

With the rollback released, I'm in favor of re-introducing the test coverage and corresponding behavior, if we can!

manuelpuyol commented 1 year ago

@seanpdoyle cool, I can try looking into it...

One question though, why do we always prevent turbo:click for frames?

https://github.com/hotwired/turbo/blob/22fd88a47cfa5d0ed11c8cda13ee8250ff23749b/src/core/frames/link_interceptor.ts#L42

seanpdoyle commented 1 year ago

One question though, why do we always prevent turbo:click for frames?

That is a decision that pre-dates my involvement in the project: https://github.com/hotwired/turbo/blame/v7.0.0-beta.1/src/core/frames/link_interceptor.ts#L39-L40

I'm in favor of experimenting with a different implementation, and support some trial and error with the hopes that the test suite will guide us!

manuelpuyol commented 1 year ago

I'm not sure it's possible to cancel frames only looking at turbo:click with the current architecture 🤔 What was the reason #412 was reverted?

As a possible workaround, we can do something like https://github.com/hotwired/turbo/pull/788, which adds a new turbo:frame-click event that controls frame clicks and can be canceled 🤷

TAR5 commented 8 months ago

before-visit events, that are caused by promoted frame links, are also not cancellable.

<a href="/myframepage" data-turbo-frame="myframe" data-turbo-action="advance">Frame link promoted to visit</a>

<script>
document.addEventListener("turbo:before-visit", function(event) {
    alert('visit!');
    event.preventDefault(); // prevents regular visit but does not prevent promoted frame load 
});
</script>