Use Bardo for all permanence handling to fix lost permanent elements in reordering morphs

[botandrose]

Hi folks, thanks a ton for Turbo! I have been building a collaborative issue tracker app, and being able to write 99% of it in Ruby makes me smile every time. :D

Motivating use-case

However, one of the issue tracker app's primary use-cases has exposed a bug in the integration between Idiomorph and the data-turbo-permanent attribute. Each ticket contains a data-turbo-permanent checkbox to keep track of the client-side state of whether or not the ticket is currently expanded for that client or not. We also allow tickets to be reordered via drag-and-drop. The issue is that this data-turbo-permanent checkbox is not always preserved across morphs involving ticket reorders. There are other more serious ramifications of this bug (data loss!), but this is the simplest case to illustrate the issue.

Diagnosis

I have reduced this scenario down to the failing test case in this PR. I've dug into the DefaultIdiomorphCallbacks integration class and into the Idiomorph code itself, and I believe the issue ultimately lies within Idiomorph, specifically here: https://github.com/bigskysoftware/idiomorph/blob/f75fba125e6e3cbfaa31dfa2af11be94455c6a38/src/idiomorph.js#L212 . In the specific reordering case captured in the test, it seems that one of the two nodes is being removed completely and then added anew with fresh DOM elements, rather than being moved and morphed. Thus, the data-turbo-permanent handling in DefaultIdiomorphCallbacks is never even run. Or at least that's my assessment. I don't claim to have a full understanding of Idiomorph's internals yet! I would love another pair of eyes on this.

Next steps?

Also, I'm unsure of how to proceed. On one hand this could be considered a bug in Idiomorph, for not morphing as much as it conceivably could. But on the other hand, the data-turbo-permanent functionality is ultimately a Turbo feature, so perhaps this is within Turbo's purview, and maybe its worth looking into integrating Idiomorph with Bardo instead to provide the data-turbo-permanent functionality?

What are your thoughts?

[botandrose]

Idiomorph looks like its going through a bit of a dry spell with regards to maintenance, so I'm going to pursue the Bardo integration path.

[botandrose]

Okay, I got these tests passing with only using the delete key! Love it.

However, some other tests are now failing, and one of them in particular strikes me as incorrectly passing on main:

turbo/src/tests/functional/page_refresh_tests.js

Lines 273 to 286 in 0cc9508

	test("it preserves focus across morphs", async ({ page }) => {
	await page.goto("/src/tests/fixtures/page_refresh.html")
	const input = await page.locator("#form input[type=text]")
	await input.fill("Preserve me")
	await input.press("Enter")
	await nextEventNamed(page, "turbo:render", { renderMethod: "morph" })
	await expect(input).toBeFocused()
	await expect(input).toHaveValue("Preserve me")
	})
	test("it preserves focus and the [data-turbo-permanent] element's value across morphs", async ({ page }) => {

That last assertion! Should the input's value really be preserved in this case?? It doesn't have data-turbo-permanent, nor is the preservation specified in the test name. Note that the very next test's name asserts value preservation specifically WITH data-turbo-permanent, and adding that attribute is the sole difference between the two tests. So, is this assertion indeed in error, and perhaps even be flipped to assert that the value is empty?

For ease, here's a link to the fixture it's operating on:
https://github.com/hotwired/turbo/blob/0cc950891adb8075298faa99c34b5e5f988b4fc2/src/tests/fixtures/page_refresh.html

[botandrose]

Huh, also, that next test exposes inconsistencies because data-turbo-permanent is added dynamically in the test, so it isn't on the element in the response. This doesn't trip up the permanent handling in DefaultIdiomorphCallbacks, which doesn't require the response element to also have the attribute, but Bardo DOES require this. A subtle difference, but a difference nonetheless. In my personal experience, there is significant utility in being able to toggle permanence dynamically, so I submit that this should be considered a bug in Bardo, and it should be changed to allow this. Then, unifying the permanence handling system around Bardo should be possible.

[botandrose]

Okay, I've got something working here. This PR resolves the initial bug by using Bardo for all data-turbo-permanent functionality, including morphing renders. This resolves the initial motivating bug, and normalizes two other differences between the two implementations that I discovered along the way. Specifically:

1. Before, morphing renders only required the element to have the data-turbo-permanent attribute before the render. If the element didn't have the attribute in the replacement html, no problem. This allowed for client-side adjustment of what's permanent and not, which is quite useful in my experience. However, non-morphing renders were more strict, and required the data-turbo-permanent attribute to be on the element before the render, and also on the element in the replacing html. This has been relaxed and normalized so that both renderers only require the attribute on the initial html.
2. Breaking change: all data-turbo-permanent elements must also have the id attribute set in the before and after html. This was already the case in the non-morphing page and frame renderers, but is now the case in the morphing variants, as well. This is due to how Bardo works internally, and is the one drawback to this new approach that I can see. If we move forward with this approach, I think it might be worthwhile to print a warning in the console if data-turbo-permanent is set but no id, and write tests for these edge cases.

Also, the weird test behavior above turned out to be due to the test fixture setting up a global focus event listener, which toggled data-turbo-permanent during any element's focus and blur events! This was useful for an entirely separate test, but was affecting the result of the test in question in the manner that I described above. I've resolved this by changing the fixture to only apply that behavior to the relevant test, and updated the first test to assert the normal expected behavior.

Overall, I think the alternative to this PR would be to attempt to enhance Idiomorph's core algorithm to better handle morphs involving reorders. I'd be willing to give this a go, but honestly its a bit intimidating!

What are your thoughts?

[botandrose]

I'm closing this. While it is an overall improvement, I'm running into other more fatal ramifications from the core problem of Idiomorph deleting and readding siblings during a reordering morph. I have no choice now but to try to address this directly within Idiomorph.

The good news is that I have a proof-of-concept that demonstrates my desired outcome is indeed possible. I had worried that it ultimately came down to fundamental limitations of the DOM API, as described in whatwg/dom#1255 . But my PoC demonstrates that there is clear room for improvement between status-quo Idiomorph and some future Idiomorph using upcoming DOM movement APIs.

Diving into the Idiomorph guts now. Wish me luck.

[botandrose]

bigskysoftware/idiomorph#61