Extract `FrameVisit` to drive `FrameController` #430

seanpdoyle · 2021-10-27T22:14:46Z

Might close #793

The problem

Programmatically driving a <turbo-frame> element when its [src]
attribute changes is a suitable end-user experience in consumer
applications. It's a fitting black-box interface for the outside world:
change the value of the attribute and let Turbo handle the rest.

However, internally, it's a lossy abstraction.

For example, when the FrameRedirector class listens for page-wide
click and submit events, it determines if their targets are meant to
drive a <turbo-frame> element by:

finding an element that matches a clicked <a> element's [data-turbo-frame] attribute
finding an element that matches a submitted <form> element's [data-turbo-frame] attribute
finding an element that matches a submitted <form> element's
submitter's [data-turbo-frame] attribute
finding the closest <turbo-frame> ancestor to the <a> or <form>

Once it finds the matching frame element, it disposes of all that
additional context and navigates the <turbo-frame> by updating its
[src] attribute. This makes it impossible to control various aspects
of the frame navigation (like its "rendering" explored in
hotwired/turbo#146) outside of its destination URL.

Similarly, since a <form> and submitter pairing have an impact on
which <turbo-frame> is navigated, the FrameController implementation
passes around a HTMLFormElement and HTMLSubmitter? data clump and
constantly re-fetches a matching <turbo-frame> instance.

Outside of frames, page-wide navigation is driven by a Visit instance
that manages the HTTP life cycle and delegates along the way to a
Visit delegate. It also pairs calls to visit with an option object to
capture additional context.

The proposal

This commit introduces the FrameVisit class. It serves as an
encapsulation of the FetchRequest and FormSubmission lifecycle
events involved in navigating a frame.

It's implementation draws inspiration from the Visit class's delegate
and option structures. Since the FrameVisit knows how to unify
both FetchRequest and FormSubmission hooks, the resulting callbacks
fired from within the FrameController are flat and consistent.

Extra benefits

The biggest benefit is the introduction of a DRY abstraction to
manage the behind the scenes HTTP calls necessary to drive a
<turbo-frame>.

With the introduction of the FrameVisit concept, we can also declare a
visit() and submit() method for FrameElement delegate
implementations in the place of other implementation-specific methods
like loadResponse() and formSubmissionIntercepted().

In addition, these changes have the potential to close
hotwired/turbo#326, since we can consistently invoke
loadResponse() across <a>-click-initiated and
<form>-submission-initiated visits. To ensure that's the case, this
commit adds test coverage for navigating a <turbo-frame> by making a
GET request to an endpoint that responds with a 500 status.

[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, this commit integrates with the `turbo:visit` and `turbo:before-cache` events. Depending on **3** events is a touch awkward, but the event sequence occurs too "far" away from the `FrameController` instance for it to be able to integrate more tightly. 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

[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

[Follow-up to #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 [#430][] were to ship, it might be more straightforward to manage. [Follow-up to #398]: #398 [#430]: #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. 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

Introduce a beat-long delay to reduce test flakiness from a race condition between the initial `page.click` and the subsequent `page.evaluate` calls. For an example of the flakiness, this message was copied from a [CI failure on hotwired#430][]: ``` 1) [chrome] › rendering_tests.ts:68:1 › test reloads when tracked elements change due to failed form submission page.evaluate: Execution context was destroyed, most likely because of a navigation 88 | await page.click("#tracked-asset-change-form button") 89 | > 90 | const reason = await page.evaluate(() => localStorage.getItem("reason")) | ^ 91 | const unloaded = await page.evaluate(() => localStorage.getItem("unloaded")) 92 | 93 | assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html") ``` Another occurrence can be cited from a [CI failure on hotwired#800][]: ``` 1) [chrome] › rendering_tests.ts:77:1 › test reloads when tracked elements change due to failed form submission page.evaluate: Execution context was destroyed, most likely because of a navigation 97 | await page.click("#tracked-asset-change-form button") 98 | > 99 | const reason = await page.evaluate(() => localStorage.getItem("reason")) | ^ 100 | const unloaded = await page.evaluate(() => localStorage.getItem("unloaded")) 101 | 102 | assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html") ``` Troubleshooting this flakiness, uncovered a `console.error` message from the `rendering.html` test fixture: ``` rendering.html:1 An import map is added after module script load was triggered. ``` To resolve that issue, this commit also re-orders the `<head>` elements so that the `script[type="importmap"]` element is rendered before the `script[type="module"]`. [CI failure on hotwired#430]: https://github.com/hotwired/turbo/actions/runs/3797607273/jobs/6458693682#step:11:17 [CI failure on hotwired#800]: https://github.com/hotwired/turbo/actions/runs/3526641057/jobs/5914800548#step:11:16

Introduce a beat-long delay to reduce test flakiness from a race condition between the initial `page.click` and the subsequent `page.evaluate` calls. For an example of the flakiness, this message was copied from a [CI failure on #430][]: ``` 1) [chrome] › rendering_tests.ts:68:1 › test reloads when tracked elements change due to failed form submission page.evaluate: Execution context was destroyed, most likely because of a navigation 88 | await page.click("#tracked-asset-change-form button") 89 | > 90 | const reason = await page.evaluate(() => localStorage.getItem("reason")) | ^ 91 | const unloaded = await page.evaluate(() => localStorage.getItem("unloaded")) 92 | 93 | assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html") ``` Another occurrence can be cited from a [CI failure on #800][]: ``` 1) [chrome] › rendering_tests.ts:77:1 › test reloads when tracked elements change due to failed form submission page.evaluate: Execution context was destroyed, most likely because of a navigation 97 | await page.click("#tracked-asset-change-form button") 98 | > 99 | const reason = await page.evaluate(() => localStorage.getItem("reason")) | ^ 100 | const unloaded = await page.evaluate(() => localStorage.getItem("unloaded")) 101 | 102 | assert.equal(pathname(page.url()), "/src/tests/fixtures/rendering.html") ``` Troubleshooting this flakiness, uncovered a `console.error` message from the `rendering.html` test fixture: ``` rendering.html:1 An import map is added after module script load was triggered. ``` To resolve that issue, this commit also re-orders the `<head>` elements so that the `script[type="importmap"]` element is rendered before the `script[type="module"]`. [CI failure on #430]: https://github.com/hotwired/turbo/actions/runs/3797607273/jobs/6458693682#step:11:17 [CI failure on #800]: https://github.com/hotwired/turbo/actions/runs/3526641057/jobs/5914800548#step:11:16

seanpdoyle · 2023-01-31T20:46:56Z

@kevinmcconnell @afcapel with 7.2.5 released, could this changeset serve as a start toward 7.3.x development?

kevinmcconnell · 2023-02-03T09:18:40Z

@seanpdoyle we'll definitely take a look at this shortly! Sorry for the delay in getting to it so far, but I've finally carved out some time to catch up on the backlog so will be getting to it soon.

src/core/frames/frame_controller.ts

seanpdoyle · 2023-08-25T16:50:44Z

@kevinmcconnell @afcapel are either of you able to review these changes?

The problem --- Programmatically driving a `<turbo-frame>` element when its `[src]` attribute changes is a suitable end-user experience in consumer applications. It's a fitting black-box interface for the outside world: change the value of the attribute and let Turbo handle the rest. However, internally, it's a lossy abstraction. For example, when the `FrameRedirector` class listens for page-wide `click` and `submit` events, it determines if their targets are meant to drive a `<turbo-frame>` element by: 1. finding an element that matches a clicked `<a>` element's `[data-turbo-frame]` attribute 2. finding an element that matches a submitted `<form>` element's `[data-turbo-frame]` attribute 3. finding an element that matches a submitted `<form>` element's _submitter's_ `[data-turbo-frame]` attribute 4. finding the closest `<turbo-frame>` ancestor to the `<a>` or `<form>` Once it finds the matching frame element, it disposes of all that additional context and navigates the `<turbo-frame>` by updating its `[src]` attribute. This makes it impossible to control various aspects of the frame navigation (like its "rendering" explored in [hotwired#146][]) outside of its destination URL. Similarly, since a `<form>` and submitter pairing have an impact on which `<turbo-frame>` is navigated, the `FrameController` implementation passes around a `HTMLFormElement` and `HTMLSubmitter?` data clump and constantly re-fetches a matching `<turbo-frame>` instance. Outside of frames, page-wide navigation is driven by a `Visit` instance that manages the HTTP life cycle and delegates along the way to a `Visit` delegate. It also pairs calls to visit with an option object to capture additional context. The proposal --- This commit introduces the `FrameVisit` class. It serves as an encapsulation of the `FetchRequest` and `FormSubmission` lifecycle events involved in navigating a frame. It's implementation draws inspiration from the `Visit` class's delegate and option structures. Since the `FrameVisit` knows how to unify both `FetchRequest` and `FormSubmission` hooks, the resulting callbacks fired from within the `FrameController` are flat and consistent. Extra benefits --- The biggest benefit is the introduction of a DRY abstraction to manage the behind the scenes HTTP calls necessary to drive a `<turbo-frame>`. With the introduction of the `FrameVisit` concept, we can also declare a `visit()` and `submit()` method for `FrameElement` delegate implementations in the place of other implementation-specific methods like `loadResponse()` and `formSubmissionIntercepted()`. In addition, these changes have the potential to close [hotwired#326][], since we can consistently invoke `loadResponse()` across `<a>`-click-initiated and `<form>`-submission-initiated visits. To ensure that's the case, this commit adds test coverage for navigating a `<turbo-frame>` by making a `GET` request to an endpoint that responds with a `500` status. [hotwired#146]: hotwired#146 [hotwired#326]: hotwired#326

seanpdoyle mentioned this pull request Oct 30, 2021

Expand rendering capabilities of <turbo-frame> #146

Closed

seanpdoyle force-pushed the frame-visit-delegate branch from 4114bf6 to 73a3afd Compare November 9, 2021 14:26

seanpdoyle mentioned this pull request Nov 11, 2021

Expand the FrameElementDelegate interface #402

Closed

seanpdoyle force-pushed the frame-visit-delegate branch 3 times, most recently from f95cf1d to ca112d2 Compare November 11, 2021 18:12

seanpdoyle mentioned this pull request Nov 12, 2021

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

Merged

seanpdoyle mentioned this pull request Nov 16, 2021

Preserve page state while promoting Frame-to-Visit #448

Merged

seanpdoyle force-pushed the frame-visit-delegate branch 3 times, most recently from b1c9025 to 0e95df6 Compare November 25, 2021 02:50

seanpdoyle mentioned this pull request Jun 30, 2022

Update the history before rendering promoted frames #618

Merged

seanpdoyle force-pushed the frame-visit-delegate branch 5 times, most recently from cb7fcfd to b0d767e Compare July 29, 2022 21:43

seanpdoyle force-pushed the frame-visit-delegate branch 2 times, most recently from 0e2d283 to a2e2965 Compare December 29, 2022 01:19

seanpdoyle force-pushed the frame-visit-delegate branch from a2e2965 to b4d01ff Compare December 29, 2022 03:12

seanpdoyle force-pushed the frame-visit-delegate branch 2 times, most recently from c8dbbab to f8c260d Compare December 31, 2022 23:06

manuelpuyol approved these changes Jan 17, 2023

View reviewed changes

seanpdoyle force-pushed the frame-visit-delegate branch from f8c260d to 23a858c Compare January 22, 2023 01:14

seanpdoyle force-pushed the frame-visit-delegate branch from 23a858c to 44592f1 Compare February 4, 2023 17:54

seanpdoyle force-pushed the frame-visit-delegate branch from 44592f1 to 4a361a1 Compare February 13, 2023 16:03

seanpdoyle commented Feb 13, 2023

View reviewed changes

src/core/frames/frame_controller.ts Outdated Show resolved Hide resolved

seanpdoyle mentioned this pull request Feb 15, 2023

hotwire-example-pagination not works with 7.2.5 #873

Closed

seanpdoyle force-pushed the frame-visit-delegate branch from 4a361a1 to 3415576 Compare February 27, 2023 18:09

seanpdoyle commented Feb 27, 2023

View reviewed changes

src/core/frames/frame_controller.ts Outdated Show resolved Hide resolved

seanpdoyle force-pushed the frame-visit-delegate branch from 3415576 to e190d11 Compare February 28, 2023 16:07

seanpdoyle force-pushed the frame-visit-delegate branch from e190d11 to fa46afc Compare July 21, 2023 15:37

seanpdoyle force-pushed the frame-visit-delegate branch 2 times, most recently from 845459d to 5700105 Compare September 14, 2023 15:32

seanpdoyle mentioned this pull request Oct 30, 2023

Create Frame Snapshot from Fetch Response HTML #887

Merged

seanpdoyle mentioned this pull request Jan 30, 2024

Do not invalidate pages in visits that don't render #1144

Merged

seanpdoyle force-pushed the frame-visit-delegate branch from 5700105 to 4cdce4f Compare January 30, 2024 13:17

seanpdoyle mentioned this pull request Apr 15, 2024

Extract and re-use element morphing logic #1234

Merged

seanpdoyle force-pushed the frame-visit-delegate branch from 4cdce4f to 51c406d Compare April 15, 2024 17:31

seanpdoyle mentioned this pull request Sep 25, 2024

Enable frame morphing for data-turbo-frame links and forms if the URL doesn't change #1316

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract `FrameVisit` to drive `FrameController` #430

Extract `FrameVisit` to drive `FrameController` #430

seanpdoyle commented Oct 27, 2021 •

edited

Loading

seanpdoyle commented Jan 31, 2023

kevinmcconnell commented Feb 3, 2023

seanpdoyle commented Aug 25, 2023

Extract FrameVisit to drive FrameController #430

Are you sure you want to change the base?

Extract FrameVisit to drive FrameController #430

Conversation

seanpdoyle commented Oct 27, 2021 • edited Loading

The problem

The proposal

Extra benefits

seanpdoyle commented Jan 31, 2023

kevinmcconnell commented Feb 3, 2023

seanpdoyle commented Aug 25, 2023

Extract `FrameVisit` to drive `FrameController` #430

Extract `FrameVisit` to drive `FrameController` #430

seanpdoyle commented Oct 27, 2021 •

edited

Loading