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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93cd951
Add original click event to 'turbo:click' details
manuelpuyol Jun 23, 2022
f4280ab
Merge branch 'main' into pr/611
dhh Jul 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/core/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,11 @@ export class Session

// Link click observer delegate

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

Expand Down Expand Up @@ -300,8 +300,8 @@ export class Session

// Application events

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:

turbo/src/core/frames/link_interceptor.ts

Lines 37 to 43 in 98cdc40

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?

}

Expand All @@ -310,10 +310,10 @@ export class Session
return !event.defaultPrevented
}

notifyApplicationAfterClickingLinkToLocation(link: Element, location: URL) {
notifyApplicationAfterClickingLinkToLocation(link: Element, location: URL, event: MouseEvent) {
return dispatch("turbo:click", {
target: link,
detail: { url: location.href },
detail: { url: location.href, originalEvent: event },
cancelable: true,
})
}
Expand Down
4 changes: 2 additions & 2 deletions src/observers/link_click_observer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expandURL } from "../core/url"

export interface LinkClickObserverDelegate {
willFollowLinkToLocation(link: Element, location: URL): boolean
willFollowLinkToLocation(link: Element, location: URL, event: MouseEvent): boolean
followedLinkToLocation(link: Element, location: URL): void
}

Expand Down Expand Up @@ -38,7 +38,7 @@ export class LinkClickObserver {
const link = this.findLinkFromClickTarget(target)
if (link) {
const location = this.getLocationForLink(link)
if (this.delegate.willFollowLinkToLocation(link, location)) {
if (this.delegate.willFollowLinkToLocation(link, location, event)) {
event.preventDefault()
this.delegate.followedLinkToLocation(link, location)
}
Expand Down