Skip to content

Commit

Permalink
Resolve Frame-to-Page Visit event ordering (#444)
Browse files Browse the repository at this point in the history 
* Resolve TypeScript build error

Merging [9c74f77][] introduced a TypeScript build error, which is
failing our CI build. This commit replaces the old `formSubmitted`
helper with the newer `formSubmitEnded`.

[9c74f77]: 9c74f77#diff-225e8f674fa1e4fd108c22a917fd5673e51909d4a5d8958e4874ae9e402dca2f

* Extract SnapshotSubstitution from FrameController

To refactor the `proposeVisitIfNavigatedWithAction()` implementation,
this commit extracts the `SnapshotSubstitution` to manage replacing
`Snapshot` references to `<turbo-frame>` elements whose visits are
promoted to page-wide state. The class partially implement
the `VisitDelegate` interface so that it can be passed directly as a
`delegate:` option to the `session.visit()` call.

* Resolve Frame-to-Page Visit event ordering

The problem
---

By attempting to avoid unnecessary renders and events by introducing the
`willRender:` Visit option, the initial `<turbo-frame
data-turbo-action="...">` implementation was skipping several crucial
lifecycle hooks. For example, the resulting Visit would fire a
`turbo:render`, but would not fire a `turbo:load`. This left the
`<html>` element in an inconsistent state without cleaning up any
`[data-turbo-preview]` or `[aria-busy]` attribute modifications.

The solution
---

Forego the `willRender:` option, and instead propose a visit with a
pre-populated `statusCode`, `redirected`, and `responseHTML` value so
that the `Session` (including all of its hooks) can handle transparently
the same as other `Visit` instances.

The result is much simpler than the original implementation: a promoted
Visit doesn't receive any specialized treatment, so it stands to
benefits from all the existing plumbing.
  • Loading branch information
@seanpdoyle
seanpdoyle authored Nov 13, 2021
1 parent 9e0a9b4 commit 6b6bdb2
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 28 deletions.
8 changes: 2 additions & 6 deletions src/core/drive/visit.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export type VisitOptions = {
action: Action,
delegate: Partial<VisitDelegate>
historyChanged: boolean,
willRender: boolean
referrer?: URL,
snapshotHTML?: string,
response?: VisitResponse
Expand All @@ -51,7 +50,6 @@ const defaultOptions: VisitOptions = {
action: "advance",
delegate: {},
historyChanged: false,
willRender: true
}

export type VisitResponse = {
Expand All @@ -75,7 +73,6 @@ export class Visit implements FetchRequestDelegate {
readonly timingMetrics: TimingMetrics = {}
readonly optionalDelegate: Partial<VisitDelegate>

willRender: boolean
followedRedirect = false
frame?: number
historyChanged = false
Expand All @@ -94,8 +91,7 @@ export class Visit implements FetchRequestDelegate {
this.location = location
this.restorationIdentifier = restorationIdentifier || uuid()

const { action, historyChanged, referrer, snapshotHTML, response, willRender, delegate: optionalDelegate } = { ...defaultOptions, ...options }
this.willRender = willRender
const { action, historyChanged, referrer, snapshotHTML, response, delegate: optionalDelegate } = { ...defaultOptions, ...options }
this.action = action
this.historyChanged = historyChanged
this.referrer = referrer
Expand Down Expand Up @@ -211,7 +207,7 @@ export class Visit implements FetchRequestDelegate {
}

loadResponse() {
if (this.response && this.willRender) {
if (this.response) {
const { statusCode, responseHTML } = this.response
this.render(async () => {
this.cacheSnapshot()
Expand Down
51 changes: 33 additions & 18 deletions src/core/frames/frame_controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { FetchResponse } from "../../http/fetch_response"
import { AppearanceObserver, AppearanceObserverDelegate } from "../../observers/appearance_observer"
import { clearBusyState, getAttribute, parseHTMLDocument, markAsBusy } from "../../util"
import { FormSubmission, FormSubmissionDelegate } from "../drive/form_submission"
import { Visit } from "../drive/visit"
import { Visit, VisitDelegate } from "../drive/visit"
import { Snapshot } from "../snapshot"
import { ViewDelegate } from "../view"
import { getAction, expandURL, urlsAreEqual, locationIsVisitable, Locatable } from "../url"
Expand Down Expand Up @@ -261,23 +261,15 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
const action = getAttribute("data-turbo-action", submitter, element, frame)

if (isAction(action)) {
const clone = frame.cloneNode(true)
const proposeVisit = () => {
const { ownerDocument, id, src } = frame
if (src) {
const snapshotHTML = ownerDocument.documentElement.outerHTML
let snapshot: Snapshot

const delegate = {
visitStarted(visit: Visit) {
snapshot = visit.view.snapshot
},
visitCachedSnapshot() {
snapshot.element.querySelector("#" + id)?.replaceWith(clone)
}
}

session.visit(src, { willRender: false, action, snapshotHTML, delegate })
const delegate = new SnapshotSubstitution(frame)
const proposeVisit = (event: Event) => {
const { target, detail: { fetchResponse } } = event as CustomEvent
if (target instanceof FrameElement && target.src) {
const { statusCode, redirected } = fetchResponse
const responseHTML = target.ownerDocument.documentElement.outerHTML
const response = { statusCode, redirected, responseHTML }

session.visit(target.src, { action, response, delegate })
}
}

Expand Down Expand Up @@ -403,6 +395,29 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
}
}

class SnapshotSubstitution implements Partial<VisitDelegate> {
private readonly clone: Node
private readonly id: string
private snapshot?: Snapshot

constructor(element: FrameElement) {
this.clone = element.cloneNode(true)
this.id = element.id
}

visitStarted(visit: Visit) {
this.snapshot = visit.view.snapshot
}

visitCachedSnapshot() {
const { snapshot, id, clone } = this

if (snapshot) {
snapshot.element.querySelector("#" + id)?.replaceWith(clone)
}
}
}

function getFrameElementById(id: string | null) {
if (id != null) {
const element = document.getElementById(id)
Expand Down
4 changes: 2 additions & 2 deletions src/tests/functional/form_submission_tests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -584,14 +584,14 @@ export class FormSubmissionTests extends TurboDriveTestCase {
await this.clickSelector('#dialog-formmethod-turbo-frame [formmethod="dialog"]')
await this.nextBeat

this.assert.notOk(await this.formSubmitted)
this.assert.notOk(await this.formSubmitEnded)
}

async "test form submission targetting frame skipped within method=dialog"() {
await this.clickSelector('#dialog-method-turbo-frame button')
await this.nextBeat

this.assert.notOk(await this.formSubmitted)
this.assert.notOk(await this.formSubmitEnded)
}

async "test form submission targetting frame skipped with submitter formmethod=dialog"() {
Expand Down
24 changes: 22 additions & 2 deletions src/tests/functional/frame_tests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,26 @@ export class FrameTests extends TurboDriveTestCase {
this.assert.equal(requestLogs.length, 0)
}

async "test navigating pushing URL state from a frame navigation fires events"() {
await this.clickSelector("#link-outside-frame-action-advance")

this.assert.equal(await this.nextAttributeMutationNamed("frame", "aria-busy"), "true", "sets aria-busy on the <turbo-frame>")
await this.nextEventOnTarget("frame", "turbo:before-fetch-request")
await this.nextEventOnTarget("frame", "turbo:before-fetch-response")
await this.nextEventOnTarget("html", "turbo:before-visit")
await this.nextEventOnTarget("html", "turbo:visit")
await this.nextEventOnTarget("frame", "turbo:frame-render")
await this.nextEventOnTarget("frame", "turbo:frame-load")
this.assert.notOk(await this.nextAttributeMutationNamed("frame", "aria-busy"), "removes aria-busy from the <turbo-frame>")

this.assert.equal(await this.nextAttributeMutationNamed("html", "aria-busy"), "true", "sets aria-busy on the <html>")
await this.nextEventOnTarget("html", "turbo:before-cache")
await this.nextEventOnTarget("html", "turbo:before-render")
await this.nextEventOnTarget("html", "turbo:render")
await this.nextEventOnTarget("html", "turbo:load")
this.assert.notOk(await this.nextAttributeMutationNamed("html", "aria-busy"), "removes aria-busy from the <html>")
}

async "test navigating turbo-frame[data-turbo-action=advance] from within pushes URL state"() {
await this.clickSelector("#add-turbo-action-to-frame")
await this.clickSelector("#link-frame")
Expand Down Expand Up @@ -358,7 +378,7 @@ export class FrameTests extends TurboDriveTestCase {
async "test navigating back after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames previous contents"() {
await this.clickSelector("#add-turbo-action-to-frame")
await this.clickSelector("#link-frame")
await this.nextBody
await this.nextEventNamed("turbo:load")
await this.goBack()
await this.nextBody

Expand All @@ -373,7 +393,7 @@ export class FrameTests extends TurboDriveTestCase {
async "test navigating back then forward after pushing URL state from a turbo-frame[data-turbo-action=advance] restores the frames next contents"() {
await this.clickSelector("#add-turbo-action-to-frame")
await this.clickSelector("#link-frame")
await this.nextBody
await this.nextEventNamed("turbo:load")
await this.goBack()
await this.nextBody
await this.goForward()
Expand Down

0 comments on commit 6b6bdb2

Please sign in to comment.