Lift the frame morphing logic up to FrameController.reload

[krschacht]

Fixes Issue #1161

Background:

turbo-frame tags support an optional property of refresh="morph". It's used like this:

   <turbo-frame id="post" refresh="morph" src="/post/1">
      Loading... (preload contents)
   </turbo-frame>

When the full page morphs, this turbo-frame src is reloaded, and the presence of morph ensures the contents is morphed rather than replaced.

Problem:

However, if you trigger an update of this frame by another means such as document.getElementById('post').reload() then the contents of the frame is replaced without morphing. @jorgemanrubia acknowledged in this comment that it would be nice for this behavior to be consistent.

Solution within this PR:

This PR creates a new MorphFrameRenderer which inherits from FrameRenderer. (Note: MorphRenderer is renamed to MorphPageRenderer for clarity.) And just like PageView decides when to use MorphPageRenderer, now FrameController makes the appropriate decision of when to use MorphFrameRenderer.

Because MorphFrame* and MorphPage* share so much logic this has been extracted into a utility class core/morph_elements.js

Most of this was extracted into PR #1234. All that remains in this PR is (1) a new conditional to determine whether FrameRenderer or MorphingFrameRenderer should be called on this frame, and (2) good test coverage for these new cases.

I made a corresponding update to the turbo-frame docs here: hotwired/turbo-site#170

[krschacht]

There is no change to the logic of any of these methods. They're simply converted to static methods so that they can be used by FrameController.

[krschacht]

This class no longer needs to check if the frame has morph set or not. It can blindly trigger a reload() on each frame and trust that the frame will be smart — it knows if it needs to morph itself.

[krschacht]

Adding a piece of HTML which does not change made it easier for me to observe whether morphing is occurring or not within chrome inspector

[krschacht]

Every other test was referencing click("#hello a") so I updated this one for consistency.

[krschacht]

@seanpdoyle You reviewed my documentation PR yesterday, related to this change. I wanted to ping on this one in case you could also look at the bug fix.

[jorgemanrubia]

@krschacht thanks for working on this.

Instead of reusing MorphRenderer, which is a PageRenderer for doing this, I'd consider adding a specialized FrameRenderer that uses morphing. It could kick in when [refresh="morph"]. I think this would also remove some clunkiness in the current MorphRenderer, where it relies on an event to use morphing to refresh frames.

[krschacht]

@jorgemanrubia Sure thing! I'm still trying to fully understand the architecture, but I think I follow what you're saying. That event logic was clunky. I'll get this cleaned up so we can get it merged in.

Just to preview my plan in case you want to weigh in. I will plan to:

• Rename MorphRenderer to MorphPageRenderer
• Create a new MorphFrameRenderer which inherits from FrameRenderer
• Create a utility class which contains the core morphing logic so that these two Renderers don't duplicate so much code. Maybe src/core/morph_elements.js so that both Renders will be able to call MorphElements.morph(currentElement, newElement, morphStyle = "outerHTML")

[krschacht]

@jorgemanrubia This PR is ready to go. I confirmed that all tests are passing and added good test coverage for the new cases.

I also made a corresponding update to the turbo-frame docs here: hotwired/turbo-site#170

[seanpdoyle]

Since these are all static methods, is there any benefit to defining exporting a class? Would a collection of exported functions serve the same purpose?

[krschacht]

@seanpdoyle Unlike some of the other utility files, I did it as a class because they're tightly coupled methods so on any use you'd need to import all methods. None of them stand alone. In this way, it's similar to the Cache utility class. But there is no actual state that is being instantiated so I did them static methods.

But I'm happy to change them to independent functions which are each exported. Would you like me to make that change?

[seanpdoyle]

I did it as a class because they're tightly coupled methods so on any use you'd need to import all methods

Reading through the diff in its current state, the MorphElements class is only ever interacted with by importing the class and invoking MorphElements.morph directly:

import { MorphElements } from "../morph_elements"

// ...
MorphElements.morph(...)

Since its consistently invoked with a single method, the surface area of the module's interface could be reduced to only include morph:

import { morph } from "../morph_elements"

morph(...)

Then, the module could be simplified to be a collection of module-private functions supporting a single exported morph function.

[krschacht]

Oh, sorry, you're right!

Done

[krschacht]

@seanpdoyle As I was doing some additional cleanup, I discovered that this change broke a random test in the test suite. Specifically, moving from a class to a collection of methods.

I spent a little while trying to track down the source of the bug and the only way I could solve it was to bring the class back.

The issue has something to do with this context as shared between methods. The morph() method sets this.isMorphingTurboFrame which is later referenced by shouldMorphElement(), but this is intentionally an arrow function because it's provided as a callback to Idiomorph. As I was banging my head against the wall trying to figure out another way around this or threading the context through as an argument, it occurred to me: this is the problem that classes solve! :) I just mean: sharing context between discrete method calls.

For now I reverted back to the class with static methods. The full test suite passes again. If you really think it shouldn't be a class, the solution needs to involve eliminating this shared state and somehow threading it through, but I don't understand the innerworkings of Idiomorph to easily do that.

[seanpdoyle]

If the morphing is stateful (uses this), you're correct: separate functions won't be suitable.

The original feedback was around the over-use of the static keyword. A class that's composed of all static keywords is functionally equivalent to a group of functions. Any "state" preserved through assigning to a this will be global. That's equivalent to a module-local variable.

Sharing a model-local variable across all morphing will lead to sharing that state across potentially concurrent morphs. For example, if this.isMorphingTurboFrame is true for a frame but false for a page-wide morph, that contention could cause unexpected behavior.

As a balance, what do you think of this set of changes:

diff --git a/src/core/drive/morph_page_renderer.js b/src/core/drive/morph_page_renderer.js
index 3e2dbf8..9418f44 100644
--- a/src/core/drive/morph_page_renderer.js
+++ b/src/core/drive/morph_page_renderer.js
@@ -1,6 +1,6 @@
 import { dispatch } from "../../util"
 import { PageRenderer } from "./page_renderer"
-import { MorphElements } from "../morph_elements"
+import { morphElements } from "../morph_elements"
 
 export class MorphPageRenderer extends PageRenderer {
   async render() {
@@ -14,7 +14,7 @@ export class MorphPageRenderer extends PageRenderer {
   // Private
 
   async #morphBody() {
-    MorphElements.morph(this.currentElement, this.newElement)
+    morphElements(this.currentElement, this.newElement)
     this.#reloadRemoteFrames()
 
     dispatch("turbo:morph", {
diff --git a/src/core/frames/morph_frame_renderer.js b/src/core/frames/morph_frame_renderer.js
index 83d8bf1..b8404e2 100644
--- a/src/core/frames/morph_frame_renderer.js
+++ b/src/core/frames/morph_frame_renderer.js
@@ -1,5 +1,5 @@
 import { FrameRenderer } from "./frame_renderer"
-import { MorphElements } from "../morph_elements"
+import { morphElements } from "../morph_elements"
 import { dispatch } from "../../util"
 
 export class MorphFrameRenderer extends FrameRenderer {
@@ -11,6 +11,6 @@ export class MorphFrameRenderer extends FrameRenderer {
       detail: { currentElement, newElement }
     })
 
-    MorphElements.morph(currentElement, newElement, "innerHTML")
+    morphElements(currentElement, newElement, "innerHTML")
   }
 }
diff --git a/src/core/morph_elements.js b/src/core/morph_elements.js
index 0761486..7b74248 100644
--- a/src/core/morph_elements.js
+++ b/src/core/morph_elements.js
@@ -2,8 +2,14 @@ import { Idiomorph } from "idiomorph/dist/idiomorph.esm.js"
 import { dispatch } from "../util"
 import { FrameElement } from "../elements/frame_element"
 
-export class MorphElements {
-  static morph(currentElement, newElement, morphStyle = "outerHTML") {
+export function morphElements(currentElement, newElement, morphStyle = "outerHTML") {
+  const renderer = new Renderer()
+
+  renderer.morph(currentElement, newElement, morphStyle)
+}
+
+class Renderer {
+  morph(currentElement, newElement, morphStyle) {
     this.isMorphingTurboFrame = this.isFrameReloadedWithMorph(currentElement)
 
     Idiomorph.morph(currentElement, newElement, {
@@ -18,15 +24,15 @@ export class MorphElements {
     })
   }
 
-  static isFrameReloadedWithMorph(element) {
+  isFrameReloadedWithMorph(element) {
     return (element instanceof FrameElement) && element.shouldReloadWithMorph
   }
 
-  static shouldAddElement = (node) => {
+  shouldAddElement = (node) => {
     return !(node.id && node.hasAttribute("data-turbo-permanent") && document.getElementById(node.id))
   }
 
-  static shouldMorphElement = (oldNode, newNode) => {
+  shouldMorphElement = (oldNode, newNode) => {
     if (!(oldNode instanceof HTMLElement)) return
 
     if (oldNode.hasAttribute("data-turbo-permanent")) return false
@@ -44,17 +50,17 @@ export class MorphElements {
     return !event.defaultPrevented
   }
 
-  static shouldUpdateAttribute = (attributeName, target, mutationType) => {
+  shouldUpdateAttribute = (attributeName, target, mutationType) => {
     const event = dispatch("turbo:before-morph-attribute", { cancelable: true, target, detail: { attributeName, mutationType } })
 
     return !event.defaultPrevented
   }
 
-  static shouldRemoveElement = (node) => {
+  shouldRemoveElement = (node) => {
     return this.shouldMorphElement(node)
   }
 
-  static didMorphElement = (oldNode, newNode) => {
+  didMorphElement = (oldNode, newNode) => {
     if (newNode instanceof HTMLElement) {
       dispatch("turbo:morph-element", {
         target: oldNode,

That diff removes the static lines in favor of a bonafide Class with instances. It also exports a single morphElements function for callers to use without any knowledge that it's implemented with a class instance.

[krschacht]

@seanpdoyle Good call, I made this change. I confirmed that the tests all still pass.

My recommendation would be that we merge in this PR and circle back to the open "turbo:before-frame-morph" element later in a follow-up since this PR is not making any changes to events.

[brunoprietog]

This is great, it would help with quite a few things! I left minor comments.

[seanpdoyle]

Should this have a return statement?

[seanpdoyle]

Also, is it important to exclude initial <turbo-frame> elements that don't have a [src] attribute? Wouldn't morphing an initial HTTP request have a benefit?

[krschacht]

You're too fast for me! :) I actually mistakenly pushed before I was done. I'm doing a little more cleanup and making sure all tests pass before I went ahead and tagged anyone.

However, I don't understand your second comment. My mental model was that morphing for turbo frames only applied when there was a src. And I didn't introduce that extra condition, that's currently present within main.

When there is no src, then the full contents of the HTML must be supplied within the tags so that means it's part of the overall page. And so if the overall page morphs, then there is no special handling for turbo-frames — the contents within them just morphs along with all the rest of the HTML of that page. But I could be misunderstanding that.

That was my read of this original logic:
https://github.com/hotwired/turbo/blob/main/src/core/drive/morph_renderer.js#L95

The event binding hack was only being added when there was a src so I assume without that it's just regular HTML morphing of the page.

[krschacht]

If you're good with that, this PR should be ready. I just did the additional refactor Bruno suggested and I know you still want Jorge to weigh in on removing that event (or maybe you meant keep the event but just omit it from public documentation, I'm good either way).

[rbclark]

This is probably the Turbo 8 feature I'm looking forward to the most at the moment, thank you @krschacht. I just wanted to say I've been testing this locally for the past week or so and it works very well, I haven't run into any gotchas or bugs.

[brunoprietog]

The lint failure is legit, the file is only 40 lines long.

[krschacht]

@brunoprietog Ah yes, you're right. I've fixed. But let me finish investigating this other test failure that I discovered around navigated frame refresh. I didn't touch the test and didn't expect it to break this behavior, but I'm still wrapping my mind around it to debug. Tbd

[krschacht]

@brunoprietog I could use your help on this test failure. I fully understand what's going on, and it's actually touching on a really interesting edge case. I think I need ya'll to make a clear product decision and then I can implement appropriately. Let me explain:

I believe the current intended logic for turbo-frames is as follows:

When a page morph is triggered for a page because the rules are met (refresh=morph & redirect back to the same URL), as we are potentially morphing all page elements we make special decisions about how to handle the turbo-frames. If a turbo-frame has a src and it has a refresh="..." specified then we follow this directive and either reload or morph. So the overall page may be morphing, but refresh="reload" might be on a turbo-frame and we'd reload that. When the turbo-frame does not have a src, we don't do any special treatment of this frame element so as the overall parent page is undergoing a morph, if this page includes html within the turbo-frame tags then it'll be morphed along with the rest of the page, if needed.

Is this understanding all accurate? I'm not attempting to propose any changes in this paragraph, I'm just trying to articulate how I think it's supposed to work. Assuming that is correct, here is the edge case:

What happens if a turbo-frame does not have a src at the time a parent page is loaded, but something dynamically updates the turbo-frame to have a src? In fact, something could dynamically update the src & change refresh="morph". Actually, something could even dynamically insert a turbo-frame where one did not exist on page load. So, at the time the parent page is refreshing with a morph, do we respect the current src & refresh attributes of the turbo-frames on the page and handle them as indicated by the logic I summarized above. Or do we ignore the current src & refresh attributes and instead look at the src, refresh, and turbo-frame contents that was just returned to us by the parent URL and update the page state so that it respects those? This could even include removing a turbo-frame from the page if the parent URL provided us new HTML which did not contain that turbo-frame.

[brunoprietog]

That's a good question. The morphing process has two stages. In the first, you go through all the elements. If we find a Turbo frame susceptible to be reloaded with morphing, we ignore it completely, i.e., it's not touched, no morphing is applied on it (yet). This manifests itself here:

https://github.com/hotwired/turbo/blob/main/src/core/drive/morphing_page_renderer.js#L11

The other case where morphing is not done is if the element has the data-turbo-permanent attribute.

Once that process is complete, what we have are all new elements returned by the server, except for Turbo frames that are susceptible to morphing reloads and permanent elements, which remain in the state prior to the server's response. This is because we are interested in preserving the URL where the Turbo frame is located if the user has navigated through it. Otherwise, each page refresh would reset the Turbo frame to the initial state of the page, and that isn't what we want.

Then comes the second stage. We go through all the Turbo frames, and if there's one that can be reloaded with morphing, we refresh and replace the current content with the new content of the Turbo frame returned by the server using morphing. This is the process we are refactoring here.

So, to answer your question, yes, you can add Turbo frames dynamically and change their src. These won't be touched if they have the conditions for reloading using morphing. Only the reload method will be called on them to refresh them, based on the current URL they are at. In fact, you may have Turbo frames that initially have no src attribute, but when navigating through them have it due to having performed a navigation action on them. We are interested in preserving that state.

Even on HEY Calendar we do something like this for pagination. We dynamically add Turbo frames for each new page. On each page refresh, the server doesn't actually return these Turbo frames, but since they already exist on the client, they remain there, and are reloaded with morphing.

[brunoprietog]

I'm sorry if I explained wrong. Turbo frames that aren't reloaded by morphing have to be reset correctly. This test should be fine.

Turbo frames that don't have the conditions to be reloaded with morphing are affected in the first round of morphing, therefore, they are morphed according to the changes coming from the server. Turbo frames that can be reloaded with morphing due to the presence of the src attribute and refresh="morph" are ignored in the first round, and then refreshed using morphing.

[krschacht]

I was just about to send a long final comment saying I finished. :) Happy to do this either way. This test is a bit confusing so it took me a bit to fully make sense of. In your longer explanation, you said what my expectation would be. Specifically:

This is because we are interested in preserving the URL where the Turbo frame is located if the user has navigated through it. Otherwise, each page refresh would reset the Turbo frame to the initial state of the page, and that isn't what we want.

This test was enforcing the opposite, so I flipped it to reflect what you said here. ^ And it fits my intuition that if a user navigations a turbo frame, and it's implicitly reload="refresh" (not morph) then I would expect that navigation to stay. But you think we should revert it back since it started with no src when the page loaded?

[brunoprietog]

Yes, that's correct, but only for Turbo frames marked with refresh="morph" attribute. Sorry if I wasn't clear enough.

[brunoprietog]

What I mean is that the goal of the test as it was before was correct. Turbo frames that don't contain the refresh="morph" attribute are reset. We only retain the state of Turbo frames that are reloaded with morphing. The rest are morphed in the first round according to what the server returns in the request made at the time of the page refresh.

[krschacht]

The test has been reverted (so that it fails again) and the logic has been corrected (so now the test passes).

I think that's it and things are now ready. 🤞

[krschacht]

This PR should be ready to merge in

[jorgemanrubia]

Great work here @krschacht 👏, I'll merge shortly.

[krschacht]

@brunoprietog I reverted the last-run file and also some stray changes to yarn.lock. Be sure to merge the change to the docs as well over here: hotwired/turbo-site#170 . Then you can close out the Issue which started all this: #1161

Guys — While I have your attention, I am using turbo-frames extensively in my app. I have discovered a bug with history and turbo frames. I have opened a new issue regarding this, others have confirmed the bug and there is a reliable repro and fix. I can't tell if the core team has had eyes on this yet so it's worth a quick look as people are sharing monkey patch hacks and teeing up different PRs attempting to fix it: #1300

[jorgemanrubia]

Thanks @krschacht. We'll have a look at that one.

[seanpdoyle]

Thank you for pushing this through @krschacht!

I've modified #1028 to incorporate the changes made here. It also addresses the underlying issues highlighted by #1303.

[jordancoil]

When I saw this PR get merged I was excited, but then I realized the original idea to expose the MorphingPageRenderer and MorphingFrameRenderer got scrapped 😢 .

#1234 pointed out that by exposing things like MorphingPageRenderer and MorphingFrameRenderer that we could enable custom morphing behaviour across URL changes. For example, we have a page where a difference in URL means that a sidebar is open or not with certain content. In order to fit this use case, we have had to replicate the morphing functions. We were planning on scrapping our custom implementation and using the exposed renderers once this was merged.

Is there any plans to eventually expose MorphingPageRenderer and MorphingFrameRenderer, or not?

[botandrose]

@jordancoil While I can't speak to your question about exporting those classes, I wonder if you could accomplish what you describe with less coupling to Turbo's internals (and less of a public API commitment from Turbo) by hooking into the existing rendering events: https://turbo.hotwired.dev/reference/events

[krschacht]

@jordancoil I'm not sure that you've lost any flexibility. I think it should work for you to catch the event:

  frame.addEventListener("turbo:before-frame-render", ({ detail }) => {
    detail.render = CustomFrameRenderer.renderElement
  }, { once: true })
}

You might need to fiddle with capture: true/false to get the order of firing correct, but by default your event listener should fire last so you'll get to set the final detail.render value which will be used.

[jordancoil]

@botandrose @krschacht thanks for the suggestions. Listening to the turbo:before-render and turbo:before-frame-render events is essentially what we have done. The thing we are missing out on is having to manually update our custom frame renderer with upgrades and bug fixes in the turbo repo. We are pretty much using the exact same morphing behaviour, so it would have been nice to just call something like detail.render = Turbo.MorphingFrameRenderer.renderElement, and not have to worry about updating our custom implementation.