Preserve page state while promoting Frame-to-Visit#448
Merged
dhh merged 1 commit intohotwired:mainfrom Nov 19, 2021
Merged
Conversation
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
f77dea7 to
f3345a9
Compare
Contributor
Author
|
@dhh if this is a good enough fix, I think that it'd be good to include in 7.1.0-rc2. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
The changes made in 444 removed the
willRender:Visit option infavor 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 newissues 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 inan out-of-order dispatching of
turbo:andturbo:frame-events, andresulted in
turbo:before-visitandturbo:visitevents firing beforeturbo:frame-renderandturbo:frame-loadevents.The solution
To resolve the rendering issues, this commit re-introduces the
willRender:option (originally introduced in 398 and removed in444). The option is captured in the
Visitconstructor and passedalong the constructed
PageRenderer. This commit adds thewillRender:property to the
PageRendererclass, which defaults totrueunlessspecified as an argument. During
PageRenderer.render()calls, thereplaceBody()call is only made ifwillRender == true.To integrate with caching, this commit invokes the
VisitDelegate.visitCachedSnapshot()callback with theSnapshotinstance that is written to the
PageView.snapshotCacheso that theFrameControllercan manage the before- and after-navigation HTML toenable 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-offfetchResponseLoaded(FetchResponse)callback that is assigned and resetduring the frame navigation. When present, that callback is invoked
after the
turbo:loadevent fires, which results in a much moreexpected event order:
turbo:before-fetch-request,turbo:before-fetch-response, andturbo:frame-events fire first,then the rest of the Visit's events fire.
The
fetchResponseLoaded(FetchResponse)callback is an improvement, butis still an awkward way to coordinate between the
formSubmissionIntercepted()andlinkClickIntercepted()delegatemethods, the
FrameControllerinstance, and theSessioninstance.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 thepreservation of scroll state and input text state after a Frame-to-Visit
navigation.
There is one quirk worth highlighting: the
FrameTestsseem incapableof 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.htmlpage's<html>to opt-out of serializing those events'event.detailobject (handled insrc/tests/fixtures/test.js). All other
tests that assert about
turbo:events (withthis.nextEventNamedorthis.nextEventOnTarget) will continue to behave as normal, theFrameTestsis the sole exception.