Integrate Frame-to-Page Visits with Snapshot Cache #441

seanpdoyle · 2021-11-12T04:11:33Z

The original implementation achieved the desired outcome: navigate the
page to reflect the URL of a <turbo-frame>.

Unfortunately, the session.visit() call happens late-enough that the
<turbo-frame> element's contents have already been updated. This means
that when navigating back or forward through the browser's History API,
the snapshots already reflect the "new" frame's HTML. This means that
navigating back won't change the page's HTML.

To resolve that issue, expands the VisitDelegate to include a caching
callback, then expands the VisitOptions type to include a
Partial<VisitDelegate>. Throughout the lifecycle, a Visit will
delegate to both its instance property and any present VisitOption
delegate hooks.

This commit aims to fix the broken behavior before the 7.1.0-rc
release, but if a concept like a FrameVisit introduced in #430
were to ship, it might be more straightforward to manage.

[Follow-up to hotwired#398][]. The original implementation achieved the desired outcome: navigate the page to reflect the URL of a `<turbo-frame>`. Unfortunately, the `session.visit()` call happens late-enough that the `<turbo-frame>` element's contents have already been updated. This means that when navigating back or forward through the browser's History API, the snapshots _already_ reflect the "new" frame's HTML. This means that navigating back _won't change the page's HTML_. To resolve that issue, expands the `VisitDelegate` to include a caching callback, then expands the `VisitOptions` type to include a `Partial<VisitDelegate>`. Throughout the lifecycle, a `Visit` will delegate to _both_ its instance property and any present `VisitOption` delegate hooks. This commit aims to fix the broken behavior before the `7.1.0-rc` release, but if a concept like a `FrameVisit` introduced in [hotwired#430][] were to ship, it might be more straightforward to manage. [Follow-up to hotwired#398]: hotwired#398 [hotwired#430]: hotwired#430

The problem --- The changes made in [444][] removed the `willRender:` Visit option in favor of allowing Frame-to-Visit navigations to participate in the entire Visit Rendering, Snapshot Caching, and History navigating pipeline. The way that the `willRender:` guard clause was removed caused new issues in how Frame-to-Visit navigations were treated. Removing the outer conditional without replacing it with matching checks elsewhere has caused Frame-to-Visit navigations to re-render the entire page, and losing the current contextual state like scroll, focus or anything else that exists outside the `<turbo-frame>` element. Similarly, the nature of the `FrameController.proposeVisitIfNavigatedWithAction()` helper resulted in an out-of-order dispatching of `turbo:` and `turbo:frame-` events, and resulted in `turbo:before-visit and `turbo:visit` events firing before `turbo:frame-render` and `turbo:frame-load` events. The solution --- To resolve the rendering issues, this commit re-introduces the `willRender:` option (originally introduced in [398][] and removed in [444][]). The option is captured in the `Visit` constructor and passed along the constructed `PageRenderer`. This commit adds the `willRender:` property to the `PageRenderer` class, which defaults to `true` unless specified as an argument. During `PageRenderer.render()` calls, the `replaceBody()` call is only made if `willRender == true`. To integrate with caching, this commit invokes the `VisitDelegate.visitCachedSnapshot()` callback with the `Snapshot` instance that is written to the `PageView.snapshotCache` so that the `FrameController` can manage the before- and after-navigation HTML to enable integration with navigating back and forward through the browser's history. To re-order the events, this commit replaces the `frame.addEventListener("turbo:frame-render")` attachment with a one-off `fetchResponseLoaded(FetchResponse)` callback that is assigned and reset during the frame navigation. When present, that callback is invoked _after_ the `turbo:load` event fires, which results in a much more expected event order: `turbo:before-fetch-request`, `turbo:before-fetch-response`, and `turbo:frame-` events fire first, then the rest of the Visit's events fire. To ensure this behavior, this commit adds several new types of tests, including coverage to make sure that the frame navigations can be transformed into page Visits without lasting consequences to the `<turbo-frame>` element. Similarly, another test ensures the preservation of scroll state and input text state after a Frame-to-Visit navigation. There is one quirk worth highlighting: the `FrameTests` seem incapable of using Selenium to serialize the `{ detail: { newBody: <body> } }` value out of the driven Browser's environment and into the Test harness environment. The event itself fires, but references a detached element or instance that results in a [Stale Element Reference][]. To work around that issue while delivering the bug fixes, this commit alters the `frame.html` page's `<html>` to opt-out of serializing those events' `event.detail` object (handled in [src/tests/fixtures/test.js](./src/tests/fixtures/test.js)). All other tests that assert about `turbo:` events (with `this.nextEventNamed` or `this.nextEventOnTarget`) will continue to behave as normal, the `FrameTests` is the sole exception. [398]: hotwired#398 [441]: hotwired#441 [444]: hotwired#444 [Stale Element Reference]: https://developer.mozilla.org/en-US/docs/Web/WebDriver/Errors/StaleElementReference

The problem --- The changes made in [444][] removed the `willRender:` Visit option in favor of allowing Frame-to-Visit navigations to participate in the entire Visit Rendering, Snapshot Caching, and History navigating pipeline. The way that the `willRender:` guard clause was removed caused new issues in how Frame-to-Visit navigations were treated. Removing the outer conditional without replacing it with matching checks elsewhere has caused Frame-to-Visit navigations to re-render the entire page, and losing the current contextual state like scroll, focus or anything else that exists outside the `<turbo-frame>` element. Similarly, the nature of the `FrameController.proposeVisitIfNavigatedWithAction()` helper resulted in an out-of-order dispatching of `turbo:` and `turbo:frame-` events, and resulted in `turbo:before-visit and `turbo:visit` events firing before `turbo:frame-render` and `turbo:frame-load` events. The solution --- To resolve the rendering issues, this commit re-introduces the `willRender:` option (originally introduced in [398][] and removed in [444][]). The option is captured in the `Visit` constructor and passed along the constructed `PageRenderer`. This commit adds the `willRender:` property to the `PageRenderer` class, which defaults to `true` unless specified as an argument. During `PageRenderer.render()` calls, the `replaceBody()` call is only made if `willRender == true`. To integrate with caching, this commit invokes the `VisitDelegate.visitCachedSnapshot()` callback with the `Snapshot` instance that is written to the `PageView.snapshotCache` so that the `FrameController` can manage the before- and after-navigation HTML to enable integration with navigating back and forward through the browser's history. To re-order the events, this commit replaces the `frame.addEventListener("turbo:frame-render")` attachment with a one-off `fetchResponseLoaded(FetchResponse)` callback that is assigned and reset during the frame navigation. When present, that callback is invoked _after_ the `turbo:load` event fires, which results in a much more expected event order: `turbo:before-fetch-request`, `turbo:before-fetch-response`, and `turbo:frame-` events fire first, then the rest of the Visit's events fire. The `fetchResponseLoaded(FetchResponse)` callback is an improvement, but is still an awkward way to coordinate between the `formSubmissionIntercepted()` and `linkClickIntercepted()` delegate methods, the `FrameController` instance, and the `Session` instance. It's functional for now, and we'll likely have a change to improve it with work like what's proposed in [430][] (which we can take on while developing `7.2.0`). To ensure this behavior, this commit adds several new types of tests, including coverage to make sure that the frame navigations can be transformed into page Visits without lasting consequences to the `<turbo-frame>` element. Similarly, another test ensures the preservation of scroll state and input text state after a Frame-to-Visit navigation. There is one quirk worth highlighting: the `FrameTests` seem incapable of using Selenium to serialize the `{ detail: { newBody: <body> } }` value out of the driven Browser's environment and into the Test harness environment. The event itself fires, but references a detached element or instance that results in a [Stale Element Reference][]. To work around that issue while delivering the bug fixes, this commit alters the `frame.html` page's `<html>` to opt-out of serializing those events' `event.detail` object (handled in [src/tests/fixtures/test.js](./src/tests/fixtures/test.js)). All other tests that assert about `turbo:` events (with `this.nextEventNamed` or `this.nextEventOnTarget`) will continue to behave as normal, the `FrameTests` is the sole exception. [398]: hotwired#398 [430]: hotwired#430 [441]: hotwired#441 [444]: hotwired#444 [Stale Element Reference]: https://developer.mozilla.org/en-US/docs/Web/WebDriver/Errors/StaleElementReference

The problem --- The changes made in [444][] removed the `willRender:` Visit option in favor of allowing Frame-to-Visit navigations to participate in the entire Visit Rendering, Snapshot Caching, and History navigating pipeline. The way that the `willRender:` guard clause was removed caused new issues in how Frame-to-Visit navigations were treated. Removing the outer conditional without replacing it with matching checks elsewhere has caused Frame-to-Visit navigations to re-render the entire page, and losing the current contextual state like scroll, focus or anything else that exists outside the `<turbo-frame>` element. Similarly, the nature of the `FrameController.proposeVisitIfNavigatedWithAction()` helper resulted in an out-of-order dispatching of `turbo:` and `turbo:frame-` events, and resulted in `turbo:before-visit and `turbo:visit` events firing before `turbo:frame-render` and `turbo:frame-load` events. The solution --- To resolve the rendering issues, this commit re-introduces the `willRender:` option (originally introduced in [398][] and removed in [444][]). The option is captured in the `Visit` constructor and passed along the constructed `PageRenderer`. This commit adds the `willRender:` property to the `PageRenderer` class, which defaults to `true` unless specified as an argument. During `PageRenderer.render()` calls, the `replaceBody()` call is only made if `willRender == true`. To integrate with caching, this commit invokes the `VisitDelegate.visitCachedSnapshot()` callback with the `Snapshot` instance that is written to the `PageView.snapshotCache` so that the `FrameController` can manage the before- and after-navigation HTML to enable integration with navigating back and forward through the browser's history. To re-order the events, this commit replaces the `frame.addEventListener("turbo:frame-render")` attachment with a one-off `fetchResponseLoaded(FetchResponse)` callback that is assigned and reset during the frame navigation. When present, that callback is invoked _after_ the `turbo:load` event fires, which results in a much more expected event order: `turbo:before-fetch-request`, `turbo:before-fetch-response`, and `turbo:frame-` events fire first, then the rest of the Visit's events fire. The `fetchResponseLoaded(FetchResponse)` callback is an improvement, but is still an awkward way to coordinate between the `formSubmissionIntercepted()` and `linkClickIntercepted()` delegate methods, the `FrameController` instance, and the `Session` instance. It's functional for now, and we'll likely have a change to improve it with work like what's proposed in [430][] (which we can take on while developing `7.2.0`). To ensure this behavior, this commit adds several new types of tests, including coverage to make sure that the frame navigations can be transformed into page Visits without lasting consequences to the `<turbo-frame>` element. Similarly, another test ensures the preservation of scroll state and input text state after a Frame-to-Visit navigation. There is one quirk worth highlighting: the `FrameTests` seem incapable of using Selenium to serialize the `{ detail: { newBody: <body> } }` value out of the driven Browser's environment and into the Test harness environment. The event itself fires, but references a detached element or instance that results in a [Stale Element Reference][]. To work around that issue while delivering the bug fixes, this commit alters the `frame.html` page's `<html>` to opt-out of serializing those events' `event.detail` object (handled in [src/tests/fixtures/test.js](./src/tests/fixtures/test.js)). All other tests that assert about `turbo:` events (with `this.nextEventNamed` or `this.nextEventOnTarget`) will continue to behave as normal, the `FrameTests` is the sole exception. [398]: #398 [430]: #430 [441]: #441 [444]: #444 [Stale Element Reference]: https://developer.mozilla.org/en-US/docs/Web/WebDriver/Errors/StaleElementReference

Follow-up to [hotwired#441][] Depends on [hotwired#487][] Closes hotwired#472 --- When caching Snapshots during a `Visit`, elements are not cached until after the `turbo:before-cache` event fires. This affords client applications with an opportunity to disconnect and deconstruct and JavaScript state, and provides an opportunity to encode that state into the HTML so that it can survive the caching process. The timing of the construction of the `SnapshotSubstitution` instance occurs too early in the frame rendering process: the `<turbo-frame>` descendants have not been disconnected. The handling of the `<turbo-frame>` caching is already an exception from the norm. Unfortunately, the current implementation is caching _too early_ in the process. If the Snapshot were cached too late in the process with the rest of the page (as described in [hotwired#441][]), the `[src]` attribute and descendant content would have already changed, so any previous state would be lost. This commit strikes a balance between the two extremes by introducing the `FrameRendererDelegate` interface and the `frameContentsExtracted()` hook. During `<turbo-frame>` rendering, the `FrameRenderer` instance selects a [Range][] of nodes and removes them by calling [Range.deleteContents][]. The `deleteContents()` method removes the Nodes and discards them. This commit replaces the `deleteContents()` call with one to [Range.extractContents][], so that the Nodes are retained as a [DocumentFragment][] instance. While handling the callback, the `FrameController` retains that instance by setting an internal `previousContents` property. Later on in the Frame rendering-to-Visit-promotion process, the `FrameController` implements the `visitCachedSnapshot()` hook to read from the `previousContents` property and substitute the frame's contents with the `previousContents`, replacing the need for the `SnapshotSubstitution` class. [hotwired#441]: hotwired#441 [hotwired#487]: hotwired#487 [Range]: https://developer.mozilla.org/en-US/docs/Web/API/Range [Range.deleteContents]: https://developer.mozilla.org/en-US/docs/Web/API/range/deleteContents [Range.extractContents]: https://developer.mozilla.org/en-US/docs/Web/API/Range/extractContents [DocumentFragment]: https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment

Follow-up to [#441][] Depends on [#487][] Closes #472 --- When caching Snapshots during a `Visit`, elements are not cached until after the `turbo:before-cache` event fires. This affords client applications with an opportunity to disconnect and deconstruct and JavaScript state, and provides an opportunity to encode that state into the HTML so that it can survive the caching process. The timing of the construction of the `SnapshotSubstitution` instance occurs too early in the frame rendering process: the `<turbo-frame>` descendants have not been disconnected. The handling of the `<turbo-frame>` caching is already an exception from the norm. Unfortunately, the current implementation is caching _too early_ in the process. If the Snapshot were cached too late in the process with the rest of the page (as described in [#441][]), the `[src]` attribute and descendant content would have already changed, so any previous state would be lost. This commit strikes a balance between the two extremes by introducing the `FrameRendererDelegate` interface and the `frameContentsExtracted()` hook. During `<turbo-frame>` rendering, the `FrameRenderer` instance selects a [Range][] of nodes and removes them by calling [Range.deleteContents][]. The `deleteContents()` method removes the Nodes and discards them. This commit replaces the `deleteContents()` call with one to [Range.extractContents][], so that the Nodes are retained as a [DocumentFragment][] instance. While handling the callback, the `FrameController` retains that instance by setting an internal `previousContents` property. Later on in the Frame rendering-to-Visit-promotion process, the `FrameController` implements the `visitCachedSnapshot()` hook to read from the `previousContents` property and substitute the frame's contents with the `previousContents`, replacing the need for the `SnapshotSubstitution` class. [#441]: #441 [#487]: #487 [Range]: https://developer.mozilla.org/en-US/docs/Web/API/Range [Range.deleteContents]: https://developer.mozilla.org/en-US/docs/Web/API/range/deleteContents [Range.extractContents]: https://developer.mozilla.org/en-US/docs/Web/API/Range/extractContents [DocumentFragment]: https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment

seanpdoyle force-pushed the frame-push-state-stack branch from e1811c3 to 357382a Compare November 12, 2021 15:19

seanpdoyle force-pushed the frame-push-state-stack branch from 357382a to 2ea70d5 Compare November 12, 2021 16:14

dhh merged commit 9e0a9b4 into hotwired:main Nov 12, 2021

seanpdoyle deleted the frame-push-state-stack branch November 12, 2021 17:39

seanpdoyle mentioned this pull request Dec 1, 2021

Frame Visits: Cache Snapshot later in process #488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Frame-to-Page Visits with Snapshot Cache #441

Integrate Frame-to-Page Visits with Snapshot Cache #441

seanpdoyle commented Nov 12, 2021

Integrate Frame-to-Page Visits with Snapshot Cache #441

Integrate Frame-to-Page Visits with Snapshot Cache #441

Conversation

seanpdoyle commented Nov 12, 2021