LiveView Native Clients: Strange behavior with Picker and conditionally rendered Image elements

[supernintendo]

Using a Picker and a native binding to conditionally render content within a case statement can cause errors if that conditional content contains Image elements. The error seems to result from rendering any number of Image views and then updating the assigns so that when the LiveView re-renders there are less Image views than before:

https://user-images.githubusercontent.com/5893007/229330166-062f23fc-0502-40c9-b5a8-b9fa09b87852.mov

https://user-images.githubusercontent.com/5893007/229330174-78f5b357-e973-4b42-82f4-ebc33c9a1eba.mov

You can reproduce this with the bleeding-edge branch of the scratchboard repo (link). It uses a forked version of this repo but the changes are mostly limited to adding new modifiers.

[carson-katri]

Minimal example:

defmodule AppWeb.IndexLive do
  use AppWeb, :live_view
  use LiveViewNative.LiveView

  def mount(_params, _session, socket) do
    {:ok, assign(socket, show: true)}
  end

  def render(assigns) do
    render_native(assigns)
  end

  def handle_event("hide", _params, socket) do
    {:noreply, assign(socket, :show, false)}
  end
end

<%= if @show do %>
  <Image system-name="ant" />
<% end %>
<Button phx-click="hide">Hide</Button>

[shadowfacts]

So, this issue has to do with how Core generates diffs. Essentially, when you click the Hide button in @carson-katri's example, Core produces a patch list that changes the Image element into a Button and then removes the second Button, rather than just removing the Image. Because we store NodeRefs across document "generations", elements appear to change out from under views. This also causes issues (though not as severe) elsewhere, like when removing live forms.

Unfortunately, changing that behavior in Core for cases more complex than that example is a substantial refactor. The expected behavior (elements being able to move around in the tree, rather than being removed and re-added elsewhere) is at odds with how Core generates diffs by walking both old & new trees simultaneously (see this Slack thread for more details). It may be the less efficient, but I think that the correct solution may be to switch to an approach more similar to how morphdom works[^1].

One possible way around that big refactor that could solve this issue (big emphasis on the "could", I have not fully thought this through) is having element changes be generate diffs that are a InsertAfter and a Remove patch, rather than a Replace patch. That would end up using a different ref for the new element, which solves the dangling observers issue that's the source of the crash here. However, the underlying issue would remain, and the new and old Buttons would still be distinct in terms of SwiftUI view identity (potentially resulting in the loss of view state, for other views).

cc @KronicDeth, thoughts on either possible refactor?

[^1]: When the new DOM comes in, morphdom will look at each node in the new DOM and try to find the matching element (child of the existing node corresponding to the new node's parent that shares the same id or tag name) in the existing DOM, regardless of order. So, in Carson's example, it'd look at the new DOM, see the <Button>, and then go through the children of the root node in the original DOM until it found the original <Button>, and perform any necessary changes internal to that element. Then it'll go through any remaining original nodes that didn't have corresponding new nodes (just the <Image>, in this case) and remove them.

[KronicDeth]

I would count this as a bug in Core because it doesn’t replicate the Morphdom algorithm correctly and we should fix it to handle the updates based on ID like morphdom.

[AZholtkevych]

Blocked by https://github.com/liveview-native/liveview-native-core/issues/17

[Bajix]

On the core side, this issue is resolved by the morphing algorithm in PR https://github.com/liveview-native/liveview-native-core/pull/26 but will require additional work on the iOS side to support Patch::Detach and Patch::InsertBefore. In the new version, Morph is an iterator of patches, so using VecDeque<Patch> is now optional.

[supernintendo]

This has been resolved in this repo as well. Closing...

[AZholtkevych]

reopening since https://github.com/liveview-native/liveview-native-core/pull/26 has not merged yet. Sorry