Skip to content

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

Merged
dhh merged 2 commits intohotwired:mainfrom
manuelpuyol:add-click-event-to-turbo-click
Jul 15, 2022
Merged

Add original click event to 'turbo:click' details#611
dhh merged 2 commits intohotwired:mainfrom
manuelpuyol:add-click-event-to-turbo-click

Conversation

@manuelpuyol
Copy link
Copy Markdown
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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 pushed 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