Bugfix: Don't use Bardo during morphing frame render (its only for non-morphing renders)

[botandrose]

Using the turbo frame morph renderer with permanent elements results in incorrect morphs. On v8.0.5, I'm seeing duplicate elements, missing elements, and other strange behavior. This took a while to track down!

It turns out that the data-turbo-permanent functionality is implemented via two different strategies: Bardo for non-morphing renderers, and DefaultIdiomorphCallbacks for morphing renders.

MorphingFrameRenderer's parent class FrameRenderer uses Bardo, so MorphingFrameRenderer continues to use that in addition to DefaultIdiomorphCallbacks, resulting in incorrect morphs.

Looking at how the morphing and non-morphing page renderers handle this strategy switch, MorphingPageRenderer works correctly, because its overwriting its preservingPermanentElements function with a no-op, so let's just copy that strategy in MorphingFrameRenderer, et voila.

[seanpdoyle]

Thank you. This implementation change makes sense.

Could you add a test case to ensure that permanent elements are not duplicated during frame morphing?

[brunoprietog]

Are you sure that this really resolves the error? Unless I'm missing something, this doesn't produce any change, since no instance of MorphingFrameRenderer is created yet. Only the static method is used separately here:

turbo/src/core/drive/morphing_page_renderer.js

Lines 42 to 48 in f48b0de

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

Or am I missing something?

[botandrose]

@seanpdoyle Yes, I looked into adding a test, but it looks like no morphing frame rendering tests exist at all at the moment, so there didn't seem to be a reasonable place to add one. If you want to gate merging this on a test, let's wait until #1192 lands, and then I'll rebase and add one.

[botandrose]

@brunoprietog Right you are, I've been manually creating an instance of MorphingFrameRenderer to refresh a turbo frame with morphing. I'm really looking forward to #1192 landing so I don't have to do that anymore!

[botandrose]

@seanpdoyle @brunoprietog Okay, now that #1192 has landed, I've rebased and added a test to assert that a permanent element remains after a frame[refresh=morph] reload. This test exposes the issue described above by failing after refresh, complaining that the permanent element now has a duplicate: locator('#permanent-input') resolved to 2 elements:. This is because Bardo and DefaultIdiomorphCallbacks are both cooking in the kitchen. I'll push the fix as a separate commit now.

[botandrose]

@seanpdoyle @brunoprietog Okay, I got the test passing by disabling Bardo for morphing frame renders, as described above. However, I've had to completely change the implementation strategy here, because #1192 has since turned the MorphingFrameRenderer subclass into a bag of methods to monkeypatch onto the current renderer, rather than using an instance of it to perform the render. This isn't the approach I would choose, personally, but I'm sure there are good reasons. I'm guessing this is a transitionary state of affairs?

Edit: My mistake. I see from the code snippet Bruno posted above that it was this way before #1192. I think I had expected that PR to start instantiating MorphingFrameRenderer, but now I see that this didn't happen.

[seanpdoyle]

@botandrose thank you for exploring this.

I've modified #1028 to incorporate the test coverage you've introduced in this change. Piggy-backing on its original change, its implementation uses an instance of MorphingFrameRenderer (like you highlighted in #1303 (comment)) instead of a static MorphingFrameRenderer.renderElement method).

I've added you as a co-author on that branch. If you're able to review it, please provide feedback there. If you feel like it's a viable solution to the issues you've highlighted here, please close this issue.

[botandrose]

Closed in favor of #1028