Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add original click event to 'turbo:click' details #611

Merged
merged 2 commits into from
Jul 15, 2022
Merged

Add original click event to 'turbo:click' details #611

merged 2 commits into from
Jul 15, 2022

Conversation

manuelpuyol
Copy link
Contributor

Right now, if you preventDefault a turbo:click, it will still fallback to the original click, causing a full-page reload. Sending the original click event will allow users to completely cancel a click if they need/want to.

applicationAllowsFollowingLinkToLocation(link: Element, location: URL) {
const event = this.notifyApplicationAfterClickingLinkToLocation(link, location)
applicationAllowsFollowingLinkToLocation(link: Element, location: URL, ev: MouseEvent) {
const event = this.notifyApplicationAfterClickingLinkToLocation(link, location, ev)
return !event.defaultPrevented
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative to dispatching the event with a reference to the original, would explicitly preventing the default if event.defaultPrevented serve the same purpose without expanding the surface area of the CustomEvent.detail?

applicationAllowsFollowingLinkToLocation(link: Element, location: URL, mouseEvent: MouseEvent) {
  const event = this.notifyApplicationAfterClickingLinkToLocation(link, location)

  if (event.defaultPrevented) {
    mouseEvent.preventDefault()
    return false
  } else {
    return true
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they are the same behavior. We don't always want to prevent the browser click when preventing turbo:click

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the use cases would be:

  1. I want to prevent Turbo from navigating and let the browser do it -> prevent turbo:click
  2. I want to prevent all navigations -> prevent both events

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that in mind, could we make Frame navigation internally aware of the cancellation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my understanding, this is where we trigger the Frame navigation:

if (this.clickEvent && this.respondsToEventTarget(event.target) && event.target instanceof Element) {
if (this.delegate.shouldInterceptLinkClick(event.target, event.detail.url)) {
this.clickEvent.preventDefault()
event.preventDefault()
this.delegate.linkClickIntercepted(event.target, event.detail.url)
}
}

this is listening to the turbo:click event, so it should be possible for it to know if the event is defaultPrevented. The problem is that the interceptor runs before any addEventListener I add, so the it always evaluates the event first, and it is never prevented.

maybe we could update

turbo/src/core/session.ts

Lines 153 to 159 in 98cdc40

willFollowLinkToLocation(link: Element, location: URL) {
return (
this.elementDriveEnabled(link) &&
locationIsVisitable(location, this.snapshot.rootLocation) &&
this.applicationAllowsFollowingLinkToLocation(link, location)
)
}

to dispatch another event (if the click isn't prevented) that the LinkInterceptor listens to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanpdoyle would adding another internal event make sense here?

@seanpdoyle
Copy link
Contributor

Is this related to #610?

If it is, could we add a test case that exercises a call to event.preventDefault() preventing a frame navigation?

@manuelpuyol
Copy link
Contributor Author

Is this related to #610?

If it is, could we add a test case that exercises a call to event.preventDefault() preventing a frame navigation?

No, this is just something I've been thinking about for some time 😅

@manuelpuyol
Copy link
Contributor Author

@seanpdoyle wdyt of moving forward with only this change and fixing the frame issue in another PR?

* main:
  Drive Browser tests with `playwright` (#609)
  Allow Turbo Streams w/ GET via `data-turbo-stream` (#612)
@dhh dhh merged commit d743085 into hotwired:main Jul 15, 2022
@dhh
Copy link
Member

dhh commented Jul 15, 2022

Let's get a doc PR going to explain this as well for the 7.2.0 release 👍

dhh added a commit to feliperaul/turbo that referenced this pull request Jul 16, 2022
* main:
  Allow frames to scroll smoothly into view (hotwired#607)
  Export Type declarations for `turbo:` events (hotwired#452)
  Add .php as a valid isHTML extension (hotwired#629)
  Add original click event to 'turbo:click' details (hotwired#611)
  Drive Browser tests with `playwright` (hotwired#609)
  Allow Turbo Streams w/ GET via `data-turbo-stream` (hotwired#612)
  Only update history when Turbo visit is renderable (hotwired#601)
  Support development ChromeDriver version overrides (hotwired#606)
  Turbo stream source (hotwired#415)
  Expose Frame load state via `[complete]` attribute (hotwired#487)
  fix(ie/edge): form.method='delete', raises Invalid argument. (hotwired#586)
  Do not declare global types/constants (hotwired#524)
  Defensively create custom turbo elements (hotwired#483)
  Use `replaceChildren` in StreamActions.update (hotwired#534)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants