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

Provide a way to lock/unlock elements purely client-side #3516

Open
SteffenDE opened this issue Nov 18, 2024 · 31 comments
Open

Provide a way to lock/unlock elements purely client-side #3516

SteffenDE opened this issue Nov 18, 2024 · 31 comments

Comments

@SteffenDE
Copy link
Collaborator

When using external libraries like motion.dev with LiveView, we'd ideally have a way to trigger those animations similar to how we do a JS.transition. The important part is that we'd want to support locking the elements to prevent them from being patched while the animation is running.

I think we could extend the JS commands (on the client) to also support a locking API, which we could then handle similar to how we handle locked elements for regular server events (patching a clone in the background and applying the updates on unlock). We should also handle those locked elements in phx-remove to allow custom animations when an element is removed.

cc @chrismccord @bcardarella

@bcardarella
Copy link
Contributor

bcardarella commented Nov 18, 2024

Thanks @SteffenDE. i'm currently using JS.dispatch to send to an event listener to trigger Motion's animation.

It was suggested to use JS.transition to manage the "locking" but timers in JS are not precise and I'd rather have explicit control from Motion's own complete/cancel callbacks on when to unlock elements.

@SteffenDE
Copy link
Collaborator Author

For phx-remove it is probably important to lock the whole DOM like JS based transitions do. So that might be a separate feature. Maybe we could just have

liveSocket.transition((done) => {
  // all DOM patching is paused for now,
  // animate and finally call done
  ...
  done()
})

as public API, but element-level locking might be interesting for other things?

@bcardarella
Copy link
Contributor

@SteffenDE what are your thoughts on being able to pass a promise to the lock itself? That way when the promise resolves or is cancelled the lock will disengage? It should probably also have a timeout so you aren't stuck in a locked state

@salvatorecriscioneweb
Copy link

salvatorecriscioneweb commented Dec 15, 2024

Hey @SteffenDE , I have the same issue ( but not only for animations but mostly for components ) , for example dropdown using just client state to avoid to keep this information on the server.

IMHO more than create a Promise callback (un)block i would suggest more a way similar to this ( allow to ignore specific element's attribute by phx-something-ignore-attrs or in the hook using this.ignoreDOMPatchAttributes([ "style" ]);

I created a test PR to check, something similar to this: #3574

@bcardarella
Copy link
Contributor

Is this under consideration at all? I'd like to know if I should continue with the motion JS command wrapper, without this feature I don't think it's worth continuing.

@bcardarella
Copy link
Contributor

I spoke with @chrismccord and I want to provide my understanding of what Chris's position is (if I'm misunderstanding this please correct me) as well as my feedback on what I understood.

Chris said there were two methods currently available to do what I want. The first is to lock the entire UI during the animation. The second is to use the JS.transition command.

My posisition is that neither solve the problem and are a misunderstanding of the underlying need.

Locking the entire UI is a non-starter. IMO LiveView shouldn't care if a client-side async events runs for 1 second or 100 years. The node's tree should be locked until the callback/promise/or a given timeout happens. (timeout should be optional but recommended so UI doesn't permalock). Locking the entire UI means that updates that are applied outside of the target element's sub-tree should still be able to update and I don't understand why this was a recommendation. It feels very counter to the position that LiveView is a modern client-side alternative to SPA reactive libraries which would never present this as an option.

Using the transition command for a timeout is also a very bad idea. Duplicating the timeout value may be something you can get away with every so often but should never be depended upon and will introduce race conditions. Again, the lock should be lifted after a callback/promise/or fallback optional timeout to prevent permalock.

Finally, having a formalize public API for this need will open the door to more interoperability with the wider JS ecosystem which is what we've been trying to tell people outside of Elixir that LiveView plays nice in that space. Currently, I don't think that's true without being able to properly introduce a public API for this.

@SteffenDE
Copy link
Collaborator Author

@bcardarella can you maybe show an example where locking is needed? I just tried a simple motion example of rotating a box and even with DOM patching inside, it works just fine: https://gist.github.com/SteffenDE/498469e6531a7ba8b9ba73d4dc32d5ca

So an example where the current way of integration is a problem would be helpful.

@bcardarella
Copy link
Contributor

@SteffenDE have your update delete the element from the DOM

@SteffenDE
Copy link
Collaborator Author

okay, so we're specifically talking about the phx-remove case, that helps :)

@bcardarella
Copy link
Contributor

@SteffenDE no I'm talking about how LV can patch when another async operation acting upon that element is already in-flight.

@SteffenDE
Copy link
Collaborator Author

I do think is is worth differentiating this though. Many updates can be applied without problem while things like animations or "another async operation" are running. But this doesn't apply when an element is removed.

How would an API you'd like to see look like?

@bcardarella
Copy link
Contributor

@SteffenDE yes but it should be up to the developer to make that call.

I described the API here #3516 (comment)

SteffenDE added a commit that referenced this issue Jan 4, 2025
Relates to: #3516

When integrating external animation libraries like motion.dev, the existing
JS functions are not sufficient. Instead, the third party library needs to
be triggered via JS. This has the downside of not being able to block the DOM
until the animation is complete, which prevents this from working when
elements are removed using `phx-remove`.

This commit introduces a new `blocking: true` option to `JS.dispatch/3`,
which injects a `done` function into the event's `detail` object.

Using this with motion could look like this:

```elixir
def render(assigns) do
  ~H"""
  <div :if={@show} phx-remove={JS.dispatch("motion:rotate", blocking: true)}>
    ...
  </div>
  """
end
```

```javascript
const { animate } = Motion

window.addEventListener("motion:rotate", (e) => {
  animate(e.target, { rotate: [0, 360] }, { duration: 1 }).then(() => {
    if (e.detail.done) {
      e.detail.done()
    }
  })
})
```

It is still necessary to block the DOM while the remove animation is
running, as the remove can happen because of a navigation, where the
animation would otherwise not run as the whole LiveView is just replaced.
@SteffenDE
Copy link
Collaborator Author

@bcardarella I thought about this a little more and came to the conclusion that those are separate issues. Locking elements to prevent DOM patching only for the element is something where I don't necessarily see the need for at the moment, but you can convince me if you can provide an example where it is indeed needed. For animations using external libraries, I propose #3615.

This adds JS.dispatch(event, blocking: true) which can be used to lock the whole DOM until the event handler calls event.detail.done().

It also adds liveSocket.asyncTransition(promise) which can be used from hooks

liveSocket.asyncTransition(async () => {
  await animate(...)
})

The important part is that you'll probably only want to use blocking: true or asyncTransition if your animation is used when the element is removed.

@bcardarella
Copy link
Contributor

bcardarella commented Jan 4, 2025

And what happens in the event that the dispatched event takes say 1 hour to complete? Locking the entire DOM to wait for a single async event to complete doesn't seem like a good idea. Locking the specific element's subtree does.

@SteffenDE
Copy link
Collaborator Author

If you want your animation to run when the element is removed, and it takes 1 hour for the element to be removed, then that is probably not good UI design.

Again, as I mentioned, for me those are separate issues. There might be real world reasons to want the functionality of "don't touch this element until I tell you" where lock/unlock makes sense. Currently, I'm only focusing on the use case "I want to animate elements with external libraries" in the same way as you can currently animate using JS.transition et al.

@bcardarella
Copy link
Contributor

@SteffenDE it doesn't need to be an animation, any async event. And I really don't think that LV core should be limiting API access based upon their opinions of UI design. If we need to go through the process of making this a request for general async then let's do that.

@SteffenDE
Copy link
Collaborator Author

Don't get me wrong, I agree with you that LV should provide all the necessary APIs. But let's try to work with a clear goal in mind. Please provide a concrete, real use case, where you need such kind of API.

Initially, you've been playing around with motion.dev. That's a good use case, but I think this one is handled sufficiently with #3615, allowing to use such libraries in an equal way as you can use the existing JS transition API. So let's focus on what isn't possible with that.

@bcardarella
Copy link
Contributor

My use case is I want to dispatch very long running animations and not have inbound patch updates cause jank or otherwise disrupt that animation.

@bcardarella
Copy link
Contributor

I'll add: while allowing other nodes in the DOM to continue to receive patches.

@SteffenDE
Copy link
Collaborator Author

@bcardarella here's one way to do it: https://gist.github.com/SteffenDE/3313b15160a1404d3a9101d0c2ab0f66; so I'm still not convinced a locking API is needed. And if I say "concrete, real use case", I'd love to see actual code that shows "jank" or something that currently is not possible.

@bcardarella
Copy link
Contributor

bcardarella commented Jan 4, 2025

@SteffenDE just three days ago you were in favor of this, you yourself opened the issue. I'm not sure what happened here but clearly there was activity and conversation that is not in the public so I have no ability to push back on that. This is a fairly self-evident problem and a very common need and pattern for UI frameworks in both JS and native. When it comes to two or more things acting on the same element or view concurrently there are good reasons to disallow that or delay one in favor of the other. Dismissing this because the animation lengths are not what you'd recommend I don't think is an inclusive way to approach this. I don't want to invest the time on producing an example then just to get the same response. So I think close this issue as a won't fix and I'll retire the library.

@SteffenDE
Copy link
Collaborator Author

clearly there was activity and conversation that is not in the public so I have no ability to push back on that

No, there was not.

just three days ago you were in favor of this, you yourself opened the issue

I opened the issue because I thought it could be a solution to the problem at hand and wanted to get some feedback (looking at @chrismccord). I did not get any. I also didn't invest the time to look into other solutions or to try working with motion.dev myself. I only heard about the fact that Chris seemingly does not like this from you. When chatting with you I always said that I'm in favor of LiveView providing the necessary APIs to work with external libraries, so while I didn't go through our conversation again before writing this answer, I don't think I ever said that I'm specifically for this lock/unlock functionality - that isn't even really defined yet.

So finally today I took some time to try working with motion.dev myself and thinking this through a little bit more. While doing this, I realized that the locking API I imagined for this does not really solve the problem of dealing with elements that are being removed. So while it would be possible to prevent updates to an element (like phx-update="ignore" does, but applying the patches automatically on unlock), such elements would still require locking the whole DOM as soon as they are removed due to a navigation. Otherwise, the parent container would be patched (-> removed) and you don't get a remove animation at all. With that in mind, I tried to get motion.dev to work consistently to what JS.transition does at the moment. And the only missing piece I've identified is the ability to lock the DOM while a remove transition is in progress.

So I opened up #3615, trying to get some feedback. I even demonstrated that, while this does not prevent patching the element on its own, you can also achieve "animate and prevent patching" with the current phx-update="ignore" and a subsequent patch.

I tried to get a concrete example from you where this would be problematic, but you've been quite vague, stating that it would be a "self-evident problem and a very common need" and such. If it is, it should be not too much work to create a showcase, no?

I don't want to invest the time on producing an example then just to get the same response.

I can assure you that I will change my mind when given new facts. So when you say the problem is self evident, please do convince me.

So I think close this issue as a won't fix and I'll retire the library.

Do you have some code public for this yet? I'd really like to see it in action. With it, showcasing the remaining problems should be a good way to move forward, shouldn't it?

@bcardarella
Copy link
Contributor

bcardarella commented Jan 4, 2025

@SteffenDE I appreciate that you're trying to be convinced here but my experience over the past year has been investing a lot of time to develop reproductions or videos of issues with YAGNI or no response. I don't mean for you to now be downstream of me getting burned on this repeatedly but I'm just past the point of continuing to invest time with the trust that this time it will work. As I mentinoed, I think it is just best to close this issue at this point. Thanks for your time.

@josevalim
Copy link
Member

josevalim commented Jan 4, 2025

Closing this per above and in favor of #3615. It is clear #3615 applies only to certain cases, so if folks need more locking APIs, please open up new issues. If you do so, please try to provide an example of where the functionality is needed, so we can tinker with it.

@salvatorecriscioneweb, please give #3615 a try and let us know if it is enough for you. Thanks! ❤️

@josevalim josevalim closed this as not planned Won't fix, can't repro, duplicate, stale Jan 4, 2025
@nerdyworm
Copy link

An example was requested and I just happen to have one!

Please keep in mind, this was just a weekend messing around with things, didn't really intend for this to be anything other than a personal learning exercise. Also, I do feel like this is the sort of video game world would be best handled completely client side. It's a blessing that I can get it this far without much javascript 😁

Background for this example: I'm trying to create an AI NPC village. Basically, little characters that go about thier day, finding food, sleeping, talking with each other. You can see the demo video here: https://bsky.app/profile/bennyjamins.bsky.social/post/3leq4uqpkds2t

For the most part, live view was fantastic at syncing world state to the browser, really helped me get my proof of concept done in a day. Always sparks joy :)

However, there was one particular issue with trying to get NPCs to walk smoothly from one square to the next.

I was able to find a quick and dirty walk around (previously mentioned above):
https://github.com/nerdyworm/goap-demo-thingy/blob/main/apps/demo_web/assets/js/app.js#L30

element.style.transition = "transform 0.5s linear"; 
element.style.transform = `translate(${offsetX}px, ${offsetY}px)`;
element.setAttribute("phx-update", "ignore");

setTimeout(() => {
    this.pushEvent("walked", event);
    element.removeAttribute("phx-update");
}, 500);

Here is the live view: This ticks every 500ms, plans/updates the world state, and runs the actions in a Task, which then update world state.
https://github.com/nerdyworm/goap-demo-thingy/blob/main/apps/demo_web/lib/demo_web/live/world_live.ex

While this solution worked well enough for my proof of concept, it didn't doesn't really work smoothly when there was added latency involved or many NPCs running together (you can see some NPC teleporting). To be fair, this is something that is expected. I should really send the entire planned path to the client and animate the whole thing instead round tripping as often as it does.

I hope my toy project helps demonstrate the need for this api. It would be amazing to be able to build a world without having to resort to writing the entire thing on the client.

Thanks!

@josevalim josevalim reopened this Jan 5, 2025
@josevalim
Copy link
Member

Thanks @nerdyworm, this is actually quite helpful. Am I wrong to say that, in this case, blocking the whole client from receiving server updates while animations are on going, is probably the simplest choice you can make?

We could add APIs for locking individual components, but that could introduce other issues. For example, maybe a server update makes a square become a black hole, but that would happen while the NPC is walking over it, which would equally become as jarring. Blocking the UI from any update is the simplest way to guarantee you don't see any other inconsistency.

If that's too coarse-grained, then I assume you would still want to lock major pieces, like the whole board, instead of each NPC individually?

@nerdyworm
Copy link

I think blocking the whole client from patching would work really well for the general live view animation use case.

The game world use case would require more fine grain locking. In general, always something animating, so it would get awkward trying to figure out when to patch. Maybe we could pause animations, patch, and then resume animations?

We have to lock each NPC because each one of them can change from 😄 to 😴 while others are animating.

Maybe the real solution here is to just render the NPCs purely client side. It actually seems more reasonable the more I think about having to deal with locking/unlocking/patching individual components. I also don't want to send ya'll down a rabbit hole for a niche use case 😁

@josevalim
Copy link
Member

The game world use case would require more fine grain locking. In general, always something animating, so it would get awkward trying to figure out when to patch. Maybe we could pause animations, patch, and then resume animations?

I may be misrepresenting how LV works but I don't think you would have to worry about figuring out when to patch. As you receive events from the server, those events would block the UI when applied. Meanwhile, you continue receiving server events, which get automatically queued and are automatically applied when the previous animation finishes. As so on.

If you think about it, even if you were to roll your own solution with JS, could it be pretty similar? While animations happen, you would queue events from the server, and consume them once the animations are over. The difference is that this machinery is already all within LiveView, so you might as well use it.

However, the LV solution above won't work if the animation is initiated fully independently by the client. Then you are indeed correct you may enter a loop where the server patch is never applied, because the UI is always locked. But if everything comes from the server, then it is all ordered in single-queue anyway for you. However, we could also perhaps solve this by making sure all of the UI locking requests go through the same queue we put the server events on. Then it is guaranteed everything will run in the order they arrive. Both client and server operations.

@SteffenDE
Copy link
Collaborator Author

@nerdyworm I think in this case I would just position the bots absolutely on the grid, then you don't need a hook at all, and the bots are just moved from one position to another, which can be smoothly animated:

diff --git a/apps/demo_web/assets/js/app.js b/apps/demo_web/assets/js/app.js
index 330e7f7..d2c2e79 100644
--- a/apps/demo_web/assets/js/app.js
+++ b/apps/demo_web/assets/js/app.js
@@ -22,61 +22,11 @@ import { Socket } from "phoenix";
 import { LiveSocket } from "phoenix_live_view";
 import topbar from "../vendor/topbar";
 
-let hooks = {};
-
-hooks.game = {
-  mounted() {
-    console.log("hi");
-    this.handleEvent("walk", (event) => {
-      console.log(event);
-      const element = document.getElementById(`${event.name}`);
-
-      const target = document.getElementById(
-        `${event.target_x},${event.target_y}`
-      );
-
-      if (!element || !target) {
-        console.error("Element or target not found");
-        return;
-      }
-
-      // target.classList.add("bg-red-500");
-
-      // Get bounding rectangles relative to the grid container
-      const elementRect = element.getBoundingClientRect();
-      const targetRect = target.getBoundingClientRect();
-
-      // calculate to target rect's center
-      const offsetX =
-        targetRect.left +
-        targetRect.width / 2 -
-        (elementRect.left + elementRect.width / 2);
-
-      const offsetY =
-        targetRect.top +
-        targetRect.height / 2 -
-        (elementRect.top + elementRect.height / 2);
-
-      // Apply the translation
-      element.style.transition = "transform 0.5s linear"; // Smooth animation
-      element.style.transform = `translate(${offsetX}px, ${offsetY}px)`;
-      element.setAttribute("phx-update", "ignore");
-
-      setTimeout(() => {
-        this.pushEvent("walked", event);
-        element.removeAttribute("phx-update");
-        // target.classList.remove("bg-red-500");
-      }, 500);
-    });
-  },
-};
-
 let csrfToken = document
   .querySelector("meta[name='csrf-token']")
   .getAttribute("content");
 
 let liveSocket = new LiveSocket("/live", Socket, {
-  hooks,
   longPollFallbackMs: 2500,
   params: { _csrf_token: csrfToken },
 });
diff --git a/apps/demo_web/lib/demo_web/live/world_live.ex b/apps/demo_web/lib/demo_web/live/world_live.ex
index 60ee0b8..cfb46b3 100644
--- a/apps/demo_web/lib/demo_web/live/world_live.ex
+++ b/apps/demo_web/lib/demo_web/live/world_live.ex
@@ -28,7 +28,7 @@ defmodule DemoWeb.WorldLive do
 
   def render(assigns) do
     ~H"""
-    <div id="game" phx-hook="game">
+    <div id="game">
       <div class="text-xs space-y-1 relative">
         <div :for={row <- [email protected]} class="flex space-x-1">
           <div
@@ -36,17 +36,6 @@ defmodule DemoWeb.WorldLive do
             class="size-14 border flex items-center justify-center relative"
             id={"#{row},#{col}"}
           >
-            <% bots = Enum.filter(@world.bots, &(&1.state.x == row && &1.state.y == col)) %>
-            <div
-              :for={bot <- bots}
-              id={bot.name}
-              class="z-10 bg-blue-100 w-4 h-4 flex items-center justify-center text-xl"
-            >
-              <span :if={bot.state.hungry}>😋</span>
-              <span :if={bot.state.tired}>🥱</span>
-              <span :if={bot.state.sleeping}>😴</span>
-            </div>
-
             <% food = Enum.find(@world.foods, &(&1.x == row && &1.y == col)) %>
             <div :if={food}>🍲</div>
 
@@ -57,6 +46,16 @@ defmodule DemoWeb.WorldLive do
             </div>
           </div>
         </div>
+        <div
+          :for={bot <- @world.bots}
+          id={bot.name}
+          class="z-10 bg-blue-100 w-4 h-4 flex items-center justify-center text-xl absolute top-0 left-0 transition-transform ease-linear duration-500"
+          style={"transform: translate(#{bot.state.y * (56 + 4) + 20}px, #{bot.state.x * (56 + 4) + 16}px)"}
+        >
+          <span :if={bot.state.hungry}>😋</span>
+          <span :if={bot.state.tired}>🥱</span>
+          <span :if={bot.state.sleeping}>😴</span>
+        </div>
       </div>
 
       <div class="text-xs">
@@ -106,15 +105,8 @@ defmodule DemoWeb.WorldLive do
         socket
 
       {:action, bot, %{name: "walk"} = action}, socket ->
-        # :timer.sleep(100)
-        # send(pid, {:ran, bot.name, action})
-        state = Demo.Bot.walk_to(bot.state, %{x: bot.state.target_x, y: bot.state.target_y})
-
-        push_event(socket, "walk", %{
-          name: bot.name,
-          target_x: state.x,
-          target_y: state.y
-        })
+        Process.send_after(self(), {:ran, bot.name, "walk"}, 500)
+        socket
 
       {:action, bot, %{name: "eat"} = action}, socket ->
         Task.async(fn ->
@@ -135,9 +127,4 @@ defmodule DemoWeb.WorldLive do
         socket
     end)
   end
-
-  def handle_event("walked", %{"name" => name}, socket) do
-    send(self(), {:ran, name, "walk"})
-    {:noreply, socket}
-  end
 end

Keeping it all on the server without any round trip to the client for walking also prevents issues with latency. Otherwise you'd always have the problem of the client possibly being too slow for the server ticks.

The only downside I can see with my change is that multiple bots in the same location are not neatly next to each other, but instead on top of each other. You could probably calculate in the state if there are multiple in the same location and provide some kind of offset for each.

@nerdyworm
Copy link

@SteffenDE Yeah, that works much better! 😄 I suppose the downside of having to calculate collisions on the server side wouldn't be too bad—seems doable without much fuss.

My mental model leans heavily towards the client handling more of the workload than the server. For example, determining the walking direction and applying the appropriate walking animation, or playing a sound when characters collide—these are user-facing features that don’t necessarily need to be computed on the server.

The thing is, I know how I’d build a fully js version of this, and I have a solid idea of how it could be done solely in LiveView (your suggestion is basically spot on). The challenge lies in blending the two approaches, which is what led me to consider the need for preventing an element from being patched.

The more I think about it, the more it feels like I’ve stumbled onto an anti-pattern. 🤣

That said, I’m not particularly convinced that I need to lock elements in LiveView—it was just a random issue that came up while having fun over the weekend.

@LannyBose
Copy link

LannyBose commented Jan 9, 2025

@SteffenDE and @josevalim In the spirit of providing other concrete use-cases, I wanted to share mine, even though I know #3616 is already moving along. This is an extraction from a project I'm currently working on:

Focusable and Filterable Bento Grid

It uses GSAP and its Flip plugin to smoothly render layout changes. I thought a non-Motion example might help.

Currently, hiding records (instead of :if={}-ing them) is the only way for me to get a before-and-after snapshot for GSAP to move items around. It looks like #3616 might be a solution since we're getting the opportunity to see fromEl and toEl and I think that beforeUpdate runs before things start actually getting removed from the DOM, but I wanted to throw this out there to:

  1. see if my inferences about the current PR are right
  2. possibly help influence that PR around another use-case in case it's close but no cigar

Thanks, all!

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

No branches or pull requests

6 participants