-
Notifications
You must be signed in to change notification settings - Fork 476
Use navigatable associated form unless submitter opts out (#1246) #1251
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
base: main
Are you sure you want to change the base?
Use navigatable associated form unless submitter opts out (#1246) #1251
Conversation
) When using a submitter with [form=id] attribute, it should respect the form's navigatable state unless the submitter explicitly opts out of turbo. Implements the expected behavior outlined in this comment: hotwired#1246 (comment)
9d9ac17 to
15db627
Compare
| } else if (this.submitterReferencesNavigatable(form, submitter)) { | ||
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this work as a recursive call to submissionIsNavigatable(submitter.form) (intentionally omitting the submitter argument) to re-use the existing code?
| } else if (this.submitterReferencesNavigatable(form, submitter)) { | |
| return true | |
| } else if (this.elementIsNavigatable(submitter) && submitter.form instanceof HTMLFormElement) { | |
| return this.submissionIsNavgatable(submitter.form) |
| import { test } from "@playwright/test" | ||
| import { assert } from "chai" | ||
| import { getFromLocalStorage, nextEventNamed, readEventLogs, setLocalStorageFromEvent } from "../helpers/page" | ||
|
|
||
| test.beforeEach(async ({ page }) => { | ||
| await page.goto("/src/tests/fixtures/form_turbo_optin_submitter.html") | ||
| await setLocalStorageFromEvent(page, "turbo:submit-start", "formSubmitStarted", "true") | ||
| await setLocalStorageFromEvent(page, "turbo:submit-end", "formSubmitEnded", "true") | ||
| await readEventLogs(page) | ||
| }) | ||
|
|
||
| test("allows submitting an associated data-turbo=true form without data-turbo=true on submitter", async ({ page }) => { | ||
| await page.click("#default-behavior-submit") | ||
|
|
||
| assert.ok(await formSubmitStarted(page), "fires turbo:submit-start") | ||
|
|
||
| const { fetchOptions } = await nextEventNamed(page, "turbo:before-fetch-request") | ||
|
|
||
| assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html")) | ||
|
|
||
| await nextEventNamed(page, "turbo:before-fetch-response") | ||
|
|
||
| assert.ok(await formSubmitEnded(page), "fires turbo:submit-end") | ||
| }) | ||
|
|
||
| test("prevents submitting an associated data-turbo=true form when explicitly opted out", async ({ page }) => { | ||
| await page.click("#opted-out-submit") | ||
|
|
||
| assert.notOk(await formSubmitStarted(page), "fires turbo:submit-start") | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these assert calls be re-written to use Playwright's expect instead?
| import { test } from "@playwright/test" | |
| import { assert } from "chai" | |
| import { getFromLocalStorage, nextEventNamed, readEventLogs, setLocalStorageFromEvent } from "../helpers/page" | |
| test.beforeEach(async ({ page }) => { | |
| await page.goto("/src/tests/fixtures/form_turbo_optin_submitter.html") | |
| await setLocalStorageFromEvent(page, "turbo:submit-start", "formSubmitStarted", "true") | |
| await setLocalStorageFromEvent(page, "turbo:submit-end", "formSubmitEnded", "true") | |
| await readEventLogs(page) | |
| }) | |
| test("allows submitting an associated data-turbo=true form without data-turbo=true on submitter", async ({ page }) => { | |
| await page.click("#default-behavior-submit") | |
| assert.ok(await formSubmitStarted(page), "fires turbo:submit-start") | |
| const { fetchOptions } = await nextEventNamed(page, "turbo:before-fetch-request") | |
| assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html")) | |
| await nextEventNamed(page, "turbo:before-fetch-response") | |
| assert.ok(await formSubmitEnded(page), "fires turbo:submit-end") | |
| }) | |
| test("prevents submitting an associated data-turbo=true form when explicitly opted out", async ({ page }) => { | |
| await page.click("#opted-out-submit") | |
| assert.notOk(await formSubmitStarted(page), "fires turbo:submit-start") | |
| }) | |
| import { expect, test } from "@playwright/test" | |
| import { getFromLocalStorage, nextEventNamed, readEventLogs, setLocalStorageFromEvent } from "../helpers/page" | |
| test.beforeEach(async ({ page }) => { | |
| await page.goto("/src/tests/fixtures/form_turbo_optin_submitter.html") | |
| await setLocalStorageFromEvent(page, "turbo:submit-start", "formSubmitStarted", "true") | |
| await setLocalStorageFromEvent(page, "turbo:submit-end", "formSubmitEnded", "true") | |
| await readEventLogs(page) | |
| }) | |
| test("allows submitting an associated data-turbo=true form without data-turbo=true on submitter", async ({ page }) => { | |
| await page.click("#default-behavior-submit") | |
| expect(await formSubmitStarted(page), "fires turbo:submit-start").toBeTruthy() | |
| const { fetchOptions } = await nextEventNamed(page, "turbo:before-fetch-request") | |
| expect(fetchOptions.headers["Accept"]).toContain("text/vnd.turbo-stream.html") | |
| await nextEventNamed(page, "turbo:before-fetch-response") | |
| expect(await formSubmitEnded(page), "fires turbo:submit-end").toBeTruthy() | |
| }) | |
| test("prevents submitting an associated data-turbo=true form when explicitly opted out", async ({ page }) => { | |
| await page.click("#opted-out-submit") | |
| expect(await formSubmitStarted(page), "fires turbo:submit-start").not.toBeTruthy() | |
| }) |
When using a submitter with [form=id] attribute, it should respect the form's navigatable state unless the submitter explicitly opts out of turbo.
Implements the expected behavior outlined in this comment:
#1246 (comment)
Related issue: #1246