Shuffled order of components

[vi4m]

I've found some interesting behavior related to Views drawing order.

I'd like to show "Add", "List" views depending on the window location hash. In this example code components order is not deterministic. Is this because of some Pub/Sub delay?

1. When i visit it the first time, it works 100% correctly

2. After clicking "list", it should show "begin" + list + "end", but the order is messed up.

3. Refreshing with Cmd+R always helps redrawing everything in the correct order. Here's reproducible case in one file:

https://gist.github.com/vi4m/300dffe461811aa96dae210929ddb5e5

[MaxDesiatov]

@vi4m thanks for the detailed report! I'll be able to have a closer look in the next few days, unless someone else picks this up in the meantime.

[MaxDesiatov]

Thanks again for raising this, and sorry for the delay. I've narrowed it down to this very simple case:

struct Choice: View {
  @State private var choice = false

  var body: some View {
    HStack {
      Button("Trigger") {
        choice = true
      }
      if choice {
        Text("true")
      } else {
        VStack {
          Text("false")
        }
      }
      Text("end")
    }
  }
}

The fact that the Text element in the mounted views tree is replaced with a VStack currently causes these elements to be remounted. The former is removed and the latter is added with insertAdjacentHTML. But during the insertion of the new element it doesn't have access to its siblings, only to its parent, and we mount it with the beforeend argument passed to insertAdjacentHTML. It inserts the element as the last child of its parent. That is clearly wrong. I currently see two possible solutions for it:

1. Allow elements to reference their previous siblings during remounting and use insertAdjacentHTML with afterend on the sibling instead of beforeend on the parent. I've started with this option and it touches too much of our renderer and reconciler code and complicates things significantly.
2. In these remounting scenarios we shouldn't remove the DOM element and insert a new one, but just set a new outerHTML value on it. I think this will be a less invasive refactor, and hopefully implies only a slight change to existing renderer callbacks or an addition of a new one. I haven't tried it yet, but so far it looks more promising to me.

This is the current status, and I hope this will be resolved in the next few days. I will keep you updated!

[MaxDesiatov]

Unfortunately, the second option also didn't work as I hoped. In this simple example, an update on the existing DOM node is not currently possible. In Tokamak terms Text is a primitive "host" node, while VStack<Text> is a composite node. Simply updating one with the other is not possible with the current code, as we need to rebuild the internal tree of elements, and then remap existing DOM elements to this new internal tree.

From this perspective, unmount/remount process described in the first option seems a bit cleaner. But then again we need to pass the position in the tree to the renderer somehow. For the last few days I tried to do that by passing element indices computed from the internal elements tree to the renderer directly. While this works in simple cases, there's an edge case with composite group elements that don't have underlying nodes. In this module views would have these indices:

Group {
  Text("foo") // 0
  Text("bar") // 1
  Group {
    Text("baz") // 0
  }
}

As we see, the index for the "baz" view would be incorrect, it should be 2 for updates and remounts. But then our reconciler logic should be more aware of these nested structures to assign correct indices. I'm still working on that, but if anyone has better ideas, please let me know.

[MaxDesiatov]

@vi4m I've resolved the issue in #301, but I want to keep this open until we have proper automated tests added for it on CI. There are multiple important cases I had to cover and tested manually, as we don't have end-to-end automated tests that run in the browser yet. As soon as automated end-to-end browser testing is enabled, I want to cover these important cases to prevent any breakage in the future.

Thanks again for uncovering it! It's really a bit embarrassing that we didn't know about this problem all this time...

[MaxDesiatov]

@vi4m the issue is fixed in Tokamak 0.5.2 I tagged just now. I'll keep the issue open until we have a test case in our test suite that covers this, but I hope with the new release the issue doesn't block you in any way.

[vi4m]

Fantastic news, thanks for fixing it! This little bug was a deal breaker for my project, and now I can continue hacking :)

[shial4]

Hi there! I might have similar issue. I have left vertical stack with items you can select and a detailed view to the right. However some details view are optional which means they are not sowing.

So when going from detail view to detail view order is messed up. As presented below in the image. My code to the project

[Feuermurmel]

I came across a very simple case where reordering of children also happens:

import TokamakDOM

struct MyApp: App {
    @State var showThing = true
    
    var body: some Scene {
        WindowGroup("Test") {
            if showThing { Text("[thing]") }
            Toggle("show thing", isOn: $showThing)
        }
    }
}

MyApp.main()

When running the application, it starts out like this:

Unchecking and checking the checkbox again leads to this state:

[ffried]

Just came across the same issue. Very simple reproduction case:

import Foundation
import TokamakDOM

struct ContentView: View {
    @State
    private var uuids = Array<UUID>()
    
    var body: some View {
        VStack {
            ForEach(uuids, id: \.self) {
                Text($0.uuidString)
            }
            Button("Add UUID") { uuids.append(UUID()) }
        }
    }
}

The UUIDs are supposed to appear above the button, but they'll always appear below it.

[carson-katri]

@ffried Are you using the fiber reconciler? If not, can you replicate the issue with that?

See this section in the README for more details: https://github.com/TokamakUI/Tokamak/#fiber-renderers

[ffried]

@carson-katri No, the fiber reconciler (both with and without dynamic layout) does not have this particular issue. Can't use that one unfortunately, because it has a bunch of other (layout) issues.