Render 4xx responses within frame

[seanpdoyle]

The use case

Consider submitting a <form data-turbo-frame="_top"> from within a
<turbo-frame>:

<dialog open>
  <turbo-frame id="dialog">
    <form data-turbo-frame="_top" method="POST" action="/posts">
      <!-- ... -->
    </form>
  </turbo-frame>
</dialog>

In the case of success, the response will be a 303 redirecting to
another URL.

In the case of a failed submission, the response will be a 422 with an
HTML body:

class PostsController < ApplicationController
  def create
    @post = Post.create!(post_params)

    redirect_to post_url(@post)
  rescue ActiveRecord::RecordNotSaved
    render inline: <<~HTML, status: :unprocessable_entity
      <turbo-frame id="dialog">
        <form data-turbo-frame="_top" method="POST" action="/posts">
          <!-- ... -->
        </form>
      </turbo-frame>
    HTML
  end
end

The current behavior

Since the <form> element declares its target as _top, a successful
submission resulting in the 303 response, the entire page redirect.

Similarly, in the case of an invalid submission, the entire page's
HTML will be replaced by the response, in spite of the submission
occurring within a <turbo-frame> element.

The desired outcome

A valid submission behaves as expected and desired: redirect the _top
(entire page) to the URL.

In the case of a submission without Turbo, a failed submission would
result in a full re-render of the page, keeping the <dialog>
containing the form open. Any information or context in the "background"
would be re-rendered, but would exist.

With the introduction of Turbo Frames to the situation as a progressive
enhancement, a developer might expect the server's response to be
limited to only replacing the contents of the <turbo-frame> after a
failure.

Proposed change

The current implementation treats submissions from within a
<turbo-frame target="_top"> different than it treats elements declared
without a [data-turbo-frame] attribute or with [data-turbo-frame]
referring to another that refers to another <turbo-frame> element on
the page.

To account for the proposed change in behavior,
this commit changes the FrameController and FrameRedirector to
handle all frame submissions. If a Frame or form within a frame
targets _top, that redirect would only occur after a successful
redirect response.

[seanpdoyle]

@sstephenson this felt like the shortest distance to achieve the outcome. The awkwardness signals to me that there is a missing concept or delegate here.

A means of delegating or directly accessing the Navigator would also enable the transformation of a frame navigation to a fully formed Visit to drive the page.

[jon-sully]

Hey @seanpdoyle 👋🏻

Thanks for putting this together, first of all!

I'm running into some similar issues with Turbo Frames and responses/navigations so wanted to get your thoughts. The way I'm thinking about it, I'm seeing two main options:

1. The frame can target _top so that successful form submissions that respond with a 303 will redirect at the Drive level, and changes can be made such that 422/5xx responses (form validation failures) would be shoved back into the frame as to avoid a full-page/Drive navigation (this PR)

or

2. The frame can not target _top so that any 422/5xx responses (form validation failures) will automatically be 'inside the frame' then changes can be made to such that any 303 responses to the frame (e.g. a successful form submission) could just propagate up to Drive-level to result in a full-page navigation

Side-effects wise, the former means that any time the server responds with a 422/5xx to a request coming from within a frame that's targeting _top the server's error(s) / issue(s) may not be rendered at all (if they're outside of the frame in the markup) while the latter means that redirection-inside-a-frame is basically dead and having a multi-step in-frame experience would be tough 😕

I'm not sure I've got a strong opinion on either direction, but could you speak to the strategy behind choosing one or the other for the implementation of this feature?

I wonder if the real drawback here is the simple binary nature of _top as a target. Maybe we just need a second 'special' target like redirect_top that behaves normally for all responses but does propagate redirects to _top? Just spitballing 🙂

Tagging these because they feel relevant: #138, Hotwire Discourse #1562

[seanpdoyle]

@jon-sully my intuition pushes me toward the first option you laid out. It's also least disruptive in terms of existing Turbo conventions and patterns.

In my experiences, there certainly is a need for some version of the second option you mentioned, with regard to an ability to "break out" of a frame and combine the scoped HTML modifications and DOM state preservation of a Turbo Frame with the URL push state changes of a Turbo Visit.

We still haven't thread that needle yet, in terms of an attribute-based "interface", but there has been some exploration (#167).

[jon-sully]

Cool! Thanks for the thoughts. Like I said, I don't think I have a strong opinion here; either way accomplishes the goal of my forms working inside a frame when they fail and when they don't... and this PR does that from what I can tell! Hopefully this gets some traction in the coming weeks 💯

[tleish]

I'd like to be able to allow certain pages to respond with html that forces "top" in the response.

For example, if a users session times out and they click on a form (or link) within a frame which does not have target="_top", the response redirects the user to the login page, but to the user nothing happens on the page. If the response returned data which told Turbo to load at the top, this would be useful in this scenario.

[Growiel]

Where are we on this ? I had yet another case where this was an issue today. I work around it by using Turbo streams to update my entire page or use javascript to force a refresh, but both are, at best, temporary workarounds.

[ansonhoyt]

@Growiel probably not what you intended, but asking for status suggests this issue is being neglected and asks a maintainer to give it their attention. The project is in beta, so some issues like this are expected. If you look at PR and commit activity on https://github.com/hotwired projects, you'll see quite a bit of momentum from contributors, especially Basecamp and Thoughtbot folks. This issue doesn't need extra attention.

Like you, I share some concerns around this issue. I don't have bandwidth right now to either help solve it for everyone or to dig in to work around it for myself, so I've subscribed for GitHub updates and I choose not to deploy to this production yet.

This is open source. While Basecamp made Turbo happen (with the community and standing on Turbolinks shoulders), we all have some responsibility for helping it along. I've seen several small PRs merged in from folks new to Turbo, fixing some simple errors they ran into. ✌️ ❤️

[jon-sully]

Fantastic answer, @ansonhoyt 👏🏻

As with all things OSS, if the issue is pressing enough to an individual, we are all capable of forking the project, fixing it in our fork, and running that code wherever we need it to. Otherwise open-source is a contribution-based process we should all take part in and have good expectations around ☺️

And I'm also not assuming intent on any prior comment's tone, just speaking in a general sense!

[Growiel]

I get both of your points but I can't really contribute mote in this specific case: the PR is already there and working, the only thing I can do is make sure it isn't forgotten.

Not because the project is inactive, like @ansonhoyt seems to think I meant, but because there's so much going on, it's easy for one to slip through the cracks.

Before investing the time to do what @jon-sully suggested, it's better to know if the official fix will be released or not.

Why ? Because based on other PRs like this one: #56 sometimes while the use case is relevant (at least 3 tickets and 2 questions on the discussions site exist), the maintainers have decided that this specific case will not be supported.

So before I invest in a custom fork, I'd rather ask if I can hope to see official support one day.

Hope it makes sense.

[ansonhoyt]

@Growiel I'm just suggesting you look at the issue (in)activity in context of the project activity. I'd probably ask for status too if I see an RC release come out without this being addressed. That'd be a good signal this might be forgotten.

No worries though, I know it's easy to misread someone's comment. Sorry that I misread yours. I'm just another guy excited to get this working too. 😄

[jon-sully]

Hey @seanpdoyle what lead to this one getting closed? 👀

[mrhead]

@seanpdoyle What do you think about rendering 4xx responses within the frame, but replacing the whole page for 5xx responses?

5xx responses are usually designed for the whole page, and they won't fit small frames.

[seanpdoyle]

@mrhead I think that's a reasonable expectation. Since Frame requests with a 5xx status aren't handled at all, introducing that behavior feels like it could be better addressed in a separate pull request.

[tleish]

@seanpdoyle What do you think about rendering 4xx responses within the frame, but replacing the whole page for 5xx responses?

5xx responses are usually designed for the whole page, and they won't fit small frames.

I think the logic should be simpler. If the turbo-frame response does not include the expected turbo-frame id, replace the whole page and update the URL, regardless if the response is a 2xx, 3xx, 4xx or 5xx.

This solves multiple success or error cases when you want the response to turbo-frame to replace the whole page.

[seanpdoyle]

If the turbo-frame response does not include the expected turbo-frame id, replace the whole page and update the URL, regardless if the response is a 2xx, 3xx, 4xx or 5xx.

@tleish I think that's also a reasonable behavior to explore. If I'm understanding your proposal correctly, that feels outside the scope of this proposal.

This PR is scoped to handling 422 status codes served in response to a <turbo-frame> element that targets _top. In this case, there are matching <turbo-frame> elements in both the requesting document and the response body. Navigating the page when a matching <turbo-frame> is missing in the response might make this logic simpler, but that strikes me as an enhancement to make during a second pass, after this change is shipped. Does that sound reasonable?

[mollerhoj]

@seanpdoyle should this not also be the case for normal GET requests? If a turbo_frame has a src: attribute, and the response is a 422 or a 500, should that not also be rendered? Is far as I can tell, turbo has no way of dealing with this case?

[glaszig]

what’s the stopper here? this missing behavior is what keeps me from using turbo and others even (wrongfully) assume this behavior, see e.g. hotwired/turbo-rails#122.

[tonywok]

Appreciate all your work. Just wondering if this is still up for consideration or if perhaps a different approach is being considered. ❤️

[jon-sully]

I can't speak for @seanpdoyle but I believe this draft PR represents an idea that can accommodate several different functionalities, but there are a handful of other draft and/or open PRs aiming to accommodate other functionalities that may be the same. E.g. there are a lot of ideas floating around in the "make frames more flexible" space at the moment, and none has been settled on yet by the core team(s). Thus, this PR is still just an idea for consideration as it pertains to the overall Turbo API and expectations, less so a fully-featured implementation ready-for-merge.