Fix forward tabbing through items (#1149)

* Fix forward tabbing through items

The implmentation never properly stepped out of repeaters, so it went
into a loop when a repeater was below a node without siblings. This in
turn led to the window aborting the focus item search. So the focus
never moved forward.

Add a test to make sure this stays fixed.

* Update internal/core/items.rs

Co-authored-by: Simon Hausmann <hausmann@gmail.com>

Co-authored-by: Simon Hausmann <hausmann@gmail.com>

internal/core/item_focus.rs

@@ -9,14 +9,10 @@ This module contains the code moving the keyboard focus between items
 
 use crate::item_tree::ComponentItemTree;
 
-pub fn default_next_in_local_focus_chain(
+pub fn step_out_of_node(
     index: usize,
     item_tree: &crate::item_tree::ComponentItemTree,
 ) -> Option<usize> {
-    if let Some(child) = item_tree.first_child(index) {
-        return Some(child);
-    }
-
     let mut self_or_ancestor = index;
     loop {
         if let Some(sibling) = item_tree.next_sibling(self_or_ancestor) {
@@ -30,7 +26,18 @@ pub fn default_next_in_local_focus_chain(
     }
 }
 
-fn rightmost_node(item_tree: &ComponentItemTree, index: usize) -> usize {
+pub fn default_next_in_local_focus_chain(
+    index: usize,
+    item_tree: &crate::item_tree::ComponentItemTree,
+) -> Option<usize> {
+    if let Some(child) = item_tree.first_child(index) {
+        return Some(child);
+    }
+
+    step_out_of_node(index, item_tree)
+}
+
+fn step_into_node(item_tree: &ComponentItemTree, index: usize) -> usize {
     let mut node = index;
     loop {
         if let Some(last_child) = item_tree.last_child(node) {
@@ -46,7 +53,7 @@ pub fn default_previous_in_local_focus_chain(
     item_tree: &crate::item_tree::ComponentItemTree,
 ) -> Option<usize> {
     if let Some(previous) = item_tree.previous_sibling(index) {
-        Some(rightmost_node(item_tree, previous))
+        Some(step_into_node(item_tree, previous))
     } else {
         item_tree.parent(index)
     }
@@ -75,7 +82,7 @@ mod tests {
         };
         let reverse_backward_chain = {
             let mut tmp = alloc::vec::Vec::with_capacity(item_tree.node_count());
-            let mut node = rightmost_node(&item_tree, 0);
+            let mut node = step_into_node(&item_tree, 0);
 
             loop {
                 tmp.push(node);

internal/core/items.rs

@@ -436,7 +436,8 @@ impl ItemRc {
         focus_step: &dyn Fn(&crate::item_tree::ComponentItemTree, usize) -> Option<usize>,
         subtree_step: &dyn Fn(ItemRc) -> Option<ItemRc>,
         subtree_child: &dyn Fn(usize, usize) -> usize,
-        wrap_around: &dyn Fn(ItemRc) -> ItemRc,
+        step_in: &dyn Fn(ItemRc) -> ItemRc,
+        step_out: &dyn Fn(&crate::item_tree::ComponentItemTree, usize) -> Option<usize>,
     ) -> Self {
         let comp_ref_pin = vtable::VRc::borrow_pin(&self.component);
         let item_tree = crate::item_tree::ComponentItemTree::new(&comp_ref_pin);
@@ -450,7 +451,7 @@ impl ItemRc {
                     next,
                     &item_tree,
                     subtree_child,
-                    wrap_around,
+                    step_in,
                 ) {
                     return item;
                 }
@@ -460,13 +461,31 @@ impl ItemRc {
                 // Step out of this component:
                 let root = ItemRc::new(self.component(), 0);
                 if let Some(item) = subtree_step(root.clone()) {
-                    return wrap_around(item);
+                    return step_in(item);
                 } else {
                     // Go up a level!
                     if let Some(parent) = root.parent_item().upgrade() {
-                        return parent;
+                        let parent_ref_pin = vtable::VRc::borrow_pin(&parent.component);
+                        let parent_item_tree =
+                            crate::item_tree::ComponentItemTree::new(&parent_ref_pin);
+                        if let Some(next) = step_out(&parent_item_tree, parent.index()) {
+                            if let Some(item) = step_into_node(
+                                &parent.component(),
+                                &parent_ref_pin,
+                                next,
+                                &parent_item_tree,
+                                subtree_child,
+                                step_in,
+                            ) {
+                                return item;
+                            }
+                            to_focus = next;
+                        } else {
+                            // Moving out
+                            return step_in(root);
+                        }
                     } else {
-                        return wrap_around(root);
+                        return step_in(root);
                     }
                 }
             }
@@ -491,6 +510,7 @@ impl ItemRc {
                     }
                 }
             },
+            &|_, index| Some(index),
         )
     }
 
@@ -502,7 +522,8 @@ impl ItemRc {
             },
             &|root| root.next_sibling(),
             &|start, _| start,
-            &|root| root,
+            &core::convert::identity,
+            &|item_tree, index| crate::item_focus::step_out_of_node(index, item_tree),
         )
     }
 }
@@ -2228,4 +2249,108 @@ mod tests {
         cursor = cursor.previous_focus_item();
         assert!(cursor == item);
     }
+
+    #[test]
+    fn test_tree_traversal_item_subtree_child() {
+        let component = VRc::new(TestComponent {
+            parent_component: None,
+            item_tree: vec![
+                ItemTreeNode::Item {
+                    children_count: 2,
+                    children_index: 1,
+                    parent_index: 0,
+                    item_array_index: 0,
+                },
+                ItemTreeNode::Item {
+                    children_count: 1,
+                    children_index: 3,
+                    parent_index: 0,
+                    item_array_index: 0,
+                },
+                ItemTreeNode::Item {
+                    children_count: 0,
+                    children_index: 4,
+                    parent_index: 0,
+                    item_array_index: 0,
+                },
+                ItemTreeNode::Item {
+                    children_count: 1,
+                    children_index: 4,
+                    parent_index: 1,
+                    item_array_index: 0,
+                },
+                ItemTreeNode::DynamicTree { index: 0, parent_index: 3 },
+            ],
+            subtrees: std::cell::RefCell::new(vec![]),
+            subtree_index: core::usize::MAX,
+        });
+
+        component.as_pin_ref().subtrees.replace(vec![vec![VRc::new(TestComponent {
+            parent_component: Some(VRc::into_dyn(component.clone())),
+            item_tree: vec![ItemTreeNode::Item {
+                children_count: 0,
+                children_index: 1,
+                parent_index: 4,
+                item_array_index: 0,
+            }],
+            subtrees: std::cell::RefCell::new(vec![]),
+            subtree_index: 0,
+        })]]);
+
+        let component = VRc::into_dyn(component);
+
+        // Examine root node:
+        let item = ItemRc::new(component.clone(), 0);
+        assert!(item.previous_sibling().is_none());
+        assert!(item.next_sibling().is_none());
+
+        let c1 = item.first_child().unwrap();
+        assert_eq!(c1.index(), 1);
+        assert!(VRc::ptr_eq(&c1.component(), &item.component()));
+
+        let c2 = c1.next_sibling().unwrap();
+        assert_eq!(c2.index(), 2);
+        assert!(VRc::ptr_eq(&c2.component(), &item.component()));
+
+        let c1_1 = c1.first_child().unwrap();
+        assert_eq!(c1_1.index(), 3);
+        assert!(VRc::ptr_eq(&c1_1.component(), &item.component()));
+
+        let sub = c1_1.first_child().unwrap();
+        assert_eq!(sub.index(), 0);
+        assert!(!VRc::ptr_eq(&sub.component(), &item.component()));
+
+        // Focus traversal:
+        let mut cursor = item.clone();
+
+        cursor = cursor.next_focus_item();
+        assert!(cursor == c1);
+
+        cursor = cursor.next_focus_item();
+        assert!(cursor == c1_1);
+
+        cursor = cursor.next_focus_item();
+        assert!(cursor == sub);
+
+        cursor = cursor.next_focus_item();
+        assert!(cursor == c2);
+
+        cursor = cursor.next_focus_item();
+        assert!(cursor == item);
+
+        cursor = cursor.previous_focus_item();
+        assert!(cursor == c2);
+
+        cursor = cursor.previous_focus_item();
+        assert!(cursor == sub);
+
+        cursor = cursor.previous_focus_item();
+        assert!(cursor == c1_1);
+
+        cursor = cursor.previous_focus_item();
+        assert!(cursor == c1);
+
+        cursor = cursor.previous_focus_item();
+        assert!(cursor == item);
+    }
 }