Morphing + complete frame issue : Stop reloading turbo frames when complete attribute changes

[Intrepidd]

I encountered the following problem :

consider a turbo frame like so on index.html

<turbo-frame id="foobar">
  <a href="/some/path">Click me</a>
</turbo-frame>

When the link is clicked, the HTML will look like this

<turbo-frame id="foobar" src="/some/path" complete>
  Some content
</turbo-frame>

If the page is then refreshed with morphing, my expectation was that everything will return to the initial state.
However, in practice, the frame first appears to be reset, but then is reloaded and ends up in the second state.

This is because we have a callback that reloads the frame when there is any change on the complete attribute :

turbo/src/core/frames/frame_controller.js

Lines 101 to 105 in e2e5782

	completeChanged() {
	if (this.#isIgnoringChangesTo("complete")) return
	this.#loadSourceURL()
	}

The morphing causes this attribute to change thus the frame to reload.

I don't really understand why we want to reload a frame when the complete attribute changes, isn't it supposed to be only a read-only value ?

This PR removes the callback on the complete attribute, this is probably very naive, happy to explore another route if you point me in the right direction !

Repro and video in this comment : #1175 (comment)

[Intrepidd]

figured it's a bit safer

[jorgemanrubia]

@Intrepidd if you mark the turbo-frame with refresh="morph", when a page refresh happens:

1. It won't get replaced.
2. It will reload, and its contents will be reloaded with morphing.

Would that cover your use case?

[Intrepidd]

No, I want the frame to be replaced with the content from the page that is being refreshed.

Imagine a page where you have a list of record as rows, and by clicking on an edit button (one on each row) you transform the row to a form, through a turbo frame.

When submitting the form, you expect it to "close" and render as if you refresh the page ( this behavior happens naturally )

But with a morph refresh, this will happen then a few milliseconds later the form will "open" again.

I can try to write a repro if that helps understanding the issue ?

[Intrepidd]

@jorgemanrubia here is a repository with a repro : https://github.com/Intrepidd/turbo_8_bug_demo

And here is a video of the issue

CleanShot.2024-02-09.at.10.03.56.mp4

As you can see, the frame first resets with the content of the index (expected) but then since the morphing removes the complete attribute, the callback is executed and the frame is loaded from its src attribute

[jorgemanrubia]

@Intrepidd changing the complete callback behavior would be a breaking change, so I don't love that.

As a workaround here, you could prevent the morphing of the attribute relying on the event. Is not ideal, but that should do it.

For example with this stimulus controller:

import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
  prevent(event) {
    event.preventDefault()
  }
}

To use like:

<%= turbo_frame_tag "frame", data: { controller: "event-default", action: "turbo:before-morph-attribute->event-default#prevent" }  do %>
# ...

As a longer term solution, I wonder if we could use some convention around the current refresh=morph attribute. For example, if the new turbo-frame arrives without a URL, instead of reloading the frame morph the contents 🤔 .

[Intrepidd]

@jorgemanrubia Thanks for having a look !

I understand, however this callback is not documented as far as I know, and doesn't seem to make sense in the first place, I fail to understand when such behaviour is expected or wanted. Do you know why it is here in the first place ?

I would argue that now more than ever is the perfect time to make a "breaking" change as people are going to upgrade from 7 to 8 and expect some breaking changes.

As a longer term solution, I wonder if we could use some convention around the current refresh=morph attribute. For example, if the new turbo-frame arrives without a URL, instead of reloading the frame morph the contents 🤔 .

There is definitely going to be some improvements around the refresh attribute of turbo frames I believe, as many behaviours can be desired ( I was toying with the idea of having refresh="none" )

But this would still leave the broken behaviour when no refresh attribute is specified, wouldn't it ?

[jorgemanrubia]

@afcapel @seanpdoyle what do you think about removing that callback? I'm trying to think of real scenarios where one wants to mutate the complete and get this behavior, but I can't think of any example. That was added here. Could you add some context there @seanpdoyle?

[seanpdoyle]

Do you know why it is here in the first place

I'm trying to think of real scenarios where one wants to mutate the complete and get this behavior, but I can't think of any example.

From the commit message (the [loaded] attribute was renamed to [complete] before merging that PR):

The act of "reloading" involves the removal of the [loaded] attribute,
which can be done either by FrameElement.reload() or
document.getElementById("frame-element").removeAttribute("loaded").

[jorgemanrubia]

Thanks for the context @seanpdoyle, I had missed that.

It seems that this is an internal implementation detail. No reference to this behavior in the docs. So it seems that @Intrepidd original approach is good: it's a callback we can remove. Of course, we need to make sure that reload() reloads the frame as expected.

[Intrepidd]

Good ! Thanks to you both 🙏

What are the next steps to make it a go ?

We should probably write a regression test but it's a bit tricky to test that the frame does not reload after a bit of time :

- navigate inside the frame
- trigger a morph
- make sure the content is the one we expect
- wait a bit?
- make sure the content has not changed ? 

[jorgemanrubia]

@Intrepidd why not try to imitate the example you used in the description for the test: after morphing, the turbo frame contents – which changed through the test – should be reset to the initial state?

[Intrepidd]

@jorgemanrubia the problem is that it does get changed to the initial state but shortly after switches to the navigated state once the frame has been reloaded. So I'm afraid such test will pass even if the frame gets reloaded in the end.

I'll draft up something and we can iterate if needed

[Intrepidd]

@jorgemanrubia I managed to write a spec that seems to fail on main and to pass on this branch ! However it appears to be flaky, although I can't seem to reproduce the flakyness with --headed on my machine, do you have any insights ? Thanks !

[benwalks]

Hi @jorgemanrubia, @Intrepidd, any updates on this PR? This issue is affecting my projects as well, am happy to help.

[Intrepidd]

Waiting as well regarding the specs 🙏

[jorgemanrubia]

@Intrepidd sorry what's the problem with the specs? The referred flakiness? Is that happening in CI? We definitely need to sort that out, if you want to help with that @benwalks.

[Intrepidd]

@jorgemanrubia my specs were indeed flaky but I think I managed to fix it. So I don't think there are any blockers anymore for this ?

[Intrepidd]

Thanks a lot @jorgemanrubia for merging !

Any chance we can get a release soon ? As I believe this issue is a blocker for some to update to turbo 8. Thanks !

[afcapel]

@Intrepidd I've just cut a new release https://github.com/hotwired/turbo/releases/tag/v8.0.4

[jonsgreen]

I actually was relying on removing the 'complete' attribute as a convenient way to be able to reload a turbo_frame. I was doing it using turbo_power and sending turbo_streams like turbo_stream.remove_attribute('#comp-packages', 'complete'). Is there an official way to trigger a turbo_frame to reload from its src?

[alexgerber]

I actually was relying on removing the 'complete' attribute as a convenient way to be able to reload a turbo_frame. I was doing it using turbo_power and sending turbo_streams like turbo_stream.remove_attribute('#comp-packages', 'complete'). Is there an official way to trigger a turbo_frame to reload from its src?

You may use the reload method.

FrameElement.reload() is a function that reloads the frame element from its src.

[jonsgreen]

Thanks @alexgerber. I have discovered that turbo_power has turbo_stream.turbo_frame_reload which is perfect!

[ghiculescu]

Thanks for fixing this @Intrepidd 😍