From c31f4cd0ed451bb1b47b9a3156a2823e0a2d8482 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Mon, 15 Nov 2021 09:10:28 -0500 Subject: [PATCH] Frames: handle `GET` form submissions Closes [446][]. When handling a `
` submission targeting a `` element, responses that don't redirect do not change the element's `[src]` attribute. Following the changes made in [424][], `` submissions that result in `GET` requests are handled the same as other form submissions in that they fire `turbo:submit-start` and `turbo:submit-end` events. What [424][] did not account for is navigations initiated by `` submissions that _do not_ redirect. In response to those requests, this commit adds two additional criteria for updating the `` attribute: * the response's status code is [200 OK][], which is idempotent and does not indicate a server-side state change (for example, like a [201 Created][] with a `Location:` header might) * the response's `Content-Type:` is HTML (not JSON or even Turbo Stream) Under those circumstances, the frame's `[src]` is updated. This commit adds test coverage for this new behavior along with additional coverage to guard against regressions. [446]: https://github.com/hotwired/turbo/issues/446 [424]: https://github.com/hotwired/turbo/pull/424 [200 OK]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200 [201 Created]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201 --- src/core/frames/frame_controller.ts | 2 +- src/tests/fixtures/frames.html | 3 ++ src/tests/functional/form_submission_tests.ts | 7 +++++ src/tests/functional/frame_tests.ts | 30 +++++++++++++++++-- 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/core/frames/frame_controller.ts b/src/core/frames/frame_controller.ts index 53a585331..e4287c570 100644 --- a/src/core/frames/frame_controller.ts +++ b/src/core/frames/frame_controller.ts @@ -99,7 +99,7 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest } async loadResponse(fetchResponse: FetchResponse) { - if (fetchResponse.redirected) { + if (fetchResponse.redirected || (fetchResponse.succeeded && fetchResponse.isHTML)) { this.sourceURL = fetchResponse.response.url } diff --git a/src/tests/fixtures/frames.html b/src/tests/fixtures/frames.html index acfc17257..ed74efa3f 100644 --- a/src/tests/fixtures/frames.html +++ b/src/tests/fixtures/frames.html @@ -19,6 +19,9 @@

Frames

Frames: #frame

+ + + Navigate #frame from within Navigate #frame from within with a[data-turbo-action="advance"] diff --git a/src/tests/functional/form_submission_tests.ts b/src/tests/functional/form_submission_tests.ts index 519cd04f4..9df835722 100644 --- a/src/tests/functional/form_submission_tests.ts +++ b/src/tests/functional/form_submission_tests.ts @@ -359,6 +359,9 @@ export class FormSubmissionTests extends TurboDriveTestCase { const otherEvents = await this.eventLogChannel.read() this.assert.equal(otherEvents.length, 0, "no more events") + + const src = await (await this.querySelector("#frame")).getAttribute("src") || "" + this.assert.equal((new URL(src)).pathname, "/src/tests/fixtures/frames/frame.html") } async "test frame POST form targetting frame toggles submitter's [disabled] attribute"() { @@ -387,6 +390,9 @@ export class FormSubmissionTests extends TurboDriveTestCase { const otherEvents = await this.eventLogChannel.read() this.assert.equal(otherEvents.length, 0, "no more events") + + const src = await (await this.querySelector("#frame")).getAttribute("src") || "" + this.assert.equal((new URL(src)).pathname, "/src/tests/fixtures/frames/frame.html") } async "test frame GET form targetting frame toggles submitter's [disabled] attribute"() { @@ -529,6 +535,7 @@ export class FormSubmissionTests extends TurboDriveTestCase { this.assert.ok(await this.hasSelector("#frame form.redirect")) this.assert.equal(await message.getVisibleText(), "Hello!") this.assert.equal(await this.pathname, "/src/tests/fixtures/form.html") + this.assert.notOk(await this.propertyForSelector("#frame", "src"), "does not change frame's src") } async "test frame form submission with HTTP verb other than GET or POST"() { diff --git a/src/tests/functional/frame_tests.ts b/src/tests/functional/frame_tests.ts index d853021da..15f2810f7 100644 --- a/src/tests/functional/frame_tests.ts +++ b/src/tests/functional/frame_tests.ts @@ -302,6 +302,16 @@ export class FrameTests extends TurboDriveTestCase { this.assert.notOk(await this.nextAttributeMutationNamed("html", "aria-busy"), "removes aria-busy from the ") } + async "test navigating a frame with a form[method=get] that does not redirect still updates the [src]"() { + await this.clickSelector("#frame-form-get-no-redirect") + await this.nextBeat + + this.assert.equal(await (await this.querySelector("h1")).getVisibleText(), "Frames") + this.assert.equal(await (await this.querySelector("#frame h2")).getVisibleText(), "Frame: Loaded") + this.assert.equal(await this.pathname, "/src/tests/fixtures/frames.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") + } + async "test navigating turbo-frame[data-turbo-action=advance] from within pushes URL state"() { await this.clickSelector("#add-turbo-action-to-frame") await this.clickSelector("#link-frame") @@ -313,6 +323,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") } async "test navigating turbo-frame from within with a[data-turbo-action=advance] pushes URL state"() { @@ -325,6 +336,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") } async "test navigating frame with a[data-turbo-action=advance] pushes URL state"() { @@ -337,6 +349,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") } async "test navigating frame with form[method=get][data-turbo-action=advance] pushes URL state"() { @@ -349,6 +362,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") } async "test navigating frame with form[method=post][data-turbo-action=advance] pushes URL state"() { @@ -361,6 +375,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") } async "test navigating frame with button[data-turbo-action=advance] pushes URL state"() { @@ -373,6 +388,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") } async "test navigating back after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames previous contents"() { @@ -380,7 +396,7 @@ export class FrameTests extends TurboDriveTestCase { await this.clickSelector("#link-frame") await this.nextEventNamed("turbo:load") await this.goBack() - await this.nextBody + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -388,6 +404,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frames: #frame") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames.html") + this.assert.equal(await this.propertyForSelector("#frame", "src"), null) } async "test navigating back then forward after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames next contents"() { @@ -395,9 +412,9 @@ export class FrameTests extends TurboDriveTestCase { await this.clickSelector("#link-frame") await this.nextEventNamed("turbo:load") await this.goBack() - await this.nextBody + await this.nextEventNamed("turbo:load") await this.goForward() - await this.nextBody + await this.nextEventNamed("turbo:load") const title = await this.querySelector("h1") const frameTitle = await this.querySelector("#frame h2") @@ -405,6 +422,7 @@ export class FrameTests extends TurboDriveTestCase { this.assert.equal(await title.getVisibleText(), "Frames") this.assert.equal(await frameTitle.getVisibleText(), "Frame: Loaded") this.assert.equal(await this.pathname, "/src/tests/fixtures/frames/frame.html") + this.assert.equal(await this.getFrameSrc("frame"), "/src/tests/fixtures/frames/frame.html") } async "test turbo:before-fetch-request fires on the frame element"() { @@ -417,6 +435,12 @@ export class FrameTests extends TurboDriveTestCase { this.assert.ok(await this.nextEventOnTarget("frame", "turbo:before-fetch-response")) } + async getFrameSrc(id: string): Promise { + const src = await this.propertyForSelector("#" + id, "src") + + return new URL(src).pathname + } + get frameScriptEvaluationCount(): Promise { return this.evaluate(() => window.frameScriptEvaluationCount) }