Resolve Frame-to-Page Visit event ordering #444
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Closed
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 16, 2021
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
dhh
pushed a commit
that referenced
this pull request
Nov 19, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 cruciallifecycle hooks. For example, the resulting Visit would fire a
turbo:render, but would not fire aturbo: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 apre-populated
statusCode,redirected, andresponseHTMLvalue sothat the
Session(including all of its hooks) can transparently handle itthe same as other
Visitinstances.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.