Resolve Frame-to-Page Visit event ordering by seanpdoyle · Pull Request #444 · hotwired/turbo

seanpdoyle · 2021-11-13T05:52:08Z

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 transparently handle it
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.

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]: hotwired@9c74f77#diff-225e8f674fa1e4fd108c22a917fd5673e51909d4a5d8958e4874ae9e402dca2f

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.

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.

seanpdoyle · 2021-11-13T05:53:33Z

If this were merged, we could skip and close #443 since 23963f1 resolves the build error and c5479d4 fixes the flaky test more directly.

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

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/turbo#398 [430]: hotwired/turbo#430 [441]: hotwired/turbo#441 [444]: hotwired/turbo#444 [Stale Element Reference]: https://developer.mozilla.org/en-US/docs/Web/WebDriver/Errors/StaleElementReference

seanpdoyle added 3 commits November 13, 2021 00:47

dhh merged commit 6b6bdb2 into hotwired:main Nov 13, 2021

dhh mentioned this pull request Nov 13, 2021

Pass CI on main branch #443

Closed

seanpdoyle deleted the frame-advance-event-ordering branch November 13, 2021 12:04

seanpdoyle mentioned this pull request Nov 16, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve Frame-to-Page Visit event ordering#444

Resolve Frame-to-Page Visit event ordering#444
dhh merged 3 commits intohotwired:mainfrom
seanpdoyle:frame-advance-event-ordering

seanpdoyle commented Nov 13, 2021 •

edited

Loading

Uh oh!

seanpdoyle commented Nov 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

seanpdoyle commented Nov 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

The solution

Uh oh!

seanpdoyle commented Nov 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

seanpdoyle commented Nov 13, 2021 •

edited

Loading