From 349f983c2d73926f53141c37371bc75956f75b48 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Tue, 26 Mar 2024 23:17:08 -0400 Subject: [PATCH] Resolve Turbo Frame navigation bug 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 Article #1 Some preview text Next Page

Details

 ``` The `#list` element is a `` to handle pagination, and the `#details` element is a `` as well. The `` elements within the `#list` frame drive the `#details` frame through their `[data-turbo-frame="details"]`. The `` element nests a third kind of `` 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 `` element drives the `#details` frame as expected. However, clicking the `Some preview text` within the `` element's nested `` 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__ <%= csrf_meta_tags %> <% 1.upto(5).each do |key| %> <%= link_to({key:}, data: {turbo_frame: "details"}) do %> Drive #details to <%= {key:}.to_query %> <% end %> <% end %> <%= params.fetch(:key, "0") %> ``` The solution --- When observing `click` events, utilize the `findLinkFromClickTarget` to find the nearest `` element to the `click` target so that **that** element's ancestors are used to determine which `` to target instead of risking the possibility of using one of its **descendants**. --- src/core/frames/link_interceptor.js | 11 ++++++++++- src/tests/fixtures/frames.html | 3 +++ src/tests/functional/frame_tests.js | 13 +++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/core/frames/link_interceptor.js b/src/core/frames/link_interceptor.js index 9b9249544..235383c06 100644 --- a/src/core/frames/link_interceptor.js +++ b/src/core/frames/link_interceptor.js @@ -1,3 +1,5 @@ +import { findLinkFromClickTarget } from "../../util" + export class LinkInterceptor { constructor(delegate, element) { this.delegate = delegate @@ -40,7 +42,14 @@ export class LinkInterceptor { } respondsToEventTarget(target) { - const element = target instanceof Element ? target : target instanceof Node ? target.parentElement : null + let element + + if (target instanceof Element) { + element = findLinkFromClickTarget(target) || target + } else if (target instanceof Node) { + element = target.parentElement + } + return element && element.closest("turbo-frame, html") == this.element } } diff --git a/src/tests/fixtures/frames.html b/src/tests/fixtures/frames.html index 24c3272b9..0513e911a 100644 --- a/src/tests/fixtures/frames.html +++ b/src/tests/fixtures/frames.html @@ -37,6 +37,9 @@

Frames: #frame

 Navigate #frame to /frames/form.html + + Has a turbo-frame child + Navigate #frame from outside with a[data-turbo-action="advance"]
diff --git a/src/tests/functional/frame_tests.js b/src/tests/functional/frame_tests.js index f1925908b..3b0770c39 100644 --- a/src/tests/functional/frame_tests.js +++ b/src/tests/functional/frame_tests.js @@ -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")