Skip to content

Commit

Permalink
Don't morph frames flagged with "refresh=morph" as part of the full p…
Browse files Browse the repository at this point in the history
…age refresh

We will refresh those frames with morphing as part of the page refresh, so it does
not make sense to morph them also as part of the full page refresh. If we do, we'll
trigger a manual reload because the complete attribute will get removed. This aligns
the upstreamed version with the private gem we've been using internally.

This also fixes a couple of issues:

- We don't want to manually reload all the remote turbo-frames, only those that are
flagged with "refresh=morph". Regular remote frames will get reloaded automatically
when removing their complete attribute during regular page refreshes.

- Using idiomorph's "innerHTML" was resulting in a turbo-frame nested inside the
target turbo-frame. I think its semantics is not morphing inner contents from both
currentElement and newElement, but morphing newElement as the inner contents of currentElement.

See #1019 (comment)
  • Loading branch information
jorgemanrubia committed Oct 23, 2023
1 parent a653d12 commit 7ac1faf
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 3 deletions.
8 changes: 5 additions & 3 deletions src/core/drive/morph_renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ export class MorphRenderer extends Renderer {
}

#morphElements(currentElement, newElement, morphStyle = "outerHTML") {
this.isMorphingTurboFrame = this.#isFrameReloadedWithMorph(currentElement)

Idiomorph.morph(currentElement, newElement, {
morphStyle: morphStyle,
callbacks: {
Expand All @@ -40,8 +42,8 @@ export class MorphRenderer extends Renderer {
this.#remoteFrames().forEach((frame) => {
if (this.#isFrameReloadedWithMorph(frame)) {
this.#renderFrameWithMorph(frame)
frame.reload()
}
frame.reload()
})
}

Expand All @@ -56,7 +58,7 @@ export class MorphRenderer extends Renderer {
target: currentElement,
detail: { currentElement, newElement }
})
this.#morphElements(currentElement, newElement, "innerHTML")
this.#morphElements(currentElement, newElement)
}

#shouldRemoveElement = (node) => {
Expand All @@ -65,7 +67,7 @@ export class MorphRenderer extends Renderer {

#shouldMorphElement = (node) => {
if (node instanceof HTMLElement) {
return !node.hasAttribute("data-turbo-permanent")
return !node.hasAttribute("data-turbo-permanent") && (this.isMorphingTurboFrame || !this.#isFrameReloadedWithMorph(node))
} else {
return true
}
Expand Down
18 changes: 18 additions & 0 deletions src/tests/functional/page_refresh_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,24 @@ test("uses morphing to update remote frames marked with refresh='morph'", async
// Only the frame marked with refresh="morph" uses morphing
expect(await nextEventOnTarget(page, "refresh-morph", "turbo:before-frame-morph")).toBeTruthy()
expect(await noNextEventOnTarget(page, "refresh-reload", "turbo:before-frame-morph")).toBeTruthy()

await expect(page.locator("#refresh-morph")).toHaveText("Loaded morphed frame")

// Regular turbo-frames also gets reloaded since their complete attribute is removed
await expect(page.locator("#refresh-reload")).toHaveText("Loaded reloadable frame")
})

test("frames marked with refresh='morph' are excluded from full page morphing", async ({ page }) => {
await page.goto("/src/tests/fixtures/page_refresh.html")

await page.evaluate(() => document.getElementById("refresh-morph").setAttribute("data-modified", "true"))

await page.click("#form-submit")
await nextEventNamed(page, "turbo:render", { renderMethod: "morph" })
await nextBeat()

await expect(page.locator("#refresh-morph")).toHaveAttribute("data-modified", "true")
await expect(page.locator("#refresh-morph")).toHaveText("Loaded morphed frame")
})

test("it preserves the scroll position when the turbo-refresh-scroll meta tag is 'preserve'", async ({ page }) => {
Expand Down

0 comments on commit 7ac1faf

Please sign in to comment.