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

Introduce turbo:frame-missing event #445

Merged
merged 5 commits into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
61 changes: 40 additions & 21 deletions src/core/frames/frame_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import { isAction, Action } from "../types"
import { VisitOptions } from "../drive/visit"
import { TurboBeforeFrameRenderEvent } from "../session"

export type TurboFrameMissingEvent = CustomEvent<{ fetchResponse: FetchResponse }>

export class FrameController
implements
AppearanceObserverDelegate,
Expand Down Expand Up @@ -145,23 +147,29 @@ export class FrameController
const html = await fetchResponse.responseHTML
if (html) {
const { body } = parseHTMLDocument(html)
const snapshot = new Snapshot(await this.extractForeignFrameElement(body))
const renderer = new FrameRenderer(
this,
this.view.snapshot,
snapshot,
FrameRenderer.renderElement,
false,
false
)
if (this.view.renderPromise) await this.view.renderPromise
this.changeHistory()

await this.view.render(renderer)
this.complete = true
session.frameRendered(fetchResponse, this.element)
session.frameLoaded(this.element)
this.fetchResponseLoaded(fetchResponse)
const newFrameElement = await this.extractForeignFrameElement(body)

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()

await this.view.render(renderer)
this.complete = true
session.frameRendered(fetchResponse, this.element)
session.frameLoaded(this.element)
this.fetchResponseLoaded(fetchResponse)
} else if (this.sessionWillHandleMissingFrame(fetchResponse)) {
await session.frameMissing(this.element, fetchResponse)
}
}
} catch (error) {
console.error(error)
Expand Down Expand Up @@ -378,12 +386,24 @@ export class FrameController
}
}

private sessionWillHandleMissingFrame(fetchResponse: FetchResponse) {
this.element.setAttribute("complete", "")

const event = dispatch<TurboFrameMissingEvent>("turbo:frame-missing", {
target: this.element,
detail: { fetchResponse },
cancelable: true,
})

return !event.defaultPrevented
}

private findFrameElement(element: Element, submitter?: HTMLElement) {
const id = getAttribute("data-turbo-frame", submitter, element) || this.element.getAttribute("target")
return getFrameElementById(id) ?? this.element
}

async extractForeignFrameElement(container: ParentNode): Promise<FrameElement> {
async extractForeignFrameElement(container: ParentNode): Promise<FrameElement | null> {
let element
const id = CSS.escape(this.id)

Expand All @@ -398,13 +418,12 @@ export class FrameController
await element.loaded
return await this.extractForeignFrameElement(element)
}

console.error(`Response has no matching <turbo-frame id="${id}"> element`)
} catch (error) {
console.error(error)
return new FrameElement()
}

return new FrameElement()
return null
}

private formActionIsVisitable(form: HTMLFormElement, submitter?: HTMLElement) {
Expand Down
1 change: 1 addition & 0 deletions src/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export {
} from "./session"

export { TurboSubmitStartEvent, TurboSubmitEndEvent } from "./drive/form_submission"
export { TurboFrameMissingEvent } from "./frames/frame_controller"
export { TurboBeforeFetchRequestEvent, TurboBeforeFetchResponseEvent } from "../http/fetch_request"
export { TurboBeforeStreamRenderEvent } from "../elements/stream_element"

Expand Down
7 changes: 7 additions & 0 deletions src/core/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,13 @@ export class Session
this.notifyApplicationAfterFrameRender(fetchResponse, frame)
}

async frameMissing(frame: FrameElement, fetchResponse: FetchResponse): Promise<void> {
const responseHTML = await fetchResponse.responseHTML
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since servers (like Rails) can respond to Frame requests with incomplete HTML documents, I'm unsure about how to best handle a response without a matching frame.

I'm not sure whether or not Drive should re-use the response HTML or issue a new request to fetch a full page of content. My gut tells me to always make a full request, but I'm curious if there's a quick win I'm not considering.

Copy link
Member

Choose a reason for hiding this comment

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

I think the current default is correct, because the majority case here will be to deal with pages like 500 errors or authentication expired errors, where full pages will be returned. But I think we can help developers here by logging a warning to point them in the direction of what's going on.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see you changed it to a full visit instead. I guess it doesn't matter that much. This behavior is for the exceptional case in any circumstance. Not for the normal, steady flow.

const { location, redirected, statusCode } = fetchResponse

return this.visit(location, { response: { redirected, statusCode, responseHTML } })
}

// Application events

applicationAllowsFollowingLinkToLocation(link: Element, location: URL, ev: MouseEvent) {
Expand Down
1 change: 1 addition & 0 deletions src/tests/fixtures/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,6 @@
"turbo:before-frame-render",
"turbo:frame-load",
"turbo:frame-render",
"turbo:frame-missing",
"turbo:reload"
])
55 changes: 52 additions & 3 deletions src/tests/functional/frame_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,59 @@ test("test following a link driving a frame toggles the [aria-busy=true] attribu
)
})

test("test following a link to a page without a matching frame results in an empty frame", async ({ page }) => {
test("test following a link to a page without a matching frame dispatches a turbo:frame-missing event", async ({
page,
}) => {
await page.click("#missing a")
await nextBeat()
assert.notOk(await innerHTMLForSelector(page, "#missing"))
await noNextEventOnTarget(page, "missing", "turbo:frame-render")
await noNextEventOnTarget(page, "missing", "turbo:frame-load")
const { fetchResponse } = await nextEventOnTarget(page, "missing", "turbo:frame-missing")
await nextEventNamed(page, "turbo:load")

assert.ok(fetchResponse, "dispatchs turbo:frame-missing with event.detail.fetchResponse")
assert.equal(pathname(page.url()), "/src/tests/fixtures/frames/frame.html", "navigates the page")

await page.goBack()
await nextEventNamed(page, "turbo:load")

assert.equal(pathname(page.url()), "/src/tests/fixtures/frames.html")
assert.ok(await innerHTMLForSelector(page, "#missing"))
})

test("test following a link to a page without a matching frame dispatches a turbo:frame-missing event that can be cancelled", async ({
page,
}) => {
await page.locator("#missing").evaluate((frame) => {
frame.addEventListener(
"turbo:frame-missing",
(event) => {
event.preventDefault()

if (event.target instanceof HTMLElement) {
event.target.textContent = "Overridden"
}
},
{ once: true }
)
})
await page.click("#missing a")
await nextEventOnTarget(page, "missing", "turbo:frame-missing")

assert.equal(await page.textContent("#missing"), "Overridden")
})

test("test following a link to a page with a matching frame does not dispatch a turbo:frame-missing event", async ({
page,
}) => {
await page.click("#link-frame")
await noNextEventNamed(page, "turbo:frame-missing")
await nextEventOnTarget(page, "frame", "turbo:frame-load")

const src = await attributeForSelector(page, "#frame", "src")
assert(
src?.includes("/src/tests/fixtures/frames/frame.html"),
"navigates frame without dispatching turbo:frame-missing"
)
})

test("test following a link within a frame with a target set navigates the target frame", async ({ page }) => {
Expand Down