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.

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

"Break out" of a frame from the server #367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ end

group :test do
gem 'capybara'
gem 'capybara_accessible_selectors', github: 'citizensadvice/capybara_accessible_selectors', branch: 'main'
gem 'rexml'
gem 'selenium-webdriver'
gem 'webdrivers'
Expand Down
25 changes: 25 additions & 0 deletions app/controllers/turbo/streams/redirect.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
module Turbo::Streams::Redirect
extend ActiveSupport::Concern

def redirect_to(options = {}, response_options = {})
turbo_frame = response_options.delete(:turbo_frame)
turbo_action = response_options.delete(:turbo_action)
location = url_for(options)

if request.format.turbo_stream? && turbo_frame.present?
alert, notice, flash_override = response_options.values_at(:alert, :notice, :flash)
flash.merge!(flash_override || {alert: alert, notice: notice})

Choose a reason for hiding this comment

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

A possible alternative to handling the flash options here could be to call super first, then set the response body with the Turbo Stream. This way, the existing redirect_to still handles the flash options, including any custom flash types added with add_flash_types.

Similar to the Turbo::Redirection module here, we could have something like:

def redirect_to(url = {}, options = {})
  if override_turbo_frame?(options)
    super.tap { visit_location_with_turbo_via_turbo_stream(location, options) }
  else
    super
  end
end

private

  def override_turbo_frame?(options)
    options[:turbo_frame].present? && previous_turbo_frame.present?
  end

  def previous_turbo_frame
    request.headers["Turbo-Frame"]
  end

  def visit_location_with_turbo_via_turbo_stream(location, options)
    turbo_frame = options.delete(:turbo_frame)
    turbo_action = options.delete(:turbo_action)

    self.status = 200
    self.response_body = render_to_string(
      "turbo/streams/redirect",
      locals: { location:, turbo_frame:, turbo_action:, previous_turbo_frame: },
      formats: [:turbo_stream]
    )
    response.content_type = "text/vnd.turbo-stream.html"
  end


case Rack::Utils.status_code(response_options.fetch(:status, :created))
when 300..399 then response_options[:status] = :created
end

render "turbo/streams/redirect", **response_options.with_defaults(
locals: {location: location, turbo_frame: turbo_frame, turbo_action: turbo_action},
location: location,
)
else
super
end
end
end
11 changes: 11 additions & 0 deletions app/views/turbo/streams/redirect.turbo_stream.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<%= turbo_stream.append_all "head" do %>
<%= javascript_tag nonce: true, data: {turbo_cache: false} do %>
window.Turbo.visit("<%= escape_javascript response.location %>", {
Copy link

@ollieh-m ollieh-m Mar 10, 2023

Choose a reason for hiding this comment

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

Having the flexibility to redirect the whole page from a request that came from a Turbo Frame would be really useful - thank you 🙇‍♂️.

I gave this change a spin and hit a bit of an issue...

  1. I have a data-turbo-stream link inside a Turbo Frame
  2. That Turbo Frame is inside a data-turbo-permanent element
  3. On clicking the link, I redirect in the controller action with redirect_to some_path, turbo_frame: "_top"
  4. The redirect.turbo_stream.erb is rendered and the correct Turbo.visit is executed
  5. The data-turbo-permanent element is copied to the new page
  6. The Turbo Frame within the data-turbo-permanent element still has src set (from when the link was clicked), but the Turbo Frame isn't complete
  7. When the Turbo Frame connects, the visit from the Turbo Frame is triggered again, because src is set and the Turbo Frame isn't complete

I removed the if request.format.turbo_stream? condition at one point, and ended up with an infinite loop, where the request from point 7 above kept triggering the redirect from point 3 😅.

A similar issue happens if the page is cached with the incomplete Turbo Frame that already has src set. Navigating back to the cached page triggers the request from the Turbo Frame. This then redirects the whole page, because of the server-side `redirect_to some_path, turbo_frame: "_top"``.

(More widely, this affects any Turbo Stream response triggered from a link inside a Turbo Frame. The Turbo Frame can end up in this incomplete state with src already set...)

I 'solved' the problem by forcing the Turbo Frame to be complete.

  1. I added this before line 3 here:
document.getElementById("<%= escape_javascript previous_turbo_frame %>").setAttribute("complete", "")
  1. I passed in previous_turbo_frame from the controller like this:
previous_turbo_frame: request.headers["Turbo-Frame"]

I'm guessing that workaround isn't the best approach - because complete is supposed to be read-only. Should src be unset on the Turbo Frame instead? Would that make sense as a general step within redirect.turbo_stream.erb?

Would really welcome your thoughts on the right approach to this.

frame: "<%= escape_javascript turbo_frame %>",
<% if turbo_action.present? %>
action: "<%= escape_javascript turbo_action %>",
<% end %>
})
document.currentScript.remove()
<% end %>
<% end %>
4 changes: 3 additions & 1 deletion lib/turbo/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ class Engine < Rails::Engine

initializer "turbo.helpers", before: :load_config_initializers do
ActiveSupport.on_load(:action_controller_base) do
include Turbo::Streams::TurboStreamsTagBuilder, Turbo::Frames::FrameRequest, Turbo::Native::Navigation
include Turbo::Streams::TurboStreamsTagBuilder, Turbo::Frames::FrameRequest, Turbo::Native::Navigation,
Turbo::Streams::Redirect

helper Turbo::Engine.helpers
end
end
Expand Down
18 changes: 15 additions & 3 deletions test/dummy/app/controllers/articles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,20 @@ def update
redirect_to articles_url
end

def create
def new
@article = Article.new
end

@article.update! article_params
def create
@article = Article.new article_params

redirect_to articles_url
if @article.save
flash.notice = "Created!"

redirect_to articles_url, redirect_params.to_h.to_options
else
render :new, status: :unprocessable_entity
end
end

def destroy
Expand All @@ -39,6 +47,10 @@ def article_params
params.require(:article).permit(:body)
end

def redirect_params
params.permit(:alert, :notice, :status, :turbo_action, :turbo_frame, flash: {}).with_defaults(status: :found)
end

def assert_param_method!
raise unless params[:_method].present?
end
Expand Down
17 changes: 17 additions & 0 deletions test/dummy/app/views/articles/new.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<details>
<summary>New Article</summary>

<%= turbo_frame_tag @article do %>
<%= form_with model: @article do |form| %>
<%= form.label :body %>
<%= form.text_area :body, aria: { describedby: (dom_id(@article, :errors) if @article.errors[:body].any?) } %>
<% if @article.errors[:body].any? %>
<p id="<%= dom_id(@article, :errors) %>">
<%= @article.errors[:body].to_sentence %>
</p>
<% end %>

<%= form.button name: :turbo_frame, value: "_top"%>
<% end %>
<% end %>
</details>
143 changes: 143 additions & 0 deletions test/streams/redirect_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
require "test_helper"

class Turbo::Streams::RedirectTest < ActionDispatch::IntegrationTest
test "html requests respond with a redirect HTTP status" do
post articles_path, params: {
turbo_frame: "_top", status: 303,
article: {body: "A valid value"}
}

assert_response :see_other
assert_redirected_to articles_url
assert_equal "Created!", flash[:notice]
end

test "html redirects write to the flash" do
post articles_path, params: {
turbo_frame: "_top", flash: {alert: "Wrote to alert:"},
article: {body: "A valid value"}
}

assert_equal "Wrote to alert:", flash[:alert]
end

test "html redirects write to alert" do
post articles_path, params: {
turbo_frame: "_top", alert: "Wrote to alert:",
article: {body: "A valid value"}
}

assert_equal "Wrote to alert:", flash[:alert]
end

test "html redirects write to notice" do
post articles_path, params: {
turbo_frame: "_top", notice: "Wrote to notice:",
article: {body: "A valid value"}
}

assert_equal "Wrote to notice:", flash[:notice]
end

test "turbo_stream requests with the turbo_frame: option responds with a redirect Turbo Stream" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top",
article: {body: "A valid value"}
}

assert_turbo_stream action: :append, targets: "head", status: :created do
assert_select "script[data-turbo-cache=false]", count: 1 do |script|
assert_includes script.text, %(frame: "_top",)
assert_not_includes script.text, %(action:)
end
end
assert_equal articles_url, response.location
end

test "turbo_stream requests with the turbo_frame: and turbo_action: options responds with a redirect Turbo Stream" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top",
turbo_action: "replace",
article: {body: "A valid value"}
}

assert_turbo_stream action: :append, targets: "head", status: :created do
assert_select "script[data-turbo-cache=false]", count: 1 do |script|
assert_includes script.text, %(frame: "_top",)
assert_includes script.text, %(action: "replace",)
end
end
assert_equal articles_url, response.location
end

test "turbo_stream requests with the turbo_frame: option preserves status: values in the 2xx range" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top", status: 200,
article: { body: "A valid value" }
}

assert_response 200
end

test "turbo_stream requests with the turbo_frame: option replaces status: values in the 3xx range with 201 Created" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top", status: 303,
article: { body: "A valid value" }
}

assert_response 201
end

test "turbo_stream requests with the turbo_frame: option preserves status: values in the 4xx range" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top", status: 403,
article: { body: "A valid value" }
}

assert_response 403
end

test "turbo_stream requests with the turbo_frame: option preserves status: values in the 5xx range" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top", status: 500,
article: { body: "A valid value" }
}

assert_response 500
end

test "turbo_stream requests without the turbo_frame: option respond with a redirect HTTP status" do
post articles_path, as: :turbo_stream, params: {
article: { body: "A valid value" }
}

assert_redirected_to articles_url
end

test "turbo_stream redirects write to the flash" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top", flash: {alert: "Wrote to alert:"},
article: {body: "A valid value"}
}

assert_equal "Wrote to alert:", flash[:alert]
end

test "turbo_stream redirects write to alert" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top", alert: "Wrote to alert:",
article: {body: "A valid value"}
}

assert_equal "Wrote to alert:", flash[:alert]
end

test "turbo_stream redirects write to notice" do
post articles_path, as: :turbo_stream, params: {
turbo_frame: "_top", notice: "Wrote to notice:",
article: {body: "A valid value"}
}

assert_equal "Wrote to notice:", flash[:notice]
end
end
26 changes: 26 additions & 0 deletions test/system/frames_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require "application_system_test_case"

class FramesTest < ApplicationSystemTestCase
test "can render an invalid submission within a frame" do
visit new_article_path
toggle_disclosure "New Article" do
click_on "Create Article"
end

within_disclosure "New Article" do
assert_field "Body", described_by: "can't be blank"
end
end

test "can redirect the entire page after a valid submission within a frame" do
visit new_article_path
toggle_disclosure "New Article" do
fill_in "Body", with: "An article's body"
click_on "Create Article"
end

assert_no_selector :disclosure, "New Article"
assert_no_field "Body"
assert_text "An article's body"
end
end