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.

Sign up for GitHub

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

Enable frame morphing for data-turbo-frame links and forms if the URL doesn't change #1316

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Enable frame morphing for data-turbo-frame links and forms if the URL doesn't change #1316

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19ee448
morph frame external navigation and form submission when url doesn't …
botandrose-machine Sep 22, 2024
966ddda
compare pathnames rather than full urls, to be consistent with how pa…
botandrose-machine Sep 26, 2024
064357c
extract duplication.
botandrose-machine Sep 26, 2024
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
11 changes: 8 additions & 3 deletions src/core/frames/frame_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,9 @@ export class FrameController {
}

sourceURLReloaded() {
const { refresh, src } = this.element

this.#shouldMorphFrame = src && refresh === "morph"
const { src } = this.element

this.#shouldMorphFrame = this.element.shouldReloadWithMorph
this.element.removeAttribute("complete")
this.element.src = null
this.element.src = src
Expand Down Expand Up @@ -234,6 +233,9 @@ export class FrameController {
const frame = this.#findFrameElement(formSubmission.formElement, formSubmission.submitter)

frame.delegate.proposeVisitIfNavigatedWithAction(frame, getVisitAction(formSubmission.submitter, formSubmission.formElement, frame))
if (frame.src === response.response.url && frame.shouldReloadWithMorph) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Determining whether a page-wide Visit is to be handled as a refresh involves more than an exact comparison of URL values.

At the Page level, it involves comparing the URL.pathname values (ignoring differences in query parameters) as well as checking for whether or not the Visit.action is "replace".

This change could make a similar comparison at the [src] and URL level, but driving Frame elements does not have a concept of a Visit, and [data-turbo-action="replace"] has Page-wide navigation implications.

Does it make sense to compare pathnames and ignore query parameters?

Copy link
Contributor

Choose a reason for hiding this comment

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

Disentangling [src] attribute changes from a "Frame Visit" initiated by a <form> element resulting in a redirect (instead of an <a> element click) isn't possible at the moment.

I had opened #430 to introduce the concept of a Frame Visit. It might be worth revisiting that initiative in hopes that the concept could make these sorts of implementation changes more straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanpdoyle Thanks for taking a look at this, and for pointing out how page refreshes already handle this situation regarding query params. I definitely want to try to keep things consistent between the frame and page systems! Not to mention that it will make this feature just that little bit more useful. I'll push a commit for this shortly.

Regarding #430, I love it. Initially wrapping my head around Turbo's internals has not been easy, and that additional abstraction would help to compartmentalize and clarify some of the complexity. I hope it finally gets merged soon.

frame.delegate.#shouldMorphFrame = true
}
frame.delegate.loadResponse(response)

if (!formSubmission.isSafe) {
Expand Down Expand Up @@ -343,6 +345,9 @@ export class FrameController {
frame.delegate.proposeVisitIfNavigatedWithAction(frame, getVisitAction(submitter, element, frame))

this.#withCurrentNavigationElement(element, () => {
if (frame.src === url && frame.shouldReloadWithMorph) {
frame.delegate.#shouldMorphFrame = true
}
frame.src = url
})
}
Expand Down
36 changes: 34 additions & 2 deletions src/tests/functional/frame_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ test("following a link to a page with a matching frame does not dispatch a turbo
)
})

test("following a link within a frame which has a target set navigates the target frame without morphing even when frame[refresh=morph]", async ({ page }) => {
test("following a link within a frame which has a target set navigates the target frame without morphing even when frame[refresh=morph] if the url changes", async ({ page }) => {
await page.click("#add-refresh-morph-to-frame")
await page.click("#hello a")
await nextBeat()
Expand All @@ -277,7 +277,7 @@ test("following a link within a frame which has a target set navigates the targe
await expect(page.locator("#frame h2")).toHaveText("Frame: Loaded")
})

test("navigating from within replaces the contents even with turbo-frame[refresh=morph]", async ({ page }) => {
test("navigating from within replaces the contents even with turbo-frame[refresh=morph] if the url changes", async ({ page }) => {
await page.click("#add-refresh-morph-to-frame")
await page.click("#link-frame")
await nextBeat()
Expand All @@ -287,6 +287,38 @@ test("navigating from within replaces the contents even with turbo-frame[refresh
await expect(page.locator("#frame h2")).toHaveText("Frame: Loaded")
})

test("following a link that targets a frame with refresh morph morphs the page if the url doesn't change", async ({ page }) => {
await page.click("#add-refresh-morph-to-frame")
await page.click("#outside-frame-form")
await nextBeat()

expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-render")).toBeTruthy()
expect(await noNextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy()

await page.click("#outside-frame-form")
await nextBeat()

expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy()
expect(await noNextEventOnTarget(page, "frame", "turbo:before-frame-render")).toBeTruthy()
})

test("submitting a form that targets a frame with refresh morph morphs the page if the url doesn't change", async ({ page }) => {
test.setTimeout(3000)
expect(await noNextEventOnTarget(page, "frame", "turbo:before-frame-render")).toBeTruthy()
await page.click("#add-refresh-morph-to-frame")
await page.click("#button-frame-action-advance")
await nextBeat()

expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-render")).toBeTruthy()
expect(await noNextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy()

await page.click("#button-frame-action-advance")
await nextBeat()

expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy()
expect(await noNextEventOnTarget(page, "frame", "turbo:before-frame-render")).toBeTruthy()
})

test("calling reload on a frame replaces the contents", async ({ page }) => {
await page.click("#add-src-to-frame")

Expand Down
Loading