Skip to content

Commit

Permalink
Resolve Turbo Frame navigation bug
Browse files Browse the repository at this point in the history
The scenario
---

Imagine a List-Details style page layout, with navigation links on the
left and the contents of the page on the right:

```html
<turbo-frame id="list">
  <a href="/articles/1" data-turbo-frame="details">
    Article #1

    <turbo-frame id="preview_article_1" src="/articles/1/preview">
      Some preview text
    </turbo-frame>
  </a>
  <!-- ... -->

  <a href="/?page=2">Next Page</a>
</turbo-frame>

<turbo-frame id="details">
  <h1>Details</h1>
</turbo-frame>
```

The `#list` element is a `<turbo-frame>` to handle pagination, and the
`#details` element is a `<turbo-frame>` as well. The `<a>` elements
within the `#list` frame drive the `#details` frame through their
`[data-turbo-frame="details"]`. The `<a>` element nests a third kind of
`<turbo-frame>` that is specific to the resource being linked to. It
asynchronously loads in preview content with a `[src]` attribute.

The problem
---

Clicking the `Article #1` text within the `<a>` element drives the
`#details` frame as expected.

However, clicking the `Some preview text` within the `<a>` element's
nested `<turbo-frame>` element navigates the entire page.

This is demonstrated in the following reproduction script:

```ruby
require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails"
  gem "propshaft"
  gem "puma"
  gem "sqlite3"
  gem "turbo-rails"

  gem "capybara"
  gem "cuprite", "~> 0.9", require: "capybara/cuprite"
end

ENV["DATABASE_URL"] = "sqlite3::memory:"
ENV["RAILS_ENV"] = "test"

require "active_record/railtie"
require "action_controller/railtie"
require "action_view/railtie"
require "action_cable/engine"
require "rails/test_unit/railtie"

class App < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f

  config.root = __dir__
  config.hosts << "example.org"
  config.eager_load = false
  config.session_store :cookie_store, key: "cookie_store_key"
  config.secret_key_base = "secret_key_base"
  config.consider_all_requests_local = true
  config.action_cable.cable = {"adapter" => "async"}
  config.turbo.draw_routes = false

  Rails.logger = config.logger = Logger.new($stdout)

  routes.append do
    root to: "application#index"
  end
end

Rails.application.initialize!

ActiveRecord::Schema.define do
  create_table :messages, force: true do |t|
    t.text :body, null: false
  end
end

class Message < ActiveRecord::Base
end

class ApplicationController < ActionController::Base
  include Rails.application.routes.url_helpers

  class_attribute :template, default: DATA.read

  def index
    render inline: template, formats: :html
  end
end

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
  driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: {
    js_errors: true,
    headless: false
  }
end

Capybara.configure do |config|
  config.server = :puma, {Silent: true}
  config.default_normalize_ws = true
end

require "rails/test_help"

class TurboSystemTest < ApplicationSystemTestCase
  test "reproduces bug" do
    visit root_path

    binding.irb
    click_link "Drive #details to ?key=1"
  end
end

__END__

<!DOCTYPE html>
<html>
  <head>
    <%= csrf_meta_tags %>

    <script type="importmap">
      {
        "imports": {
          "@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>"
        }
      }
    </script>

    <script type="module">
      import "@hotwired/turbo-rails"
    </script>

    <style>
      body {
        display: grid;
        grid-template-areas:
          "list details"
          "list details";
        grid-template-columns: 150px 1fr;
        grid-template-rows: 50px 1fr;
        height: 100vh;
      }

      #list {
        display: flex;
        flex-direction: column;
        grid-area: list;
        overflow-y: scroll;
      }

      #details {
        grid-area: details;
      }
    </style>
    <meta name="turbo-prefetch" content="false">
  </head>

  <body>
    <turbo-frame id="list">
      <% 1.upto(5).each do |key| %>
        <%= link_to({key:}, data: {turbo_frame: "details"}) do %>
          <turbo-frame id="frame_<%= key %>">
            Drive #details to <%= {key:}.to_query %>
          </turbo-frame>
        <% end %>
      <% end %>
    </turbo-frame>

    <turbo-frame id="details">
      <%= params.fetch(:key, "0") %>
    </turbo-frame>
  </body>
</html>
```

The solution
---

When observing `click` events, utilize the `findLinkFromClickTarget` to
find the nearest `<a>` element to the `click` target so that **that**
element's ancestors are used to determine which `<turbo-frame>` to
target instead of risking the possibility of using one of its
**descendants**.
  • Loading branch information
seanpdoyle committed Mar 27, 2024
1 parent 600203e commit c729a70
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/core/frames/link_interceptor.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { findLinkFromClickTarget } from "../../util"

export class LinkInterceptor {
constructor(delegate, element) {
this.delegate = delegate
Expand All @@ -17,15 +19,15 @@ export class LinkInterceptor {
}

clickBubbled = (event) => {
if (this.respondsToEventTarget(event.target)) {
if (this.clickEventIsSignificant(event)) {
this.clickEvent = event
} else {
delete this.clickEvent
}
}

linkClicked = (event) => {
if (this.clickEvent && this.respondsToEventTarget(event.target) && event.target instanceof Element) {
if (this.clickEvent && this.clickEventIsSignificant(event)) {
if (this.delegate.shouldInterceptLinkClick(event.target, event.detail.url, event.detail.originalEvent)) {
this.clickEvent.preventDefault()
event.preventDefault()
Expand All @@ -39,8 +41,10 @@ export class LinkInterceptor {
delete this.clickEvent
}

respondsToEventTarget(target) {
const element = target instanceof Element ? target : target instanceof Node ? target.parentElement : null
clickEventIsSignificant(event) {
const target = event.composed ? event.target?.parentElement : event.target
const element = findLinkFromClickTarget(target) || target

return element && element.closest("turbo-frame, html") == this.element
}
}
3 changes: 3 additions & 0 deletions src/tests/fixtures/frames.html
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ <h2>Frames: #frame</h2>
</form>
</turbo-frame>
<a id="outside-frame-form" href="/src/tests/fixtures/frames/form.html" data-turbo-frame="frame">Navigate #frame to /frames/form.html</a>
<a id="outside-frame-link-with-frame-child" href="/src/tests/fixtures/frames/form.html" data-turbo-frame="frame">
<turbo-frame id="ignored-frame">Has a turbo-frame child</turbo-frame>
</a>

<a id="link-outside-frame-action-advance" href="/src/tests/fixtures/frames/frame.html" data-turbo-frame="frame" data-turbo-action="advance">Navigate #frame from outside with a[data-turbo-action="advance"]</a>
<form id="form-get-frame-action-advance" action="/__turbo/redirect" data-turbo-frame="frame" data-turbo-action="advance">
Expand Down
13 changes: 13 additions & 0 deletions src/tests/functional/frame_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,19 @@ test("'turbo:frame-render' is triggered after frame has finished rendering", asy
assert.include(fetchResponse.response.url, "/src/tests/fixtures/frames/part.html")
})

test("navigating a frame from an outer link with a turbo-frame child fires events", async ({ page }) => {
await page.click("#outside-frame-link-with-frame-child")

await nextEventOnTarget(page, "frame", "turbo:before-fetch-request")
await nextEventOnTarget(page, "frame", "turbo:before-fetch-response")
const { fetchResponse } = await nextEventOnTarget(page, "frame", "turbo:frame-render")
expect(fetchResponse.response.url).toContain("/src/tests/fixtures/frames/form.html")

await nextEventOnTarget(page, "frame", "turbo:frame-load")

expect(await readEventLogs(page), "no more events").toHaveLength(0)
})

test("navigating a frame from an outer form fires events", async ({ page }) => {
await page.click("#outside-frame-form")

Expand Down

0 comments on commit c729a70

Please sign in to comment.