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.

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

Lift the frame morphing logic up to FrameController.reload #1192

Merged
merged 25 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c03c893
Add new frame morphing refresh tests which fail
krschacht Feb 24, 2024
047a479
Get tests passing
krschacht Feb 25, 2024
e1fde68
Refactor code to remove duplication
krschacht Feb 25, 2024
5427b77
Remove comment
krschacht Feb 25, 2024
9fd1e9d
remove logging
krschacht Feb 25, 2024
a5a87e4
Remove testing server randNum hack
krschacht Feb 27, 2024
87c9553
Refactor to extract method & revert to class
krschacht Feb 29, 2024
07c3577
Refactor morphElements class
krschacht Mar 5, 2024
699f66f
Merge branch 'main' into frame-reload-uses-morphing
krschacht Aug 22, 2024
6abb120
wip
krschacht Aug 22, 2024
38e4d2c
Remove extraction added in another PR
krschacht Aug 22, 2024
8344e4d
Update reference to use new MorphingFrameRenderer
krschacht Aug 22, 2024
a342115
Reword test names
krschacht Aug 22, 2024
1ed1dc1
fixed typo
krschacht Aug 23, 2024
f738bdb
Only morph frame when using reload()
krschacht Aug 28, 2024
e956dd4
wip
krschacht Aug 28, 2024
fceb2cb
Fix a broken test
krschacht Aug 28, 2024
3af219d
Rewrite test fix to use playwright API
krschacht Aug 30, 2024
a0bf9a3
Fix unused reference
krschacht Aug 30, 2024
f45697c
Flip failing test to reflect product decision
krschacht Aug 31, 2024
55266c7
Merge branch 'main' into frame-reload-uses-morphing
krschacht Aug 31, 2024
8f0a949
Merge branch 'main' into frame-reload-uses-morphing
krschacht Aug 31, 2024
6801493
Revert "Flip failing test to reflect product decision"
krschacht Sep 3, 2024
795ab37
Correct logic for frame reloading
krschacht Sep 3, 2024
61bd384
revert unintended files
krschacht Sep 11, 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: 1 addition & 10 deletions src/core/drive/morphing_page_renderer.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { FrameElement } from "../../elements/frame_element"
import { MorphingFrameRenderer } from "../frames/morphing_frame_renderer"
import { PageRenderer } from "./page_renderer"
import { dispatch } from "../../util"
import { morphElements } from "../morphing"
Expand All @@ -13,7 +12,7 @@ export class MorphingPageRenderer extends PageRenderer {
})

for (const frame of currentElement.querySelectorAll("turbo-frame")) {
if (canRefreshFrame(frame)) refreshFrame(frame)
if (canRefreshFrame(frame)) frame.reload()
}

dispatch("turbo:morph", { detail: { currentElement, newElement } })
Expand All @@ -38,11 +37,3 @@ function canRefreshFrame(frame) {
frame.refresh === "morph" &&
!frame.closest("[data-turbo-permanent]")
}

function refreshFrame(frame) {
frame.addEventListener("turbo:before-frame-render", ({ detail }) => {
detail.render = MorphingFrameRenderer.renderElement
}, { once: true })

frame.reload()
}
9 changes: 9 additions & 0 deletions src/core/frames/frame_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { FrameView } from "./frame_view"
import { LinkInterceptor } from "./link_interceptor"
import { FormLinkClickObserver } from "../../observers/form_link_click_observer"
import { FrameRenderer } from "./frame_renderer"
import { MorphingFrameRenderer } from "./morphing_frame_renderer"
import { session } from "../index"
import { StreamMessage } from "../streams/stream_message"
import { PageSnapshot } from "../drive/page_snapshot"
Expand Down Expand Up @@ -89,6 +90,12 @@ export class FrameController {
}

sourceURLReloaded() {
if (this.element.shouldReloadWithMorph) {
this.element.addEventListener("turbo:before-frame-render", ({ detail }) => {
detail.render = MorphingFrameRenderer.renderElement
}, { once: true })
}

const { src } = this.element
this.element.removeAttribute("complete")
this.element.src = null
Expand Down Expand Up @@ -256,6 +263,7 @@ export class FrameController {
detail: { newFrame, ...options },
cancelable: true
})

const {
defaultPrevented,
detail: { render }
Expand Down Expand Up @@ -300,6 +308,7 @@ export class FrameController {
if (newFrameElement) {
const snapshot = new Snapshot(newFrameElement)
const renderer = new FrameRenderer(this, this.view.snapshot, snapshot, FrameRenderer.renderElement, false, false)

if (this.view.renderPromise) await this.view.renderPromise
this.changeHistory()

Expand Down
4 changes: 4 additions & 0 deletions src/elements/frame_element.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ export class FrameElement extends HTMLElement {
}
}

get shouldReloadWithMorph() {
return this.src && this.refresh === "morph"
}

/**
* Determines if the element is loading
*/
Expand Down
11 changes: 10 additions & 1 deletion src/tests/fixtures/frames.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
target.closest("turbo-frame")?.setAttribute("data-turbo-action", "advance")
} else if (target.id == "remove-target-from-hello") {
document.getElementById("hello").removeAttribute("target")
} else if (target.id == "add-refresh-reload-to-frame") {
target.closest("turbo-frame")?.setAttribute("refresh", "reload")
} else if (target.id == "add-refresh-morph-to-frame") {
target.closest("turbo-frame")?.setAttribute("refresh", "morph")
} else if (target.id == "add-src-to-frame") {
target.closest("turbo-frame")?.setAttribute("src", "/src/tests/fixtures/frames.html")
}
})
</script>
Expand All @@ -28,6 +34,9 @@ <h2>Frames: #frame</h2>
<button id="frame-form-get-no-redirect">Navigate #frame without a redirect</button>
</form>
<button id="add-turbo-action-to-frame" type="button">Add [data-turbo-action="advance"] to #frame</button>
<button id="add-refresh-reload-to-frame" type="button">Add [refresh="reload"] to #frame</button>
<button id="add-refresh-morph-to-frame" type="button">Add [refresh="morph"] to #frame</button>
<button id="add-src-to-frame" type="button">Add [src="/src/tests/fixtures/frames.html"] to #frame so it can be reloaded</button>
<a id="link-frame" href="/src/tests/fixtures/frames/frame.html">Navigate #frame from within</a>
<a id="link-frame-with-search-params" href="/src/tests/fixtures/frames/frame.html?key=value">Navigate #frame with ?key=value</a>
<a id="link-nested-frame-action-advance" href="/src/tests/fixtures/frames/frame.html" data-turbo-action="advance">Navigate #frame from within with a[data-turbo-action="advance"]</a>
Expand Down Expand Up @@ -60,7 +69,7 @@ <h2>Frames: #frame</h2>
<turbo-frame id="hello" target="frame">
<h2>Frames: #hello</h2>

<a id="hello-link-frame" href="/src/tests/fixtures/frames/frame.html">Load #frame</a>
<a href="/src/tests/fixtures/frames/frame.html">Load #frame</a>
<button type="button" id="remove-target-from-hello">Remove #hello[target]</button>

</turbo-frame>
Expand Down
39 changes: 35 additions & 4 deletions src/tests/functional/frame_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,12 +267,43 @@ test("following a link to a page with a matching frame does not dispatch a turbo
)
})

test("following a link within a frame with a target set navigates the target frame", 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]", async ({ page }) => {
await page.click("#add-refresh-morph-to-frame")
await page.click("#hello a")
await nextBeat()

const frameText = await page.textContent("#frame h2")
assert.equal(frameText, "Frame: Loaded")
expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-render")).toBeTruthy()
expect(await noNextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy()
await expect(page.locator("#frame h2")).toHaveText("Frame: Loaded")
})

test("navigating from within replaces the contents even with turbo-frame[refresh=morph]", async ({ page }) => {
await page.click("#add-refresh-morph-to-frame")
await page.click("#link-frame")
await nextBeat()

expect(await nextEventOnTarget(page, "frame", "turbo:before-frame-render")).toBeTruthy()
expect(await noNextEventOnTarget(page, "frame", "turbo:before-frame-morph")).toBeTruthy()
await expect(page.locator("#frame h2")).toHaveText("Frame: Loaded")
})

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

await page.evaluate(() => document.getElementById("frame").reload())

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

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

await page.evaluate(() => document.getElementById("frame").reload())

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

test("following a link in rapid succession cancels the previous request", async ({ page }) => {
Expand Down Expand Up @@ -667,7 +698,7 @@ test("navigating turbo-frame[data-turbo-action=advance] from within pushes URL s

test("navigating turbo-frame[data-turbo-action=advance] from outside with target pushes URL state", async ({ page }) => {
await page.click("#add-turbo-action-to-frame")
await page.click("#hello-link-frame")
await page.click("#hello a")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every other test was referencing click("#hello a") so I updated this one for consistency.

await nextEventNamed(page, "turbo:load")

await expect(page.locator("h1")).toHaveText("Frames")
Expand Down
9 changes: 6 additions & 3 deletions src/tests/functional/page_refresh_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ test("doesn't morph when the navigation doesn't go to the same URL", async ({ pa
expect(await noNextEventNamed(page, "turbo:render", { renderMethod: "morph" })).toBeTruthy()
})

test("uses morphing to update remote frames marked with refresh='morph'", async ({ page }) => {
test("uses morphing to only update remote frames marked with refresh='morph'", async ({ page }) => {
await page.goto("/src/tests/fixtures/page_refresh.html")

await page.click("#form-submit")
Expand All @@ -180,12 +180,15 @@ test("uses morphing to update remote frames marked with refresh='morph'", async
test("don't refresh frames contained in [data-turbo-permanent] elements", async ({ page }) => {
await page.goto("/src/tests/fixtures/page_refresh.html")

// Set the frame's text since the final assertion cannot be noNextEventOnTarget as that is passing even when the frame reloads.
const frame = page.locator("#remote-permanent-frame")
await frame.evaluate((frame) => frame.textContent = "Frame to be preserved")

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

// Only the frame marked with refresh="morph" uses morphing
expect(await noNextEventOnTarget(page, "refresh-reload", "turbo:before-frame-morph")).toBeTruthy()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assertion was not working. When I commented out code and convinced myself the permanent frame WAS reloading, this test would still pass. I think it's a race condition where the assertion breezes through too quickly.

I tried multiple different strategies to make this test durable and the solution you see here is the only one that worked.

await expect(page.locator("#remote-permanent-frame")).toHaveText("Frame to be preserved")
})

test("frames marked with refresh='morph' are excluded from full page morphing", async ({ page }) => {
Expand Down
6 changes: 6 additions & 0 deletions test-results/.last-run.json
krschacht marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"status": "failed",
"failedTests": [
"3ece60b52d6f72136f51-7cb2ec6b4d4eb9d4a91c"
]
}
Loading
Loading