fix: Handle reordedering of keyed children (#1015)

* fix: Handle reoderdering of keyed children

* fmt

* comment out some compositor tests

* feat: Add invalidation reasons in torin

* adapt tests

* test reordering of nodes in torin

* test reordering of nodes in freya-core

* update

crates/core/src/dom/mutations_writer.rs

@@ -21,7 +21,10 @@ use freya_node_state::{
     CustomAttributeValues,
     LayerState,
 };
-use torin::torin::Torin;
+use torin::torin::{
+    DirtyReason,
+    Torin,
+};
 
 use crate::prelude::{
     Compositor,
@@ -160,12 +163,34 @@ impl<'a> WriteMutations for MutationsWriter<'a> {
 
     fn insert_nodes_after(&mut self, id: dioxus_core::ElementId, m: usize) {
         if m > 0 {
+            self.layout.invalidate_with_reason(
+                self.native_writer.state.element_to_node_id(id),
+                DirtyReason::Reorder,
+            );
+            let new_nodes =
+                &self.native_writer.state.stack[self.native_writer.state.stack.len() - m..];
+            for new in new_nodes {
+                self.layout
+                    .invalidate_with_reason(*new, DirtyReason::Reorder);
+            }
+
             self.native_writer.insert_nodes_after(id, m);
         }
     }
 
     fn insert_nodes_before(&mut self, id: dioxus_core::ElementId, m: usize) {
         if m > 0 {
+            self.layout.invalidate_with_reason(
+                self.native_writer.state.element_to_node_id(id),
+                DirtyReason::Reorder,
+            );
+            let new_nodes =
+                &self.native_writer.state.stack[self.native_writer.state.stack.len() - m..];
+            for new in new_nodes {
+                self.layout
+                    .invalidate_with_reason(*new, DirtyReason::Reorder);
+            }
+
             self.native_writer.insert_nodes_before(id, m);
         }
     }

crates/core/src/layout.rs

@@ -33,7 +33,7 @@ pub fn process_layout(
         let mut dirty_accessibility_tree = fdom.accessibility_dirty_nodes();
         let mut compositor_dirty_nodes = fdom.compositor_dirty_nodes();
         let mut compositor_dirty_area = fdom.compositor_dirty_area();
-        let mut buffer = layout.dirty.iter().copied().collect_vec();
+        let mut buffer = layout.dirty.keys().copied().collect_vec();
         while let Some(node_id) = buffer.pop() {
             if let Some(node) = rdom.get(node_id) {
                 if let Some(area) =

crates/core/tests/nodes_reorder.rs

@@ -0,0 +1,47 @@
+use freya::prelude::*;
+use freya_testing::prelude::*;
+
+#[tokio::test]
+pub async fn nodes_reorder() {
+    fn nodes_reorder() -> Element {
+        let mut data = use_signal(|| vec![1, 2, 3]);
+
+        rsx!(
+            rect {
+                onclick: move |_| {
+                    let item = data.write().remove(0);
+                    data.write().push(item);
+                },
+                label {
+                    "Move"
+                }
+            }
+            for d in data.read().iter() {
+                label {
+                    key: "{d}",
+                    height: "20",
+                    "{d}"
+                }
+            }
+        )
+    }
+
+    let mut utils = launch_test(nodes_reorder);
+    utils.wait_for_update().await;
+
+    assert_eq!(utils.root().get(1).get(0).text(), Some("1"));
+    assert_eq!(utils.root().get(2).get(0).text(), Some("2"));
+    assert_eq!(utils.root().get(3).get(0).text(), Some("3"));
+
+    utils.click_cursor((5., 5.)).await;
+
+    assert_eq!(utils.root().get(1).get(0).text(), Some("2"));
+    assert_eq!(utils.root().get(2).get(0).text(), Some("3"));
+    assert_eq!(utils.root().get(3).get(0).text(), Some("1"));
+
+    utils.click_cursor((5., 5.)).await;
+
+    assert_eq!(utils.root().get(1).get(0).text(), Some("3"));
+    assert_eq!(utils.root().get(2).get(0).text(), Some("1"));
+    assert_eq!(utils.root().get(3).get(0).text(), Some("2"));
+}

crates/native-core/src/dioxus.rs

@@ -34,7 +34,7 @@ struct ElementIdComponent(ElementId);
 /// The state of the Dioxus integration with the RealDom
 pub struct DioxusState {
     templates: FxHashMap<String, Vec<NodeId>>,
-    stack: Vec<NodeId>,
+    pub stack: Vec<NodeId>,
     node_id_mapping: Vec<Option<NodeId>>,
 }
 

crates/native-core/src/tree.rs

@@ -229,13 +229,24 @@ impl<'a> TreeMut for TreeMutView<'a> {
     }
 
     fn insert_before(&mut self, old_id: NodeId, new_id: NodeId) {
+        let new_parent_id = {
+            let new_id = self.1.get(new_id).unwrap();
+            new_id.parent
+        };
+        if let Some(new_parent_id) = new_parent_id {
+            (&mut self.1)
+                .get(new_parent_id)
+                .unwrap()
+                .children
+                .retain(|id| *id != new_id);
+        }
+
         let parent_id = {
             let old_node = self.1.get(old_id).unwrap();
             old_node.parent.expect("tried to insert before root")
         };
-        {
-            (&mut self.1).get(new_id).unwrap().parent = Some(parent_id);
-        }
+        (&mut self.1).get(new_id).unwrap().parent = Some(parent_id);
+
         let parent = (&mut self.1).get(parent_id).unwrap();
         let index = parent
             .children
@@ -248,9 +259,21 @@ impl<'a> TreeMut for TreeMutView<'a> {
     }
 
     fn insert_after(&mut self, old_id: NodeId, new_id: NodeId) {
+        let new_parent_id = {
+            let new_id = self.1.get(new_id).unwrap();
+            new_id.parent
+        };
+        if let Some(new_parent_id) = new_parent_id {
+            (&mut self.1)
+                .get(new_parent_id)
+                .unwrap()
+                .children
+                .retain(|id| *id != new_id);
+        }
+
         let mut node_state = &mut self.1;
         let old_node = node_state.get(old_id).unwrap();
-        let parent_id = old_node.parent.expect("tried to insert before root");
+        let parent_id = old_node.parent.expect("tried to insert after root");
         (&mut node_state).get(new_id).unwrap().parent = Some(parent_id);
         let parent = (&mut node_state).get(parent_id).unwrap();
         let index = parent

crates/torin/src/measure.rs

@@ -75,7 +75,7 @@ where
         // 2. If this Node has been marked as dirty
         // 3. If there is no know cached data about this Node.
         let must_revalidate = parent_is_dirty
-            || self.layout.dirty.contains(&node_id)
+            || self.layout.dirty.contains_key(&node_id)
             || !self.layout.results.contains_key(&node_id);
         if must_revalidate {
             // Create the initial Node area size

crates/torin/src/torin.rs

@@ -4,10 +4,7 @@ use std::{
 };
 
 pub use euclid::Rect;
-use rustc_hash::{
-    FxHashMap,
-    FxHashSet,
-};
+use rustc_hash::FxHashMap;
 
 use crate::{
     custom_measurer::LayoutMeasurer,
@@ -67,12 +64,19 @@ impl<Key: NodeKey> RootNodeCandidate<Key> {
     }
 }
 
+#[derive(Clone, Debug, PartialEq)]
+pub enum DirtyReason {
+    None,
+    /// Node was moved from one position to another in its parent' children list.
+    Reorder,
+}
+
 pub struct Torin<Key: NodeKey> {
     /// Layout results of the registered Nodes
     pub results: FxHashMap<Key, LayoutNode>,
 
     /// Invalid registered nodes since previous layout measurement
-    pub dirty: FxHashSet<Key>,
+    pub dirty: FxHashMap<Key, DirtyReason>,
 
     /// Best Root node candidate from where to start measuring
     pub root_node_candidate: RootNodeCandidate<Key>,
@@ -89,7 +93,7 @@ impl<Key: NodeKey> Torin<Key> {
     pub fn new() -> Self {
         Self {
             results: HashMap::default(),
-            dirty: FxHashSet::default(),
+            dirty: FxHashMap::default(),
             root_node_candidate: RootNodeCandidate::None,
         }
     }
@@ -106,7 +110,7 @@ impl<Key: NodeKey> Torin<Key> {
     }
 
     /// Read the HashSet of dirty nodes
-    pub fn get_dirty_nodes(&self) -> &FxHashSet<Key> {
+    pub fn get_dirty_nodes(&self) -> &FxHashMap<Key, DirtyReason> {
         &self.dirty
     }
 
@@ -142,26 +146,32 @@ impl<Key: NodeKey> Torin<Key> {
         }
     }
 
-    /// Safely mark as dirty a Node
+    /// Safely mark as dirty a Node, with no reason.
     pub fn safe_invalidate(&mut self, node_id: Key, dom_adapter: &mut impl DOMAdapter<Key>) {
         if dom_adapter.is_node_valid(&node_id) {
-            self.dirty.insert(node_id);
+            self.dirty.insert(node_id, DirtyReason::None);
         }
     }
 
-    /// Mark as dirty a Node
+    /// Mark as dirty a Node, with no reason.
     pub fn invalidate(&mut self, node_id: Key) {
-        self.dirty.insert(node_id);
+        self.dirty.insert(node_id, DirtyReason::None);
+    }
+
+    /// Mark as dirty a Node, with a reason.
+    pub fn invalidate_with_reason(&mut self, node_id: Key, reason: DirtyReason) {
+        self.dirty.insert(node_id, reason);
     }
 
     // Mark as dirty the given Node and all the nodes that depend on it
     pub fn check_dirty_dependants(
         &mut self,
         node_id: Key,
+        reason: DirtyReason,
         dom_adapter: &mut impl DOMAdapter<Key>,
         ignore: bool,
     ) {
-        if (self.dirty.contains(&node_id) && ignore) || !dom_adapter.is_node_valid(&node_id) {
+        if (self.dirty.contains_key(&node_id) && ignore) || !dom_adapter.is_node_valid(&node_id) {
             return;
         }
 
@@ -180,12 +190,16 @@ impl<Key: NodeKey> Torin<Key> {
             if let Some(parent) = parent {
                 if parent.does_depend_on_inner() {
                     // Mark parent if it depends on it's inner children
-                    self.check_dirty_dependants(parent_id, dom_adapter, true);
+                    self.check_dirty_dependants(parent_id, DirtyReason::None, dom_adapter, true);
                 } else {
                     let parent_children = dom_adapter.children_of(&parent_id);
                     let multiple_children = parent_children.len() > 1;
 
-                    let mut found_node = false;
+                    let mut found_node = match reason {
+                        DirtyReason::None => false,
+                        // Invalidate all siblings if the node was reordered
+                        DirtyReason::Reorder => true,
+                    };
                     for child_id in parent_children {
                         if found_node {
                             self.safe_invalidate(child_id, dom_adapter);
@@ -215,8 +229,8 @@ impl<Key: NodeKey> Torin<Key> {
         if self.results.is_empty() {
             return;
         }
-        for dirty in self.dirty.clone() {
-            self.check_dirty_dependants(dirty, dom_adapter, false);
+        for (id, reason) in self.dirty.clone() {
+            self.check_dirty_dependants(id, reason, dom_adapter, false);
         }
     }