Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behavior when morphing a turbo frame src #1158

Closed
danecando opened this issue Feb 4, 2024 · 11 comments
Closed

Unexpected behavior when morphing a turbo frame src #1158

danecando opened this issue Feb 4, 2024 · 11 comments

Comments

@danecando
Copy link

danecando commented Feb 4, 2024

In my application I have a modal that gets triggered to open when a turbo frame inside of it gets loaded (pretty common pattern). The main use cases for my modal are new and edit pages. Before morphing on an edit for example, I would do a redirect from the update controller on success and the replace operation would remove the modal since the content from the redirect has it's default state with nothing loaded in the turbo frame.

With morphing enabled, the morph operation gets applied to the modals turbo frame to remove the src and everything seems dandy but immediately afterward the src attribute gets reset to the last value and the modal pops back open. I don't see any turbo event correlated with the second mutation of the src attribute. I've spent a bit of time trying to trace it with no luck.

I've included a quick screen recording to highlight the issue and a repo with a minimal reproduction of the issue with a simple stimulus modal.

https://github.com/danecando/turbo-frame-morph

Screen.Recording.2024-02-04.at.9.31.13.AM.mp4

Here's the same thing with a replace operation instead of a morph

Screen.Recording.2024-02-04.at.10.07.09.AM.mp4
@nflorentin
Copy link

nflorentin commented Feb 8, 2024

@danecando Did you try to try to add the attribute refresh="morph" on your turbo-frame ?

I encountered the same issue and this attribute did the trick. It avoids the reset of the src and the request.

I don't know if using this attribute has any drawback though...

EDIT: it does not do the trick... Modal does not pop up BUT the request is fired...

@danecando
Copy link
Author

danecando commented Feb 8, 2024

@nflorentin I don't think that it what I want. I'm expecting the frame src to be unset during the morph. Here, since the new page is morphed the frame src should be empty (that is its default state)

The confusing part is that it applies that operation but something resets it back right after

@nflorentin
Copy link

nflorentin commented Feb 8, 2024

@danecando you are right, I misunderstood.

Meanwhile, I removed the modal manually with turbo stream :/

@DLawla
Copy link

DLawla commented Feb 9, 2024

@danecando we are seeing the same thing with the same modal pattern, and came to the same conclusion as you.
I can't help but wonder if there is a race condition of some sort; the morphing does clear the turbo frame (and src), but not before the turbo_frame makes a request to to the src again.
I'm not sure what the fix will be, but we will use the default replace refresh until we can get to the bottom.

@DLawla
Copy link

DLawla commented Feb 9, 2024

I'll need to do some debugging, but this might be by intention from reviewing the source.

@radanskoric
Copy link

radanskoric commented Feb 12, 2024

I was curious to figure out exactly why this is going on so I ran your app locally and went in with the web tools debugger. It's a little "fun" dance that happens and I'm pretty sure it's a bug.

I'll explain in detail how it happens but first a small remark.
The sample app does not reproduce the bug your describe. Since in the update section your redirect back, and you just came from the edit page, you send back the edit page. I guess this was unintentional because visually it looks the same. It's just that if you render the edit page there's nothing strange with the behaviour. If I change it to redirect to index page I get exactly the bug your describe:

diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
index 0cb46c4..511fc52 100644
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@@ -27,7 +27,7 @@ class PostsController < ApplicationController
 
   def update
     if @post.update(post_params)
-      redirect_back_or_to @post
+      redirect_to posts_path
     else
       render :edit
     end
@@ -42,4 +42,4 @@ class PostsController < ApplicationController
   def post_params
     params.require(:post).permit(:title, :body)
   end
-end
\ No newline at end of file
+end

Ok, so with that out of the way, let me explain what's going on:

  1. When you click on the edit button it sets the src on the modal turbo and it also goes through some lifecycle statuses (busy -> complete) so it ends up with the following tag:
<turbo-frame data-modal-target="frame" id="turbo-frame-modal" target="_top" src="http://localhost:3000/posts/1/edit" complete="">
  1. When you finish editing and redirect back to the index page, the idiomorph algorithm does the morphing. Since the algorithm tries to minimise changes to the DOM, when it finds the matching element it will modify the attributes on the existing element to match the new content. Specifically, it attributes that the old content has and new doesn't. In this case that's going to be src and complete. If you look at the code you can see that it deletes from the list backwards. Remember this, this is important.

  2. Turbo observes a few attributes on the turbo frame element and runs some logic on its changes. Specifically, it will trigger loading of a remote frame (i.e. frame with src property) when completed element is removed. This is very clearly intentional based on the commit message that introduced it: "The act of "reloading" involves the removal of the [complete] attribute"

  3. Since completed is always set after the src, it is also in the attributes list after the src and since idiomorph deletes attributes from the back (told you it will be important :D) it means that completed is removed before src. On removing src, Turbo observes removal of completed AND it also observes that the frame has a src and it triggers loading it async.

  4. A moment later, idiomorph removes the src element as well, but too late, the Fetch call is already on the way.

  5. When the Fetch request comes back it doesn't do any extra checking and it not only puts the content into the frame but it also sets the source attribute to the url of content it just fetched. Presumably that is done so we can be sure that the content of the frame always matches the src attribute, even in case of multiple competing requests.

Given how it's a bunch of different unrelated mechanisms interacting in a strange way, I am pretty sure it's a bug. Especially considering that the behaviour would be completely different if idiomorph didn't do this small internal optimisation of removing attributes starting from the back.

The most likely correct fix seems to be postponing all "attribute changed" callbacks until idiomorph is finished. This would mean that by the time they fire they would see the real final state, instead of some random intermediate state that idiomorph is going through. But that introduces some extra complexity. Maybe it's worth thinking a bit about how to reduce the number of hidden assumptions that the turbo frame lifecycle mechanism (as encoded in FrameController) makes?

@laptopmutia
Copy link

laptopmutia commented Feb 27, 2024

this strangely happens to me too, idk how could this possible, because when I see it on network tab of inspect elements

its make a strange request to show the edit page again, maybe there is a turbo_stream response thus the behavior

I will check more about it, after my mom finished cleaning my room forcefully, can't help, she have cleaning ocd

I think this one is related https://www.reddit.com/r/rails/comments/1as69jv/do_i_need_to_use_turboactionreplace_for_morphing/?rdt=44534

@itsameandrea
Copy link

@danecando - did you manage to find a workaround (other than using replace instead of morph)?

@pfeiffer
Copy link
Contributor

This is a good catch!

I agree that it seems like a bug, that's triggered by a bunch of unrelated mechanisms and the way Idiomorph does it's thing.

A potential workaround for now could be something like:

addEventListener("turbo:before-morph-attribute", (event) => {
  const { target, detail } = event

  if (target.tagName === "TURBO-FRAME" && detail.attributeName === "complete" && target.hasAttribute("src")) {
    event.preventDefault()
    return false;
  }
})

.. which would prevent the removal of the [complete] attribute from Turbo Frames which have a [src] and are completed. If would still morph the [src] after that, ensuring that any changes in that attribute would trigger a new fetch of the frame

@alexgerber
Copy link

alexgerber commented Mar 15, 2024

Have you guys updated to the latest version? I believe it's fixed now.

#1175

@pfeiffer
Copy link
Contributor

Have you guys updated to the latest version? I believe it's fixed now.

#1175

Awesome! I can confirm that it's working with the latest release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

8 participants