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

Resolve Turbo Frame navigation bug #1231

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Resolve Turbo Frame navigation bug #1231

wants to merge 1 commit into from

Conversation

seanpdoyle
Copy link
Contributor

The scenario

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

<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:

Copy-paste into `bug.rb`, then execute `ruby bug.rb` 
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.

@seanpdoyle seanpdoyle force-pushed the link-with-frame-child branch 6 times, most recently from 5bf750b to c729a70 Compare March 27, 2024 12:53
@seanpdoyle
Resolve Turbo Frame navigation bug
513a0e0 
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**.
@seanpdoyle
Copy link
Contributor Author

@afcapel could you review this change?

@seanpdoyle
Copy link
Contributor Author

@jorgemanrubia are you able to review this change?

jorgemanrubia
jorgemanrubia approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thanks @seanpdoyle

cc @afcapel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgemanrubia jorgemanrubia jorgemanrubia approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@seanpdoyle @jorgemanrubia