From a16f65063ebba26d40ff54d067d55cfa02870970 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Fri, 26 Feb 2021 00:26:11 -0500 Subject: [PATCH] Render 4xx responses within frame The use case --- Consider submitting a `
` from within a ``: ```html ``` In the case of success, the response will be a [303 redirecting to another URL][303]. In the case of a failed submission, the response will be a [422 with an HTML body][]: ```ruby class PostsController < ApplicationController def create @post = Post.create!(post_params) redirect_to post_url(@post) rescue ActiveRecord::RecordNotSaved render inline: <<~HTML, status: :unprocessable_entity
HTML end end ``` [303]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303 [422]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422 The current behavior --- Since the `
` 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 `` 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 `` 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 `` after a failure. Proposed change --- The current implementation treats submissions from within a `` different than it treats elements declared without a `[data-turbo-frame]` attribute or with `[data-turbo-frame]` referring to another that refers to another `` 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. --- src/core/frames/frame_controller.ts | 25 +++++++--- src/core/frames/frame_redirector.ts | 30 ++++++++---- src/tests/fixtures/form.html | 26 +++++++++++ src/tests/functional/form_submission_tests.ts | 46 +++++++++++++++++++ 4 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index e276a8fe2..04e658d3e 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -31,6 +31,7 @@ import { isAction, Action } from "../types" import { VisitOptions } from "../drive/visit" import { TurboBeforeFrameRenderEvent, TurboFetchRequestErrorEvent } from "../session" import { StreamMessage } from "../streams/stream_message" +import { navigator } from "../../core" export type TurboFrameMissingEvent = CustomEvent<{ fetchResponse: FetchResponse }> @@ -273,11 +274,17 @@ export class FrameController } formSubmissionSucceededWithResponse(formSubmission: FormSubmission, response: FetchResponse) { - const frame = this.findFrameElement(formSubmission.formElement, formSubmission.submitter) + const { formElement, submitter } = formSubmission + const frame = this.findFrameElement(formElement, submitter) + const target = getAttribute("data-turbo-frame", submitter, formElement) || frame.getAttribute("target") - this.proposeVisitIfNavigatedWithAction(frame, formSubmission.formElement, formSubmission.submitter) - - frame.delegate.loadResponse(response) + if (target == "_top") { + navigator.formSubmission = formSubmission + navigator.formSubmissionSucceededWithResponse(formSubmission, response) + } else { + this.proposeVisitIfNavigatedWithAction(frame, formSubmission.formElement, formSubmission.submitter) + frame.delegate.loadResponse(response) + } } formSubmissionFailedWithResponse(formSubmission: FormSubmission, fetchResponse: FetchResponse) { @@ -447,11 +454,15 @@ export class FrameController private shouldInterceptNavigation(element: Element, submitter?: HTMLElement) { const id = getAttribute("data-turbo-frame", submitter, element) || this.element.getAttribute("target") - if (element instanceof HTMLFormElement && !this.formActionIsVisitable(element, submitter)) { + if (!this.enabled) { return false } - if (!this.enabled || id == "_top") { + if (element instanceof HTMLFormElement) { + if (!this.formActionIsVisitable(element, submitter)) { + return false + } + } else if (id == "_top") { return false } @@ -470,7 +481,7 @@ export class FrameController return false } - return true + return this.element == element.closest("turbo-frame:not([disabled])") } // Computed properties diff --git a/src/core/frames/frame_redirector.ts b/src/core/frames/frame_redirector.ts index 8019f01ad..f0ec728d8 100644 --- a/src/core/frames/frame_redirector.ts +++ b/src/core/frames/frame_redirector.ts @@ -3,6 +3,7 @@ import { FrameElement } from "../../elements/frame_element" import { expandURL, getAction, locationIsVisitable } from "../url" import { LinkClickObserver, LinkClickObserverDelegate } from "../../observers/link_click_observer" import { Session } from "../session" +import { getAttribute } from "../../util" export class FrameRedirector implements LinkClickObserverDelegate, FormSubmitObserverDelegate { readonly session: Session @@ -69,19 +70,32 @@ export class FrameRedirector implements LinkClickObserverDelegate, FormSubmitObs if (isNavigatable) { const frame = this.findFrameElement(element, submitter) - return frame ? frame != element.closest("turbo-frame") : false - } else { - return false + + if (frame) { + const id = getAttribute("data-turbo-frame", submitter, element) || frame.getAttribute("target") + + if (frame == findClosestFrameElement(element)) { + return id == "_top" + } else { + return true + } + } else { + return false + } } } private findFrameElement(element: Element, submitter?: HTMLElement) { - const id = submitter?.getAttribute("data-turbo-frame") || element.getAttribute("data-turbo-frame") + const id = getAttribute("data-turbo-frame", submitter, element) + if (id && id != "_top") { - const frame = this.element.querySelector(`#${id}:not([disabled])`) - if (frame instanceof FrameElement) { - return frame - } + return this.element.querySelector(`turbo-frame#${id}:not([disabled])`) + } else { + return findClosestFrameElement(element) } } } + +function findClosestFrameElement(element: Element) { + return element.closest("turbo-frame:not([disabled])") +} diff --git a/src/tests/fixtures/form.html b/src/tests/fixtures/form.html index 9cf0cfeca..6225cbd61 100644 --- a/src/tests/fixtures/form.html +++ b/src/tests/fixtures/form.html @@ -11,6 +11,14 @@ position: static; } +

Form

@@ -293,6 +301,24 @@

Frame: Form

+ +
+ + +
+
+ + +
+
+ + +
+
+ + +
+ Method link outside frame
Stream link outside frame diff --git a/src/tests/functional/form_submission_tests.ts b/src/tests/functional/form_submission_tests.ts index 5bc9862ea..903633421 100644 --- a/src/tests/functional/form_submission_tests.ts +++ b/src/tests/functional/form_submission_tests.ts @@ -696,6 +696,52 @@ test("test invalid frame form submission with internal server errror status", as assert.equal(await page.textContent("#frame h2"), "Frame: Internal Server Error") }) +test("test frame form submission with form[data-turbo-frame=_top]", async ({ page }) => { + await page.click("#frame-form-targets-top-303") + await nextEventNamed(page, "turbo:load") + + assert.equal(pathname(page.url()), "/src/tests/fixtures/one.html") + assert.equal(await page.textContent("h1"), "One", "follows form redirect") + assert.notOk(await hasSelector(page, "#frame"), "redirects entire page") +}) + +test("test invalid frame form submission submitted by form[data-turbo-frame=_top]", async ({ page }) => { + await page.click("#frame-form-targets-top-422") + await nextBeat() + + assert.equal(await page.textContent("h1"), "Form", "does not replace entire page") + assert.equal( + await page.textContent("#frame h2"), + "Frame: Unprocessable Entity", + "renders the response HTML inside the frame" + ) +}) + +test("test invalid frame form submission submitted by button[data-turbo-frame=_top]", async ({ page }) => { + await page.click("#frame-button-targets-top-422") + await nextBeat() + + assert.equal(await page.textContent("h1"), "Form", "does not replace entire page") + assert.equal( + await page.textContent("#frame h2"), + "Frame: Unprocessable Entity", + "renders the response HTML inside the frame" + ) +}) + +test("test unprocessable entity frame form submission submitted in turbo-frame[target=_top]", async ({ page }) => { + await page.click("#set-frame-target-top") + await page.click("#frame-form-targets-top-422") + await nextBeat() + + assert.equal(await page.textContent("h1"), "Form", "does not replace entire page") + assert.equal( + await page.textContent("#frame h2"), + "Frame: Unprocessable Entity", + "renders the response HTML inside the frame" + ) +}) + test("test frame form submission with stream response", async ({ page }) => { const button = await page.locator("#frame form.stream[method=post] input[type=submit]") await button.click()