Turbo 8 compatibility

sfnelson · sfnelson · commit 9fcb1916c4ff · 2024-02-21T12:41:16.000+10:30
This is a breaking change – Turbo 8 requires a full HTML response to
Frame requests, and the response head element needs to have the same
data-turbo-track items as the current page or the frame will not be
able to advance history.

This change introduces a replacement for the default layout lambda
that Turbo uses to achieve this, which will return a kpop/frame
layout automatically for kpop frame requests.

This is a breaking change because the layout needs to include all
of the tracked header elements from the application, so we are
changing the name of the layout to help ensure this happens.
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -1,9 +1,9 @@
 PATH
   remote: .
   specs:
-    katalyst-kpop (3.0.2)
+    katalyst-kpop (3.1.0)
       katalyst-html-attributes
-      turbo-rails (< 2)
+      turbo-rails
       view_component
 
 GEM
@@ -321,7 +321,7 @@ GEM
     stringio (3.1.0)
     thor (1.3.0)
     timeout (0.4.1)
-    turbo-rails (1.5.0)
+    turbo-rails (2.0.3)
       actionpack (>= 6.0.0)
       activejob (>= 6.0.0)
       railties (>= 6.0.0)
diff --git a/README.md b/README.md
@@ -54,12 +54,13 @@ kpop provides helpers to add a basic scrim and modal target frame. These should
 
 To show a modal you need to add content to the kpop turbo frame. You can do this in several ways:
 1. Use `content_for :kpop` in an HTML response to inject content into the kpop frame (see `yield :kpop` above)
-2. Use `layout "kpop"` in your controller to wrap your turbo response in a kpop frame
+2. Respond to a turbo frame request from the kpop frame component.
 
 You can generate a link that will cause a modal to show using the `kpop_link_to` helper.
 
 `kpop_link_to`'s are similar to a `link_to` in rails, but it will navigate to the given URL within the modal turbo
-frame. The targeted action will need to generate content in a `Kpop::FrameComponent`, e.g. using `layout "kpop"`.
+frame. The targeted action will need to generate content in a `Kpop::FrameComponent`, e.g. by responding to a turbo
+frame request with the ID `kpop`.
 
 ```html
 <!-- app/views/homepage/index.html.erb -->
@@ -73,6 +74,31 @@ frame. The targeted action will need to generate content in a `Kpop::FrameCompon
 <% end %>
 ```
 
+### Turbo Frame Layout
+
+Turbo Frame navigation responses use a layout to add a basic document structure. The `turbo-rails` gem provides the
+`turbo_rails/frame` layout, and kpop provides a similar `kpop/frame` layout. If a turbo frame response is requested with
+the `kpop` ID, the `kpop/frame` layout will be used automatically.  You can provide an alternative by setting `layout`
+in your controller, as usual.
+
+Turbo 8 assumes that the frame response will be a complete HTML document, including a `<head>` and `<body>`, and the
+Turbo Visits use the response head to deduce whether the navigation can be snap-shotted or not. This logic lives in
+`@hotwire/turbo` in the `Turbo.Visit` constructor:
+
+```javascript
+this.isSamePage = this.delegate.locationWithActionIsSamePage(this.location, this.action);
+```
+
+If the page is not the same, then the visit is not snap-shotted. Detection looks at turbo-tracked elements in the page
+head to make this decision, and history navigation with frames will not work unless the headers are compatible.
+
+As a consequence of this logic, it's really important that the layout used for kpop frame responses is compatible with
+the application layout. Kpop provides a "sensible default" that includes stylesheets and javascripts, but if your
+application doesn't use the same structure as the Rails default, you'll need to provide your own layout.
+
+If you're experiencing 'strange history behaviour', it's worth putting a breakpoint on the turbo `isSamePage`
+calculation to check that the headers are compatible.
+
 ## Development
 
 Releases need to be distributed to rubygems.org and npmjs.org. To do this, you need to have accounts with both providers
diff --git a/app/controllers/concerns/katalyst/kpop/frame_request.rb b/app/controllers/concerns/katalyst/kpop/frame_request.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+# Kpop Frame Requests use a different layout than Turbo Frame requests.
+#
+# The layout used is <tt>kpop/frame.html.erb</tt>. If there's a need to customize this layout, an application can
+# supply its own (such as <tt>app/views/layouts/kpop/frame.html.erb</tt>) which will be used instead.
+#
+# This module is automatically included in <tt>ActionController::Base</tt>.
+module Katalyst
+  module Kpop
+    module FrameRequest
+      extend ActiveSupport::Concern
+
+      class_methods do
+        # Example:
+        #  require_kpop only: %i[new edit] { url_for(resource) }
+        def require_kpop(**constraints, &fallback_location)
+          define_method(:kpop_fallback_location, fallback_location) if fallback_location
+
+          before_action :require_kpop, **constraints
+        end
+      end
+
+      included do
+        layout -> { turbo_frame_layout }
+      end
+
+      private
+
+      def kpop_frame_request?
+        turbo_frame_request_id == "kpop"
+      end
+
+      def require_kpop
+        redirect_back(fallback_location: kpop_fallback_location, status: :see_other) unless kpop_frame_request?
+      end
+
+      def turbo_frame_layout
+        if kpop_frame_request?
+          "kpop/frame"
+        elsif turbo_frame_request?
+          "turbo_rails/frame"
+        end
+      end
+    end
+  end
+end
diff --git a/app/javascript/kpop/controllers/frame_controller.js b/app/javascript/kpop/controllers/frame_controller.js
@@ -228,9 +228,9 @@ export default class Kpop__FrameController extends Controller {
 /**
  * Monkey patch for Turbo#FrameController.
  *
- * Intercept calls to navigateFrame(element, location) and ensures that src is
- * cleared if the frame is busy so that we don't restore an in-progress src on
- * restoration visits.
+ * Intercept calls to linkClickIntercepted(element, location) and ensures
+ * that src is cleared if the frame is busy so that we don't restore an
+ * in-progress src on restoration visits.
  *
  * See Turbo issue: https://github.com/hotwired/turbo/issues/1055
  *
@@ -240,18 +240,30 @@ function installNavigationInterception(controller) {
   const TurboFrameController =
     controller.element.delegate.constructor.prototype;
 
-  if (TurboFrameController._navigateFrame) return;
-
-  TurboFrameController._navigateFrame = TurboFrameController.navigateFrame;
-  TurboFrameController.navigateFrame = function (element, url, submitter) {
-    const frame = this.findFrameElement(element, submitter);
+  if (TurboFrameController._linkClickIntercepted) return;
+
+  TurboFrameController._linkClickIntercepted =
+    TurboFrameController.linkClickIntercepted;
+  TurboFrameController.linkClickIntercepted = function (element, location) {
+    // #findFrameElement
+    const id =
+      element?.getAttribute("data-turbo-frame") ||
+      this.element.getAttribute("target");
+    let frame = document.getElementById(id);
+    if (!(frame instanceof Turbo.FrameElement)) {
+      frame = this.element;
+    }
 
     if (frame.kpop) {
-      FrameModal.visit(url, frame.kpop, frame, () => {
-        TurboFrameController._navigateFrame.call(this, element, url, submitter);
+      FrameModal.visit(location, frame.kpop, frame, () => {
+        TurboFrameController._linkClickIntercepted.call(
+          this,
+          element,
+          location,
+        );
       });
     } else {
-      TurboFrameController._navigateFrame.call(this, element, url, submitter);
+      TurboFrameController._linkClickIntercepted.call(this, element, location);
     }
   };
 }
diff --git a/app/javascript/kpop/modals/frame_modal.js b/app/javascript/kpop/modals/frame_modal.js
@@ -30,7 +30,7 @@ export class FrameModal extends Modal {
 
   /**
    * When a user clicks a kpop link, turbo intercepts the click and calls
-   * navigateFrame on the turbo frame controller before setting the TurboFrame
+   * #navigateFrame on the turbo frame controller before setting the TurboFrame
    * element's src attribute. KPOP intercepts this call and calls this method
    * first so we cancel problematic navigations that might cache invalid states.
    *
diff --git a/app/javascript/kpop/turbo_actions.js b/app/javascript/kpop/turbo_actions.js
@@ -32,7 +32,10 @@ Turbo.StreamActions.kpop_redirect_to = function () {
       );
     const a = document.createElement("A");
     a.setAttribute("data-turbo-action", "replace");
-    this.targetElements[0].delegate.navigateFrame(a, this.getAttribute("href"));
+    this.targetElements[0].delegate.linkClickIntercepted(
+      a,
+      this.getAttribute("href"),
+    );
   } else {
     if (DEBUG)
       console.debug(`kpop: redirecting to ${this.getAttribute("href")}`);
diff --git a/app/views/layouts/kpop.html.erb b/app/views/layouts/kpop.html.erb
diff --git a/app/views/layouts/kpop/frame.html.erb b/app/views/layouts/kpop/frame.html.erb
@@ -0,0 +1,13 @@
+<html>
+<head>
+  <%# include all turbo-track elements so turbo knows it can cache frame visits %>
+  <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %>
+  <%= javascript_importmap_tags %>
+  <%= yield :head %>
+</head>
+<body>
+<%= render Kpop::FrameComponent.new do %>
+  <%= yield %>
+<% end %>
+</body>
+</html>
diff --git a/katalyst-kpop.gemspec b/katalyst-kpop.gemspec
@@ -2,7 +2,7 @@
 
 Gem::Specification.new do |spec|
   spec.name    = "katalyst-kpop"
-  spec.version = "3.0.2"
+  spec.version = "3.1.0"
   spec.authors = ["Katalyst Interactive"]
   spec.email   = ["developers@katalyst.com.au"]
 
@@ -16,6 +16,6 @@ Gem::Specification.new do |spec|
   spec.metadata["rubygems_mfa_required"] = "true"
 
   spec.add_dependency "katalyst-html-attributes"
-  spec.add_dependency "turbo-rails", "< 2"
+  spec.add_dependency "turbo-rails"
   spec.add_dependency "view_component"
 end
diff --git a/lib/katalyst/kpop/engine.rb b/lib/katalyst/kpop/engine.rb
@@ -6,7 +6,13 @@
 module Katalyst
   module Kpop
     class Engine < ::Rails::Engine
-      config.autoload_once_paths = %W(#{root}/app/helpers)
+      isolate_namespace Katalyst::Kpop
+      config.eager_load_namespaces << Katalyst::Kpop
+      config.autoload_once_paths = %W(
+        #{root}/app/helpers
+        #{root}/app/controllers
+        #{root}/app/controllers/concerns
+      )
 
       initializer "kpop.assets" do
         config.after_initialize do |app|
@@ -22,6 +28,7 @@ class Engine < ::Rails::Engine
         end
 
         ActiveSupport.on_load(:action_controller_base) do
+          include Katalyst::Kpop::FrameRequest
           helper Katalyst::Kpop::Engine.helpers
         end
       end
diff --git a/spec/dummy/app/controllers/children_controller.rb b/spec/dummy/app/controllers/children_controller.rb
@@ -2,8 +2,8 @@
 
 class ChildrenController < ParentsController
   def new
-    if turbo_frame_request?
-      render layout: "kpop", locals: { child: @parent.children.build }
+    if kpop_frame_request?
+      render locals: { child: @parent.children.build }
     else
       render :show, locals: { child: @parent.children.build }
     end
diff --git a/spec/dummy/app/controllers/modals_controller.rb b/spec/dummy/app/controllers/modals_controller.rb
@@ -4,17 +4,17 @@ class ModalsController < ApplicationController
   before_action :delay_request
 
   def show
-    if request.headers["Turbo-Frame"] == "kpop"
-      render "modals/frame", layout: "kpop"
+    if kpop_frame_request?
+      render "modals/frame"
     else
-      render "home/index", layout: "application", locals: { kpop: "modals/content" }
+      render "home/index", locals: { kpop: "modals/content" }
     end
   end
 
   def persistent
-    unless request.headers["Turbo-Frame"] == "kpop"
+    unless kpop_frame_request?
       @dismiss = root_path
-      render "home/index", layout: "application", locals: { kpop: "modals/persistent" }
+      render "home/index", locals: { kpop: "modals/persistent" }
     end
   end
 
diff --git a/spec/requests/modals_controller_spec.rb b/spec/requests/modals_controller_spec.rb
@@ -16,7 +16,7 @@
       let(:action) { get modal_path, headers: { "Turbo-Frame" => "kpop" } }
 
       it { is_expected.to be_successful }
-      it { is_expected.to have_rendered("layouts/kpop") }
+      it { is_expected.to have_rendered("layouts/kpop/frame") }
       it { is_expected.to have_rendered("modals/frame") }
     end
   end

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ export class FrameModal extends Modal {`
`30`	`30`
`31`	`31`	`/**`
`32`	`32`	`* When a user clicks a kpop link, turbo intercepts the click and calls`
`33`		`- * navigateFrame on the turbo frame controller before setting the TurboFrame`
	`33`	`+ * #navigateFrame on the turbo frame controller before setting the TurboFrame`
`34`	`34`	`* element's src attribute. KPOP intercepts this call and calls this method`
`35`	`35`	`* first so we cancel problematic navigations that might cache invalid states.`
`36`	`36`	`*`