From 9f429987e72cd895cc27e64338747facf6ce95ee Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Wed, 6 Aug 2025 14:03:33 -0500 Subject: [PATCH 1/5] wip --- crates/gpui/src/tab_stop.rs | 289 ++++++++++++++++++++++++++++++++++++ 1 file changed, 289 insertions(+) diff --git a/crates/gpui/src/tab_stop.rs b/crates/gpui/src/tab_stop.rs index 7dde42efed..32d5b82115 100644 --- a/crates/gpui/src/tab_stop.rs +++ b/crates/gpui/src/tab_stop.rs @@ -74,6 +74,295 @@ mod tests { use crate::{FocusHandle, FocusMap, TabHandles}; use std::sync::Arc; + /// Helper function to parse XML-like structure and test tab navigation + /// + /// The XML structure should define elements with tab-index and actual (expected order) values. + /// Elements like tab-group and focus-trap are parsed as regular elements with their own tab-index. + /// All elements are treated as flat - there is no nesting concept in TabHandles. + /// + /// Example: + /// ``` + /// + /// + /// + /// + /// + /// + /// ``` + fn check(xml: &str) { + let focus_map = Arc::new(FocusMap::default()); + let mut tab_handles = TabHandles::default(); + + // Parse the XML-like structure + let elements = parse_xml_structure(xml); + + // Create focus handles based on parsed elements + let mut all_handles = Vec::new(); + let mut actual_to_handle = std::collections::HashMap::::new(); + + for element in elements { + let mut handle = FocusHandle::new(&focus_map); + + // Set tab_index if specified + if let Some(tab_index) = element.tab_index { + handle = handle.tab_index(tab_index); + } + + // Enable tab_stop by default unless it's explicitly disabled + handle = handle.tab_stop(element.tab_stop.unwrap_or(true)); + + // Store the handle + all_handles.push(handle.clone()); + tab_handles.insert(&handle); + + // Track handles by their actual position + if let Some(actual) = element.actual { + if actual_to_handle.insert(actual, handle).is_some() { + panic!("Duplicate actual value: {}", actual); + } + } + } + + // Get the actual tab order from TabHandles + let mut tab_order: Vec = Vec::new(); + let mut current = None; + + // Build the actual navigation order + for _ in 0..tab_handles.handles.len() { + if let Some(next_handle) = + tab_handles.next(current.as_ref().map(|h: &FocusHandle| &h.id)) + { + // Check if we've cycled back to the beginning + if !tab_order.is_empty() && tab_order[0].id == next_handle.id { + break; + } + current = Some(next_handle.clone()); + tab_order.push(next_handle); + } else { + break; + } + } + + // Check that we have the expected number of tab stops + assert_eq!( + tab_order.len(), + actual_to_handle.len(), + "Number of tab stops ({}) doesn't match expected ({})", + tab_order.len(), + actual_to_handle.len() + ); + + // Check each position matches the expected handle + for (position, handle) in tab_order.iter().enumerate() { + let expected_handle = actual_to_handle.get(&position).unwrap_or_else(|| { + panic!( + "No element specified with actual={}, but tab order has {} elements", + position, + tab_order.len() + ) + }); + + assert_eq!( + handle.id, expected_handle.id, + "Tab order at position {} doesn't match expected. Got {:?}, expected {:?}", + position, handle.id, expected_handle.id + ); + } + + // Test that navigation wraps correctly + if !tab_order.is_empty() { + // Test next wraps from last to first + let last_id = tab_order.last().unwrap().id; + let first_id = tab_order.first().unwrap().id; + assert_eq!( + tab_handles.next(Some(&last_id)).map(|h| h.id), + Some(first_id), + "next should wrap from last to first" + ); + + // Test prev wraps from first to last + assert_eq!( + tab_handles.prev(Some(&first_id)).map(|h| h.id), + Some(last_id), + "prev should wrap from first to last" + ); + } + + #[derive(Debug)] + struct ParsedElement { + element_type: String, + tab_index: Option, + actual: Option, + tab_stop: Option, + } + + fn parse_xml_structure(xml: &str) -> Vec { + let mut elements = Vec::new(); + + for line in xml.lines() { + let line = line.trim(); + if line.is_empty() { + continue; + } + + // Parse opening tags like or + if line.starts_with('<') && !line.starts_with(" brackets + let content = line.trim_start_matches('<').trim_end_matches('>'); + let parts: Vec<&str> = content.split_whitespace().collect(); + + if !parts.is_empty() { + // First part might be element type or tab-index + let first_part = parts[0]; + if first_part.starts_with("tab-index=") { + element.element_type = "element".to_string(); + } else if let Some(idx) = first_part.find(' ') { + element.element_type = first_part[..idx].to_string(); + } else if !first_part.contains('=') { + element.element_type = first_part.to_string(); + } else { + element.element_type = "element".to_string(); + } + } + + // Parse attributes + for part in parts { + if let Some(eq_pos) = part.find('=') { + let key = &part[..eq_pos]; + let value = &part[eq_pos + 1..]; + + match key { + "tab-index" => { + element.tab_index = value.parse::().ok(); + } + "actual" => { + element.actual = value.parse::().ok(); + } + "tab-stop" => { + element.tab_stop = value.parse::().ok(); + } + _ => {} + } + } + } + + // Special handling for focus-trap and tab-group + if element.element_type == "focus-trap" { + // Focus traps might have special behavior + // For now, treat them as regular elements + } + + elements.push(element); + } + } + + elements + } + } + + #[test] + fn test_check_helper() { + // Test simple ordering + let xml = r#" + + + + "#; + check(xml); + + // Test with duplicate tab indices (should maintain insertion order within same index) + let xml2 = r#" + + + + + + "#; + check(xml2); + + // Test with negative and positive indices + let xml3 = r#" + + + + + "#; + check(xml3); + } + + #[test] + fn test_with_nested_structures() { + // Note: tab-group and focus-trap are parsed as regular elements + // since TabHandles treats all elements as flat (no nesting concept) + // Elements with same tab_index are kept in insertion order + + // Test with elements that look like tab groups (but are just regular elements) + // Order: tab_index=0 (first two), tab_index=1 (next two), tab_index=2, tab_index=3 + let xml = r#" + + + + + + + + "#; + check(xml); + + // Test with elements that look like focus traps (but are just regular elements) + // Order: tab_index=0 (first two), tab_index=1 (next two), tab_index=2 + let xml2 = r#" + + + + + + "#; + check(xml2); + + // Test mixed element types (all treated as flat) + // Order: tab_index=0 (all three), tab_index=1 (next two), tab_index=2 (last two) + let xml3 = r#" + + + + + + + + "#; + check(xml3); + } + + #[test] + fn test_with_disabled_tab_stops() { + // Test with mixed tab-stop values + let xml = r#" + + + + + "#; + check(xml); + + // Test with all disabled except specific ones + let xml2 = r#" + + + + + + "#; + check(xml2); + } + #[test] fn test_tab_handles() { let focus_map = Arc::new(FocusMap::default()); From 6d60d0df2240265ff052131e74dac22bb7320e92 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Thu, 7 Aug 2025 09:57:29 -0500 Subject: [PATCH 2/5] make group tests hopefully work --- crates/gpui/src/tab_stop.rs | 150 ++++++++++++++++++++++++++---------- 1 file changed, 108 insertions(+), 42 deletions(-) diff --git a/crates/gpui/src/tab_stop.rs b/crates/gpui/src/tab_stop.rs index 32d5b82115..1a07a0e97a 100644 --- a/crates/gpui/src/tab_stop.rs +++ b/crates/gpui/src/tab_stop.rs @@ -77,17 +77,20 @@ mod tests { /// Helper function to parse XML-like structure and test tab navigation /// /// The XML structure should define elements with tab-index and actual (expected order) values. - /// Elements like tab-group and focus-trap are parsed as regular elements with their own tab-index. - /// All elements are treated as flat - there is no nesting concept in TabHandles. /// - /// Example: + /// Currently only supports flat elements: /// ``` /// /// - /// - /// - /// - /// + /// + /// ``` + /// + /// Future support (not yet implemented) for nested structures: + /// ``` + /// + /// // Would be at position 2.0 + /// // Would be at position 2.1 + /// /// ``` fn check(xml: &str) { let focus_map = Arc::new(FocusMap::default()); @@ -109,16 +112,25 @@ mod tests { } // Enable tab_stop by default unless it's explicitly disabled - handle = handle.tab_stop(element.tab_stop.unwrap_or(true)); + // Container elements (tab-group, focus-trap) should not be tab stops themselves + let should_be_tab_stop = if element.is_container { + false + } else { + element.tab_stop.unwrap_or(true) + }; + handle = handle.tab_stop(should_be_tab_stop); // Store the handle all_handles.push(handle.clone()); tab_handles.insert(&handle); // Track handles by their actual position - if let Some(actual) = element.actual { - if actual_to_handle.insert(actual, handle).is_some() { - panic!("Duplicate actual value: {}", actual); + // Skip container elements as they don't participate in tab order directly + if !element.is_container { + if let Some(actual) = element.actual { + if actual_to_handle.insert(actual, handle).is_some() { + panic!("Duplicate actual value: {}", actual); + } } } } @@ -194,6 +206,7 @@ mod tests { tab_index: Option, actual: Option, tab_stop: Option, + is_container: bool, // For tab-group and focus-trap } fn parse_xml_structure(xml: &str) -> Vec { @@ -212,6 +225,7 @@ mod tests { tab_index: None, actual: None, tab_stop: None, + is_container: false, }; // Remove < and > brackets @@ -253,10 +267,17 @@ mod tests { } } - // Special handling for focus-trap and tab-group - if element.element_type == "focus-trap" { - // Focus traps might have special behavior - // For now, treat them as regular elements + // Mark tab-group and focus-trap as containers + if element.element_type == "tab-group" || element.element_type == "focus-trap" { + element.is_container = true; + // Container elements should not have 'actual' values themselves + // Only their children should have actual values + if element.actual.is_some() { + panic!( + "Container element '{}' should not have an 'actual' attribute", + element.element_type + ); + } } elements.push(element); @@ -298,47 +319,92 @@ mod tests { } #[test] - fn test_with_nested_structures() { - // Note: tab-group and focus-trap are parsed as regular elements - // since TabHandles treats all elements as flat (no nesting concept) - // Elements with same tab_index are kept in insertion order + fn test_check_helper_with_nested_structures() { + // TODO: These tests define the expected structure for tab-group and focus-trap + // but the grouping logic is not yet implemented. For now, we only test + // flat elements that will work with the current implementation. - // Test with elements that look like tab groups (but are just regular elements) - // Order: tab_index=0 (first two), tab_index=1 (next two), tab_index=2, tab_index=3 + // Test flat elements only (grouping not yet implemented) let xml = r#" - - - - - - + + + "#; check(xml); - // Test with elements that look like focus traps (but are just regular elements) - // Order: tab_index=0 (first two), tab_index=1 (next two), tab_index=2 + // Another flat test let xml2 = r#" - - - + + "#; check(xml2); - // Test mixed element types (all treated as flat) - // Order: tab_index=0 (all three), tab_index=1 (next two), tab_index=2 (last two) - let xml3 = r#" + // Future test structure (not yet implemented): + // + // // This would be at global position 2.0 + // // This would be at global position 2.1 + // + // + // + // // Navigation trapped within this group + // + // + } + + #[test] + #[ignore = "Tab-group and focus-trap functionality not yet implemented"] + fn test_tab_group_functionality() { + // This test defines the expected behavior for tab-group + // Tab-group should create a nested tab context where inner elements + // have tab indices relative to the group + let xml = r#" - - - - - - + + + + + + "#; - check(xml3); + check(xml); + } + + #[test] + #[ignore = "Tab-group and focus-trap functionality not yet implemented"] + fn test_focus_trap_functionality() { + // This test defines the expected behavior for focus-trap + // Focus-trap should trap navigation within its boundaries + let xml = r#" + + + + + + + "#; + check(xml); + } + + #[test] + #[ignore = "Tab-group and focus-trap functionality not yet implemented"] + fn test_nested_groups_and_traps() { + // This test defines the expected behavior for nested structures + let xml = r#" + + + + + + + + + + + "#; + check(xml); } #[test] From 396c5147b677439b999592e3539fa731addf528d Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Mon, 11 Aug 2025 10:50:15 -0500 Subject: [PATCH 3/5] clean --- crates/gpui/src/tab_stop.rs | 994 +++++++++++++++++++++++++++++------- 1 file changed, 811 insertions(+), 183 deletions(-) diff --git a/crates/gpui/src/tab_stop.rs b/crates/gpui/src/tab_stop.rs index 1a07a0e97a..daf1d09b6f 100644 --- a/crates/gpui/src/tab_stop.rs +++ b/crates/gpui/src/tab_stop.rs @@ -71,7 +71,7 @@ impl TabHandles { #[cfg(test)] mod tests { - use crate::{FocusHandle, FocusMap, TabHandles}; + use crate::{FocusHandle, FocusId, FocusMap, TabHandles}; use std::sync::Arc; /// Helper function to parse XML-like structure and test tab navigation @@ -93,124 +93,48 @@ mod tests { /// /// ``` fn check(xml: &str) { - let focus_map = Arc::new(FocusMap::default()); - let mut tab_handles = TabHandles::default(); + use std::collections::HashMap; - // Parse the XML-like structure - let elements = parse_xml_structure(xml); + // Tree node structure with common fields + #[derive(Debug, Clone)] + struct TreeNode { + xml_tag: String, + handle: Option, + node_type: NodeType, + } - // Create focus handles based on parsed elements - let mut all_handles = Vec::new(); - let mut actual_to_handle = std::collections::HashMap::::new(); + // Node type variants + #[derive(Debug, Clone)] + enum NodeType { + TabStop { + tab_index: Option, + actual: usize, // Required for tab stops + }, + NonTabStop { + tab_index: Option, + // No actual field - these aren't in the tab order + }, + Group { + tab_index: Option, + children: Vec, + }, + FocusTrap { + children: Vec, + }, + } - for element in elements { - let mut handle = FocusHandle::new(&focus_map); - - // Set tab_index if specified - if let Some(tab_index) = element.tab_index { - handle = handle.tab_index(tab_index); - } - - // Enable tab_stop by default unless it's explicitly disabled - // Container elements (tab-group, focus-trap) should not be tab stops themselves - let should_be_tab_stop = if element.is_container { - false - } else { - element.tab_stop.unwrap_or(true) + // Phase 1: Parse - Build tree structure from XML + fn parse(xml: &str) -> TreeNode { + let mut root = TreeNode { + xml_tag: "root".to_string(), + handle: None, + node_type: NodeType::Group { + tab_index: None, + children: Vec::new(), + }, }; - handle = handle.tab_stop(should_be_tab_stop); - // Store the handle - all_handles.push(handle.clone()); - tab_handles.insert(&handle); - - // Track handles by their actual position - // Skip container elements as they don't participate in tab order directly - if !element.is_container { - if let Some(actual) = element.actual { - if actual_to_handle.insert(actual, handle).is_some() { - panic!("Duplicate actual value: {}", actual); - } - } - } - } - - // Get the actual tab order from TabHandles - let mut tab_order: Vec = Vec::new(); - let mut current = None; - - // Build the actual navigation order - for _ in 0..tab_handles.handles.len() { - if let Some(next_handle) = - tab_handles.next(current.as_ref().map(|h: &FocusHandle| &h.id)) - { - // Check if we've cycled back to the beginning - if !tab_order.is_empty() && tab_order[0].id == next_handle.id { - break; - } - current = Some(next_handle.clone()); - tab_order.push(next_handle); - } else { - break; - } - } - - // Check that we have the expected number of tab stops - assert_eq!( - tab_order.len(), - actual_to_handle.len(), - "Number of tab stops ({}) doesn't match expected ({})", - tab_order.len(), - actual_to_handle.len() - ); - - // Check each position matches the expected handle - for (position, handle) in tab_order.iter().enumerate() { - let expected_handle = actual_to_handle.get(&position).unwrap_or_else(|| { - panic!( - "No element specified with actual={}, but tab order has {} elements", - position, - tab_order.len() - ) - }); - - assert_eq!( - handle.id, expected_handle.id, - "Tab order at position {} doesn't match expected. Got {:?}, expected {:?}", - position, handle.id, expected_handle.id - ); - } - - // Test that navigation wraps correctly - if !tab_order.is_empty() { - // Test next wraps from last to first - let last_id = tab_order.last().unwrap().id; - let first_id = tab_order.first().unwrap().id; - assert_eq!( - tab_handles.next(Some(&last_id)).map(|h| h.id), - Some(first_id), - "next should wrap from last to first" - ); - - // Test prev wraps from first to last - assert_eq!( - tab_handles.prev(Some(&first_id)).map(|h| h.id), - Some(last_id), - "prev should wrap from first to last" - ); - } - - #[derive(Debug)] - struct ParsedElement { - element_type: String, - tab_index: Option, - actual: Option, - tab_stop: Option, - is_container: bool, // For tab-group and focus-trap - } - - fn parse_xml_structure(xml: &str) -> Vec { - let mut elements = Vec::new(); + let mut stack: Vec = vec![root.clone()]; for line in xml.lines() { let line = line.trim(); @@ -218,34 +142,57 @@ mod tests { continue; } - // Parse opening tags like or - if line.starts_with('<') && !line.starts_with("').trim(); - // Remove < and > brackets + if stack.len() > 1 { + let completed = stack.pop().unwrap(); + let parent = stack.last_mut().unwrap(); + + // Verify tag matches + if completed.xml_tag != tag_name && !completed.xml_tag.starts_with(tag_name) + { + panic!( + "Mismatched closing tag: expected {}, got {}", + completed.xml_tag, tag_name + ); + } + + match &mut parent.node_type { + NodeType::Group { children, .. } + | NodeType::FocusTrap { children, .. } => { + children.push(completed); + } + _ => panic!("Tried to add child to non-container node"), + } + } + continue; + } + + // Handle opening tags + if line.starts_with('<') { let content = line.trim_start_matches('<').trim_end_matches('>'); let parts: Vec<&str> = content.split_whitespace().collect(); - if !parts.is_empty() { - // First part might be element type or tab-index - let first_part = parts[0]; - if first_part.starts_with("tab-index=") { - element.element_type = "element".to_string(); - } else if let Some(idx) = first_part.find(' ') { - element.element_type = first_part[..idx].to_string(); - } else if !first_part.contains('=') { - element.element_type = first_part.to_string(); - } else { - element.element_type = "element".to_string(); - } + if parts.is_empty() { + continue; } + let mut tab_index: Option = None; + let mut actual: Option = None; + let mut tab_stop: Option = None; + + // Determine element type + let first_part = parts[0]; + let element_type = if first_part.starts_with("tab-index=") { + "element".to_string() + } else if !first_part.contains('=') { + first_part.to_string() + } else { + "element".to_string() + }; + // Parse attributes for part in parts { if let Some(eq_pos) = part.find('=') { @@ -254,38 +201,696 @@ mod tests { match key { "tab-index" => { - element.tab_index = value.parse::().ok(); + tab_index = value.parse::().ok(); } "actual" => { - element.actual = value.parse::().ok(); + actual = value.parse::().ok(); } "tab-stop" => { - element.tab_stop = value.parse::().ok(); + tab_stop = value.parse::().ok(); } _ => {} } } } - // Mark tab-group and focus-trap as containers - if element.element_type == "tab-group" || element.element_type == "focus-trap" { - element.is_container = true; - // Container elements should not have 'actual' values themselves - // Only their children should have actual values - if element.actual.is_some() { - panic!( - "Container element '{}' should not have an 'actual' attribute", - element.element_type - ); + // Create node based on type + let node_type = match element_type.as_str() { + "tab-group" => { + if actual.is_some() { + panic!("tab-group elements should not have 'actual' attribute"); + } + NodeType::Group { + tab_index, + children: Vec::new(), + } + } + "focus-trap" => { + if actual.is_some() { + panic!("focus-trap elements should not have 'actual' attribute"); + } + NodeType::FocusTrap { + children: Vec::new(), + } + } + _ => { + // Determine if it's a tab stop based on tab-stop attribute + let is_tab_stop = tab_stop.unwrap_or(true); + if is_tab_stop { + // Tab stops must have an actual value + match actual { + Some(actual_value) => NodeType::TabStop { + tab_index, + actual: actual_value, + }, + None => panic!( + "Tab stop with tab-index={} must have an 'actual' attribute", + tab_index.map_or("None".to_string(), |v| v.to_string()) + ), + } + } else { + // Non-tab stops should not have an actual value + if actual.is_some() { + panic!( + "Non-tab stop (tab-stop=false) should not have an 'actual' attribute" + ); + } + NodeType::NonTabStop { tab_index } + } + } + }; + + let node = TreeNode { + xml_tag: element_type.clone(), + handle: None, + node_type, + }; + + // Check if this is a self-closing tag or container + let is_container = matches!(element_type.as_str(), "tab-group" | "focus-trap"); + + if is_container { + stack.push(node); + } else { + // Self-closing element, add directly to parent + let parent = stack.last_mut().unwrap(); + match &mut parent.node_type { + NodeType::Group { children, .. } + | NodeType::FocusTrap { children, .. } => { + children.push(node); + } + _ => panic!("Tried to add child to non-container node"), } } - - elements.push(element); } } - elements + // Return the root's children wrapped in the root + stack.into_iter().next().unwrap() } + + // Phase 2: Construct - Build TabHandles from tree + fn construct( + node: &mut TreeNode, + focus_map: &Arc, + tab_handles: &mut TabHandles, + ) -> HashMap { + let mut actual_to_handle = HashMap::new(); + + fn construct_recursive( + node: &mut TreeNode, + focus_map: &Arc, + tab_handles: &mut TabHandles, + actual_to_handle: &mut HashMap, + ) { + match &mut node.node_type { + NodeType::TabStop { tab_index, actual } => { + let mut handle = FocusHandle::new(focus_map); + + if let Some(idx) = tab_index { + handle = handle.tab_index(*idx); + } + + handle = handle.tab_stop(true); + tab_handles.insert(&handle); + + if actual_to_handle.insert(*actual, handle.clone()).is_some() { + panic!("Duplicate actual value: {}", actual); + } + + node.handle = Some(handle); + } + NodeType::NonTabStop { tab_index } => { + let mut handle = FocusHandle::new(focus_map); + + if let Some(idx) = tab_index { + handle = handle.tab_index(*idx); + } + + handle = handle.tab_stop(false); + tab_handles.insert(&handle); + + node.handle = Some(handle); + } + NodeType::Group { children, .. } => { + // For now, just process children without special group handling + for child in children { + construct_recursive(child, focus_map, tab_handles, actual_to_handle); + } + } + NodeType::FocusTrap { children, .. } => { + // TODO: Implement focus trap behavior + // Focus traps should create a closed navigation loop where: + // 1. Tab navigation within the trap cycles only between trap elements + // 2. The last element in the trap should navigate to the first element in the trap + // 3. The first element in the trap should navigate back to the last element in the trap + // 4. Elements outside the trap should not be reachable from within the trap + // + // This will require modifying TabHandles to support constrained navigation contexts + // or implementing a separate mechanism to override next/prev behavior for trapped elements. + // + // For now, just process children without special trap handling + for child in children { + construct_recursive(child, focus_map, tab_handles, actual_to_handle); + } + } + } + } + + construct_recursive(node, focus_map, tab_handles, &mut actual_to_handle); + actual_to_handle + } + + // Phase 3: Eval - Verify TabHandles matches expected tree traversal + // This tests that focus traps create proper closed loops and that + // navigation respects trap boundaries. + fn eval( + tree: &TreeNode, + tab_handles: &TabHandles, + actual_to_handle: &HashMap, + ) { + // First, collect information about which handles are in focus traps + #[derive(Debug, Clone)] + struct HandleContext { + handle: FocusHandle, + actual: usize, + focus_trap_members: Option>, + } + + fn collect_handle_contexts( + node: &TreeNode, + contexts: &mut Vec, + current_trap: Option>, + ) { + match &node.node_type { + NodeType::TabStop { actual, .. } => { + if let Some(handle) = &node.handle { + contexts.push(HandleContext { + handle: handle.clone(), + actual: *actual, + focus_trap_members: current_trap.clone(), + }); + } + } + NodeType::NonTabStop { .. } => { + // Non-tab stops don't participate in navigation + } + NodeType::Group { children, .. } => { + // Groups are transparent - just recurse with same trap context + for child in children { + collect_handle_contexts(child, contexts, current_trap.clone()); + } + } + NodeType::FocusTrap { children } => { + // Start collecting handles for this focus trap + let mut trap_handles = Vec::new(); + + // First pass: collect all handles in this trap + fn collect_trap_handles(node: &TreeNode, handles: &mut Vec) { + match &node.node_type { + NodeType::TabStop { .. } => { + if let Some(handle) = &node.handle { + handles.push(handle.clone()); + } + } + NodeType::NonTabStop { .. } => { + // Non-tab stops don't participate in tab navigation + } + NodeType::Group { children, .. } => { + for child in children { + collect_trap_handles(child, handles); + } + } + NodeType::FocusTrap { children } => { + // Nested traps create their own context + for child in children { + collect_trap_handles(child, handles); + } + } + } + } + + for child in children { + collect_trap_handles(child, &mut trap_handles); + } + + // Second pass: add contexts with trap information + for child in children { + collect_handle_contexts(child, contexts, Some(trap_handles.clone())); + } + } + } + } + + let mut handle_contexts = Vec::new(); + // Skip the root node + if let NodeType::Group { children, .. } = &tree.node_type { + for child in children { + collect_handle_contexts(child, &mut handle_contexts, None); + } + } + + // Sort by actual position to get expected order + handle_contexts.sort_by_key(|c| c.actual); + + // Helper function to format tree structure as XML for error messages + fn format_tree_structure( + node: &TreeNode, + label: &str, + actual_map: &HashMap, + ) -> String { + let mut result = format!("{}:\n", label); + + fn format_node( + node: &TreeNode, + actual_map: &HashMap, + indent: usize, + ) -> String { + let mut result = String::new(); + let indent_str = " ".repeat(indent); + + match &node.node_type { + NodeType::TabStop { tab_index, actual } => { + let actual_str = format!(" actual={}", actual); + + result.push_str(&format!( + "{}\n", + indent_str, + tab_index.map_or("None".to_string(), |v| v.to_string()), + actual_str + )); + } + NodeType::NonTabStop { tab_index } => { + result.push_str(&format!( + "{}\n", + indent_str, + tab_index.map_or("None".to_string(), |v| v.to_string()) + )); + } + NodeType::Group { + tab_index, + children, + } => { + result.push_str(&format!( + "{}\n", + indent_str, + tab_index.map_or("None".to_string(), |v| v.to_string()) + )); + for child in children { + result.push_str(&format_node(child, actual_map, indent + 1)); + } + result.push_str(&format!("{}\n", indent_str)); + } + NodeType::FocusTrap { children } => { + result.push_str(&format!("{}\n", indent_str)); + for child in children { + result.push_str(&format_node(child, actual_map, indent + 1)); + } + result.push_str(&format!("{}\n", indent_str)); + } + } + + result + } + + // Skip the root node and format its children + if let NodeType::Group { children, .. } = &node.node_type { + for child in children { + result.push_str(&format_node(child, actual_map, 0)); + } + } + + result + } + + // Helper function to format tree with navigation annotations + fn format_tree_with_navigation( + node: &TreeNode, + label: &str, + actual_map: &HashMap, + current_id: FocusId, + went_to_id: Option, + expected_id: FocusId, + direction: &str, + ) -> String { + let mut result = format!("{} ({}):\n", label, direction); + + fn format_node_with_nav( + node: &TreeNode, + actual_map: &HashMap, + indent: usize, + current_id: FocusId, + went_to_id: Option, + expected_id: FocusId, + direction: &str, + ) -> String { + let mut result = String::new(); + let indent_str = " ".repeat(indent); + + match &node.node_type { + NodeType::TabStop { tab_index, actual } => { + let actual_str = format!(" actual={}", actual); + + // Add navigation annotations + let nav_comment = if let Some(handle) = &node.handle { + if handle.id == current_id { + " // <- Started here" + } else if Some(handle.id) == went_to_id { + " // <- Actually went here" + } else if handle.id == expected_id { + " // <- Expected to go here" + } else { + "" + } + } else { + "" + }; + + result.push_str(&format!( + "{}{}\n", + indent_str, + tab_index.map_or("None".to_string(), |v| v.to_string()), + actual_str, + nav_comment + )); + } + NodeType::NonTabStop { tab_index } => { + // Format non-tab stops without actual value + let nav_comment = String::new(); // Non-tab stops don't participate in navigation + + result.push_str(&format!( + "{}{}\n", + indent_str, + tab_index.map_or("None".to_string(), |v| v.to_string()), + nav_comment + )); + } + NodeType::Group { + tab_index, + children, + } => { + result.push_str(&format!( + "{}\n", + indent_str, + tab_index.map_or("None".to_string(), |v| v.to_string()) + )); + for child in children { + result.push_str(&format_node_with_nav( + child, + actual_map, + indent + 1, + current_id, + went_to_id, + expected_id, + direction, + )); + } + result.push_str(&format!("{}\n", indent_str)); + } + NodeType::FocusTrap { children } => { + result.push_str(&format!("{}\n", indent_str)); + for child in children { + result.push_str(&format_node_with_nav( + child, + actual_map, + indent + 1, + current_id, + went_to_id, + expected_id, + direction, + )); + } + result.push_str(&format!("{}\n", indent_str)); + } + } + + result + } + + // Skip the root node and format its children + if let NodeType::Group { children, .. } = &node.node_type { + for child in children { + result.push_str(&format_node_with_nav( + child, + actual_map, + 0, + current_id, + went_to_id, + expected_id, + direction, + )); + } + } + + result + } + + // Check that we have the expected number of tab stops + if handle_contexts.len() != actual_to_handle.len() { + // Build maps for error display + let mut actual_map = HashMap::new(); + let mut current = None; + for i in 0..tab_handles.handles.len() { + if let Some(next_handle) = + tab_handles.next(current.as_ref().map(|h: &FocusHandle| &h.id)) + { + if i > 0 + && current.as_ref().map(|h: &FocusHandle| h.id) == Some(next_handle.id) + { + break; + } + actual_map.insert(next_handle.id, i); + current = Some(next_handle); + } else { + break; + } + } + + let mut expected_map = HashMap::new(); + for (pos, handle) in actual_to_handle.iter() { + expected_map.insert(handle.id, *pos); + } + + panic!( + "Number of tab stops doesn't match! Expected {} but found {}\n\n{}\n{}", + actual_to_handle.len(), + handle_contexts.len(), + format_tree_structure(tree, "Got", &actual_map), + format_tree_structure(tree, "Expected", &expected_map) + ); + } + + // Check if there are any focus traps at all + let has_focus_traps = handle_contexts + .iter() + .any(|c| c.focus_trap_members.is_some()); + + // If there are no focus traps, use the simpler validation + if !has_focus_traps { + // Build the actual navigation order from TabHandles + let mut tab_order: Vec = Vec::new(); + let mut current = None; + + for _ in 0..tab_handles.handles.len() { + if let Some(next_handle) = + tab_handles.next(current.as_ref().map(|h: &FocusHandle| &h.id)) + { + // Check if we've cycled back to the beginning + if !tab_order.is_empty() && tab_order[0].id == next_handle.id { + break; + } + current = Some(next_handle.clone()); + tab_order.push(next_handle); + } else { + break; + } + } + + // Build expected order from actual_to_handle + let mut expected_order = vec![None; actual_to_handle.len()]; + for (pos, handle) in actual_to_handle.iter() { + expected_order[*pos] = Some(handle.clone()); + } + let expected_handles: Vec = + expected_order.into_iter().flatten().collect(); + + // Create maps for error display + let mut actual_map_got = HashMap::new(); + for (idx, handle) in tab_order.iter().enumerate() { + actual_map_got.insert(handle.id, idx); + } + + let mut actual_map_expected = HashMap::new(); + for (idx, handle) in expected_handles.iter().enumerate() { + actual_map_expected.insert(handle.id, idx); + } + + // Check each position matches the expected handle + for (position, handle) in tab_order.iter().enumerate() { + let expected_handle = actual_to_handle.get(&position).unwrap_or_else(|| { + panic!( + "No element specified with actual={}, but tab order has {} elements", + position, + tab_order.len() + ) + }); + + if handle.id != expected_handle.id { + panic!( + "Tab order mismatch at position {}!\n\n{}\n{}", + position, + format_tree_structure(tree, "Got", &actual_map_got), + format_tree_structure(tree, "Expected", &actual_map_expected) + ); + } + } + + // Test that navigation wraps correctly + if !tab_order.is_empty() { + // Test next wraps from last to first + let last_id = tab_order.last().unwrap().id; + let first_id = tab_order.first().unwrap().id; + assert_eq!( + tab_handles.next(Some(&last_id)).map(|h| h.id), + Some(first_id), + "next should wrap from last to first" + ); + + // Test prev wraps from first to last + assert_eq!( + tab_handles.prev(Some(&first_id)).map(|h| h.id), + Some(last_id), + "prev should wrap from first to last" + ); + } + + return; // Early return for non-focus-trap case + } + + // Now test navigation for each handle (focus-trap aware) + for context in &handle_contexts { + let current_id = context.handle.id; + + // Determine expected next and prev based on context + let (expected_next, expected_prev) = + if let Some(trap_members) = &context.focus_trap_members { + // We're in a focus trap - navigation should stay within the trap + let trap_position = trap_members + .iter() + .position(|h| h.id == current_id) + .expect("Handle should be in its own trap"); + + let next_idx = (trap_position + 1) % trap_members.len(); + let prev_idx = if trap_position == 0 { + trap_members.len() - 1 + } else { + trap_position - 1 + }; + + (trap_members[next_idx].id, trap_members[prev_idx].id) + } else { + // Not in a focus trap - normal navigation through all non-trapped elements + let non_trapped: Vec<&HandleContext> = handle_contexts + .iter() + .filter(|c| c.focus_trap_members.is_none()) + .collect(); + + let non_trapped_position = non_trapped + .iter() + .position(|c| c.handle.id == current_id) + .expect("Non-trapped handle should be in non-trapped list"); + + let next_idx = (non_trapped_position + 1) % non_trapped.len(); + let prev_idx = if non_trapped_position == 0 { + non_trapped.len() - 1 + } else { + non_trapped_position - 1 + }; + + ( + non_trapped[next_idx].handle.id, + non_trapped[prev_idx].handle.id, + ) + }; + + // Test next navigation + let actual_next = tab_handles.next(Some(¤t_id)); + match actual_next { + Some(next_handle) if next_handle.id != expected_next => { + // Build maps for error display + let mut expected_map = HashMap::new(); + for ctx in handle_contexts.iter() { + expected_map.insert(ctx.handle.id, ctx.actual); + } + + panic!( + "Navigation error:\n\n{}", + format_tree_with_navigation( + tree, + "Expected", + &expected_map, + current_id, + Some(next_handle.id), + expected_next, + "testing next()" + ) + ); + } + None => { + panic!( + "Navigation error at position {}: next() returned None but expected {:?}", + context.actual, expected_next + ); + } + _ => {} // Correct navigation + } + + // Test prev navigation + let actual_prev = tab_handles.prev(Some(¤t_id)); + match actual_prev { + Some(prev_handle) if prev_handle.id != expected_prev => { + // Build maps for error display + let mut expected_map = HashMap::new(); + for ctx in handle_contexts.iter() { + expected_map.insert(ctx.handle.id, ctx.actual); + } + + panic!( + "Navigation error:\n\n{}", + format_tree_with_navigation( + tree, + "Expected", + &expected_map, + current_id, + Some(prev_handle.id), + expected_prev, + "testing prev()" + ) + ); + } + None => { + panic!( + "Navigation error at position {}: prev() returned None but expected {:?}", + context.actual, expected_prev + ); + } + _ => {} // Correct navigation + } + } + } + + // Main execution + let focus_map = Arc::new(FocusMap::default()); + let mut tab_handles = TabHandles::default(); + + // Phase 1: Parse + let mut tree = parse(xml); + + // Phase 2: Construct + let actual_to_handle = construct(&mut tree, &focus_map, &mut tab_handles); + + // Phase 3: Eval + eval(&tree, &tab_handles, &actual_to_handle); } #[test] @@ -320,42 +925,26 @@ mod tests { #[test] fn test_check_helper_with_nested_structures() { - // TODO: These tests define the expected structure for tab-group and focus-trap - // but the grouping logic is not yet implemented. For now, we only test - // flat elements that will work with the current implementation. - - // Test flat elements only (grouping not yet implemented) + // Test parsing and structure with nested groups and focus traps let xml = r#" - - - + + + + + + + + + "#; + + // This should parse successfully even though navigation won't work correctly yet + // The test verifies that our tree structure correctly represents nested elements check(xml); - - // Another flat test - let xml2 = r#" - - - - - "#; - check(xml2); - - // Future test structure (not yet implemented): - // - // // This would be at global position 2.0 - // // This would be at global position 2.1 - // - // - // - // // Navigation trapped within this group - // - // } #[test] - #[ignore = "Tab-group and focus-trap functionality not yet implemented"] fn test_tab_group_functionality() { // This test defines the expected behavior for tab-group // Tab-group should create a nested tab context where inner elements @@ -373,7 +962,6 @@ mod tests { } #[test] - #[ignore = "Tab-group and focus-trap functionality not yet implemented"] fn test_focus_trap_functionality() { // This test defines the expected behavior for focus-trap // Focus-trap should trap navigation within its boundaries @@ -389,7 +977,6 @@ mod tests { } #[test] - #[ignore = "Tab-group and focus-trap functionality not yet implemented"] fn test_nested_groups_and_traps() { // This test defines the expected behavior for nested structures let xml = r#" @@ -429,6 +1016,47 @@ mod tests { check(xml2); } + #[test] + #[should_panic(expected = "Tab stop with tab-index=1 must have an 'actual' attribute")] + fn test_tab_stop_without_actual_panics() { + // Tab stops must have an actual value + let xml = r#" + + + + "#; + check(xml); + } + + #[test] + #[should_panic( + expected = "Non-tab stop (tab-stop=false) should not have an 'actual' attribute" + )] + fn test_non_tab_stop_with_actual_panics() { + // Non-tab stops should not have an actual value + let xml = r#" + + + + "#; + check(xml); + } + + #[test] + #[should_panic(expected = "Tab order mismatch at position")] + fn test_incorrect_tab_order_shows_xml_format() { + // This test intentionally has wrong expected order to demonstrate error reporting + // The actual tab order will be: tab-index=-1, 0, 1, 2 (positions 0, 1, 2, 3) + // But we're expecting them at wrong positions + let xml = r#" + + + + + "#; + check(xml); + } + #[test] fn test_tab_handles() { let focus_map = Arc::new(FocusMap::default()); From 0c161d68903e60ce9df5c17d29c4128fb65efa3d Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Mon, 11 Aug 2025 13:50:21 -0500 Subject: [PATCH 4/5] improve error messages --- crates/gpui/src/tab_stop.rs | 180 +++++++++++++++--------------------- 1 file changed, 76 insertions(+), 104 deletions(-) diff --git a/crates/gpui/src/tab_stop.rs b/crates/gpui/src/tab_stop.rs index daf1d09b6f..10aafc769a 100644 --- a/crates/gpui/src/tab_stop.rs +++ b/crates/gpui/src/tab_stop.rs @@ -523,26 +523,38 @@ mod tests { result } + // Helper function to check navigation and panic with formatted error if it doesn't match + fn check_navigation( + tree: &TreeNode, + error_label: &str, + current_id: FocusId, + actual_id: Option, + expected_id: FocusId, + ) { + if actual_id != Some(expected_id) { + panic!( + "Tab navigation error!\n\n{}\n\n{}", + error_label, + format_tree_with_navigation(tree, current_id, actual_id, expected_id) + ); + } + } + // Helper function to format tree with navigation annotations fn format_tree_with_navigation( node: &TreeNode, - label: &str, - actual_map: &HashMap, current_id: FocusId, went_to_id: Option, expected_id: FocusId, - direction: &str, ) -> String { - let mut result = format!("{} ({}):\n", label, direction); + let mut result = String::new(); fn format_node_with_nav( node: &TreeNode, - actual_map: &HashMap, indent: usize, current_id: FocusId, went_to_id: Option, expected_id: FocusId, - direction: &str, ) -> String { let mut result = String::new(); let indent_str = " ".repeat(indent); @@ -553,12 +565,16 @@ mod tests { // Add navigation annotations let nav_comment = if let Some(handle) = &node.handle { - if handle.id == current_id { + if handle.id == current_id && current_id != expected_id { " // <- Started here" } else if Some(handle.id) == went_to_id { " // <- Actually went here" } else if handle.id == expected_id { - " // <- Expected to go here" + if went_to_id.is_none() { + " // <- Expected to go here (but got None)" + } else { + " // <- Expected to go here" + } } else { "" } @@ -597,12 +613,10 @@ mod tests { for child in children { result.push_str(&format_node_with_nav( child, - actual_map, indent + 1, current_id, went_to_id, expected_id, - direction, )); } result.push_str(&format!("{}\n", indent_str)); @@ -612,12 +626,10 @@ mod tests { for child in children { result.push_str(&format_node_with_nav( child, - actual_map, indent + 1, current_id, went_to_id, expected_id, - direction, )); } result.push_str(&format!("{}\n", indent_str)); @@ -632,12 +644,10 @@ mod tests { for child in children { result.push_str(&format_node_with_nav( child, - actual_map, 0, current_id, went_to_id, expected_id, - direction, )); } } @@ -706,25 +716,6 @@ mod tests { } } - // Build expected order from actual_to_handle - let mut expected_order = vec![None; actual_to_handle.len()]; - for (pos, handle) in actual_to_handle.iter() { - expected_order[*pos] = Some(handle.clone()); - } - let expected_handles: Vec = - expected_order.into_iter().flatten().collect(); - - // Create maps for error display - let mut actual_map_got = HashMap::new(); - for (idx, handle) in tab_order.iter().enumerate() { - actual_map_got.insert(handle.id, idx); - } - - let mut actual_map_expected = HashMap::new(); - for (idx, handle) in expected_handles.iter().enumerate() { - actual_map_expected.insert(handle.id, idx); - } - // Check each position matches the expected handle for (position, handle) in tab_order.iter().enumerate() { let expected_handle = actual_to_handle.get(&position).unwrap_or_else(|| { @@ -736,11 +727,25 @@ mod tests { }); if handle.id != expected_handle.id { - panic!( - "Tab order mismatch at position {}!\n\n{}\n{}", - position, - format_tree_structure(tree, "Got", &actual_map_got), - format_tree_structure(tree, "Expected", &actual_map_expected) + // Use navigation-style error formatting + // For position 0, we're testing what should come first + // For other positions, we show the previous position as "started" + let (started_id, went_to_id) = if position > 0 { + // Normal case: show where we started (previous position) and where we went + (tab_order[position - 1].id, Some(handle.id)) + } else { + // Position 0: we're testing what should be first + // Don't show "Started here" since we haven't started anywhere yet + // Show the actual first element as "Actually went here" + (expected_handle.id, Some(handle.id)) + }; + + check_navigation( + tree, + &format!("Tab order mismatch at position {}:\n\n", position), + started_id, + went_to_id, + expected_handle.id, ); } } @@ -750,17 +755,26 @@ mod tests { // Test next wraps from last to first let last_id = tab_order.last().unwrap().id; let first_id = tab_order.first().unwrap().id; - assert_eq!( - tab_handles.next(Some(&last_id)).map(|h| h.id), - Some(first_id), - "next should wrap from last to first" + // Test next wraps from last to first + let actual_next_from_last = tab_handles.next(Some(&last_id)); + let actual_next_id = actual_next_from_last.as_ref().map(|h| h.id); + check_navigation( + tree, + "Expected wrap from last to first (testing next() wrap-around):", + last_id, + actual_next_id, + first_id, ); // Test prev wraps from first to last - assert_eq!( - tab_handles.prev(Some(&first_id)).map(|h| h.id), - Some(last_id), - "prev should wrap from first to last" + let actual_prev_from_first = tab_handles.prev(Some(&first_id)); + let actual_prev_id = actual_prev_from_first.as_ref().map(|h| h.id); + check_navigation( + tree, + "Expected wrap from first to last (testing prev() wrap-around):", + first_id, + actual_prev_id, + last_id, ); } @@ -815,67 +829,25 @@ mod tests { // Test next navigation let actual_next = tab_handles.next(Some(¤t_id)); - match actual_next { - Some(next_handle) if next_handle.id != expected_next => { - // Build maps for error display - let mut expected_map = HashMap::new(); - for ctx in handle_contexts.iter() { - expected_map.insert(ctx.handle.id, ctx.actual); - } - - panic!( - "Navigation error:\n\n{}", - format_tree_with_navigation( - tree, - "Expected", - &expected_map, - current_id, - Some(next_handle.id), - expected_next, - "testing next()" - ) - ); - } - None => { - panic!( - "Navigation error at position {}: next() returned None but expected {:?}", - context.actual, expected_next - ); - } - _ => {} // Correct navigation - } + let actual_next_id = actual_next.as_ref().map(|h| h.id); + check_navigation( + tree, + "Expected (testing next()):", + current_id, + actual_next_id, + expected_next, + ); // Test prev navigation let actual_prev = tab_handles.prev(Some(¤t_id)); - match actual_prev { - Some(prev_handle) if prev_handle.id != expected_prev => { - // Build maps for error display - let mut expected_map = HashMap::new(); - for ctx in handle_contexts.iter() { - expected_map.insert(ctx.handle.id, ctx.actual); - } - - panic!( - "Navigation error:\n\n{}", - format_tree_with_navigation( - tree, - "Expected", - &expected_map, - current_id, - Some(prev_handle.id), - expected_prev, - "testing prev()" - ) - ); - } - None => { - panic!( - "Navigation error at position {}: prev() returned None but expected {:?}", - context.actual, expected_prev - ); - } - _ => {} // Correct navigation - } + let actual_prev_id = actual_prev.as_ref().map(|h| h.id); + check_navigation( + tree, + "Expected (testing prev()):", + current_id, + actual_prev_id, + expected_prev, + ); } } From e594541e5efc043b57dd65a3ab93e8af6a2a47f5 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Wed, 13 Aug 2025 10:12:21 -0500 Subject: [PATCH 5/5] basic impl + more comprehensive group tests --- Cargo.lock | 1 + crates/gpui/Cargo.toml | 3 +- crates/gpui/src/tab_stop.rs | 475 ++++++++++++++++++++++-------------- 3 files changed, 292 insertions(+), 187 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1eb5669fa2..53d1cf571a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7432,6 +7432,7 @@ dependencies = [ "parking_lot", "pathfinder_geometry", "postage", + "pretty_assertions", "profiling", "rand 0.8.5", "raw-window-handle", diff --git a/crates/gpui/Cargo.toml b/crates/gpui/Cargo.toml index 2bf49fa7d8..f45d2b8dcf 100644 --- a/crates/gpui/Cargo.toml +++ b/crates/gpui/Cargo.toml @@ -229,9 +229,10 @@ collections = { workspace = true, features = ["test-support"] } env_logger.workspace = true http_client = { workspace = true, features = ["test-support"] } lyon = { version = "1.0", features = ["extra"] } +pretty_assertions.workspace = true rand.workspace = true -unicode-segmentation.workspace = true reqwest_client = { workspace = true, features = ["test-support"] } +unicode-segmentation.workspace = true util = { workspace = true, features = ["test-support"] } [target.'cfg(target_os = "windows")'.build-dependencies] diff --git a/crates/gpui/src/tab_stop.rs b/crates/gpui/src/tab_stop.rs index 10aafc769a..4e6e8b381a 100644 --- a/crates/gpui/src/tab_stop.rs +++ b/crates/gpui/src/tab_stop.rs @@ -6,6 +6,13 @@ use crate::{FocusHandle, FocusId}; #[derive(Default)] pub(crate) struct TabHandles { pub(crate) handles: Vec, + groups: Vec, + group_depth: usize, +} + +struct GroupDef { + index: isize, + offset: usize, } impl TabHandles { @@ -14,7 +21,10 @@ impl TabHandles { return; } - let focus_handle = focus_handle.clone(); + let mut focus_handle = focus_handle.clone(); + for group in self.groups.iter().rev().take(self.group_depth) { + focus_handle.tab_index += group.index; + } // Insert handle with same tab_index last if let Some(ix) = self @@ -67,6 +77,18 @@ impl TabHandles { None } } + + fn begin_group(&mut self, tab_index: isize) { + self.groups.push(GroupDef { + index: tab_index, + offset: 0, + }); + self.group_depth += 1; + } + + fn end_group(&mut self) { + self.group_depth -= 1; + } } #[cfg(test)] @@ -107,7 +129,7 @@ mod tests { #[derive(Debug, Clone)] enum NodeType { TabStop { - tab_index: Option, + tab_index: isize, actual: usize, // Required for tab stops }, NonTabStop { @@ -115,7 +137,7 @@ mod tests { // No actual field - these aren't in the tab order }, Group { - tab_index: Option, + tab_index: isize, children: Vec, }, FocusTrap { @@ -129,7 +151,7 @@ mod tests { xml_tag: "root".to_string(), handle: None, node_type: NodeType::Group { - tab_index: None, + tab_index: isize::MIN, children: Vec::new(), }, }; @@ -220,6 +242,8 @@ mod tests { if actual.is_some() { panic!("tab-group elements should not have 'actual' attribute"); } + let tab_index = tab_index + .expect("tab-group elements should have 'tab-index' attribute"); NodeType::Group { tab_index, children: Vec::new(), @@ -236,18 +260,16 @@ mod tests { _ => { // Determine if it's a tab stop based on tab-stop attribute let is_tab_stop = tab_stop.unwrap_or(true); + if is_tab_stop { // Tab stops must have an actual value - match actual { - Some(actual_value) => NodeType::TabStop { - tab_index, - actual: actual_value, - }, - None => panic!( - "Tab stop with tab-index={} must have an 'actual' attribute", - tab_index.map_or("None".to_string(), |v| v.to_string()) - ), - } + let tab_index = + tab_index.expect("Tab stop must have a 'tab-index' attribute"); + let actual = actual.expect(&format!( + "Tab stop with tab-index={} must have an 'actual' attribute", + tab_index + )); + NodeType::TabStop { tab_index, actual } } else { // Non-tab stops should not have an actual value if actual.is_some() { @@ -307,8 +329,8 @@ mod tests { NodeType::TabStop { tab_index, actual } => { let mut handle = FocusHandle::new(focus_map); - if let Some(idx) = tab_index { - handle = handle.tab_index(*idx); + if *tab_index != isize::MIN { + handle = handle.tab_index(*tab_index); } handle = handle.tab_stop(true); @@ -332,11 +354,16 @@ mod tests { node.handle = Some(handle); } - NodeType::Group { children, .. } => { + NodeType::Group { + children, + tab_index, + } => { // For now, just process children without special group handling + tab_handles.begin_group(*tab_index); for child in children { construct_recursive(child, focus_map, tab_handles, actual_to_handle); } + tab_handles.end_group(); } NodeType::FocusTrap { children, .. } => { // TODO: Implement focus trap behavior @@ -454,30 +481,25 @@ mod tests { handle_contexts.sort_by_key(|c| c.actual); // Helper function to format tree structure as XML for error messages - fn format_tree_structure( - node: &TreeNode, - label: &str, - actual_map: &HashMap, - ) -> String { - let mut result = format!("{}:\n", label); + fn format_tree_structure(node: &TreeNode, tab_handles: &TabHandles) -> String { + let mut result = String::new(); - fn format_node( - node: &TreeNode, - actual_map: &HashMap, - indent: usize, - ) -> String { + fn format_node(node: &TreeNode, tab_handles: &TabHandles, indent: usize) -> String { let mut result = String::new(); let indent_str = " ".repeat(indent); match &node.node_type { NodeType::TabStop { tab_index, actual } => { + let actual = node + .handle + .as_ref() + .and_then(|handle| tab_handles.current_index(Some(&handle.id))) + .unwrap_or(*actual); let actual_str = format!(" actual={}", actual); result.push_str(&format!( "{}\n", - indent_str, - tab_index.map_or("None".to_string(), |v| v.to_string()), - actual_str + indent_str, tab_index, actual_str )); } NodeType::NonTabStop { tab_index } => { @@ -493,18 +515,17 @@ mod tests { } => { result.push_str(&format!( "{}\n", - indent_str, - tab_index.map_or("None".to_string(), |v| v.to_string()) + indent_str, tab_index )); for child in children { - result.push_str(&format_node(child, actual_map, indent + 1)); + result.push_str(&format_node(child, tab_handles, indent + 1)); } result.push_str(&format!("{}\n", indent_str)); } NodeType::FocusTrap { children } => { result.push_str(&format!("{}\n", indent_str)); for child in children { - result.push_str(&format_node(child, actual_map, indent + 1)); + result.push_str(&format_node(child, tab_handles, indent + 1)); } result.push_str(&format!("{}\n", indent_str)); } @@ -516,7 +537,7 @@ mod tests { // Skip the root node and format its children if let NodeType::Group { children, .. } = &node.node_type { for child in children { - result.push_str(&format_node(child, actual_map, 0)); + result.push_str(&format_node(child, tab_handles, 0)); } } @@ -530,12 +551,17 @@ mod tests { current_id: FocusId, actual_id: Option, expected_id: FocusId, + tab_handles: &TabHandles, ) { if actual_id != Some(expected_id) { panic!( - "Tab navigation error!\n\n{}\n\n{}", + "Tab navigation error!\n\n{}\n\n{}\n\n{}", error_label, - format_tree_with_navigation(tree, current_id, actual_id, expected_id) + format_tree_with_navigation(tree, current_id, actual_id, expected_id), + pretty_assertions::StrComparison::new( + &format_tree_structure(tree, tab_handles), + &format_tree_structure(tree, &TabHandles::default()), + ), ); } } @@ -584,10 +610,7 @@ mod tests { result.push_str(&format!( "{}{}\n", - indent_str, - tab_index.map_or("None".to_string(), |v| v.to_string()), - actual_str, - nav_comment + indent_str, tab_index, actual_str, nav_comment )); } NodeType::NonTabStop { tab_index } => { @@ -607,8 +630,7 @@ mod tests { } => { result.push_str(&format!( "{}\n", - indent_str, - tab_index.map_or("None".to_string(), |v| v.to_string()) + indent_str, tab_index )); for child in children { result.push_str(&format_node_with_nav( @@ -682,11 +704,13 @@ mod tests { } panic!( - "Number of tab stops doesn't match! Expected {} but found {}\n\n{}\n{}", + "Number of tab stops doesn't match! Expected {} but found {}\n\n{}", actual_to_handle.len(), handle_contexts.len(), - format_tree_structure(tree, "Got", &actual_map), - format_tree_structure(tree, "Expected", &expected_map) + pretty_assertions::StrComparison::new( + &format_tree_structure(tree, tab_handles), + &format_tree_structure(tree, &TabHandles::default()), + ), ); } @@ -742,10 +766,11 @@ mod tests { check_navigation( tree, - &format!("Tab order mismatch at position {}:\n\n", position), + &format!("Tab order mismatch at position {}", position), started_id, went_to_id, expected_handle.id, + tab_handles, ); } } @@ -764,6 +789,7 @@ mod tests { last_id, actual_next_id, first_id, + tab_handles, ); // Test prev wraps from first to last @@ -775,6 +801,7 @@ mod tests { first_id, actual_prev_id, last_id, + tab_handles, ); } @@ -836,6 +863,7 @@ mod tests { current_id, actual_next_id, expected_next, + tab_handles, ); // Test prev navigation @@ -847,6 +875,7 @@ mod tests { current_id, actual_prev_id, expected_prev, + tab_handles, ); } } @@ -865,168 +894,242 @@ mod tests { eval(&tree, &tab_handles, &actual_to_handle); } - #[test] - fn test_check_helper() { - // Test simple ordering - let xml = r#" - - - - "#; - check(xml); - - // Test with duplicate tab indices (should maintain insertion order within same index) - let xml2 = r#" - - - - - - "#; - check(xml2); - - // Test with negative and positive indices - let xml3 = r#" - - - - - "#; - check(xml3); + macro_rules! xml_test { + ($test_name:ident, $xml:expr) => { + #[test] + fn $test_name() { + let xml = $xml; + check(xml); + } + }; } - #[test] - fn test_check_helper_with_nested_structures() { - // Test parsing and structure with nested groups and focus traps - let xml = r#" - - - - - - - - - - - "#; + mod test_helper { + use super::*; - // This should parse successfully even though navigation won't work correctly yet - // The test verifies that our tree structure correctly represents nested elements - check(xml); - } + xml_test!( + test_simple_ordering, + r#" + + + + "# + ); - #[test] - fn test_tab_group_functionality() { - // This test defines the expected behavior for tab-group - // Tab-group should create a nested tab context where inner elements - // have tab indices relative to the group - let xml = r#" - - - - - - - - "#; - check(xml); - } - - #[test] - fn test_focus_trap_functionality() { - // This test defines the expected behavior for focus-trap - // Focus-trap should trap navigation within its boundaries - let xml = r#" - - + xml_test!( + test_duplicate_indices_maintain_insertion_order, + r#" + - - - "#; - check(xml); - } - - #[test] - fn test_nested_groups_and_traps() { - // This test defines the expected behavior for nested structures - let xml = r#" - - - - - - - + - - - "#; - check(xml); + "# + ); + + xml_test!( + test_positive_and_negative_indices, + r#" + + + + + "# + ); + + #[test] + #[should_panic( + expected = "Non-tab stop (tab-stop=false) should not have an 'actual' attribute" + )] + fn test_non_tab_stop_with_actual_panics() { + let xml = r#" + + + + "#; + check(xml); + } + + #[test] + #[should_panic(expected = "Tab stop with tab-index=1 must have an 'actual' attribute")] + fn test_tab_stop_without_actual_panics() { + // Tab stops must have an actual value + let xml = r#" + + + + "#; + check(xml); + } + + #[test] + #[should_panic(expected = "Tab order mismatch at position")] + fn test_incorrect_tab_order_shows_xml_format() { + // This test intentionally has wrong expected order to demonstrate error reporting + // The actual tab order will be: tab-index=-1, 0, 1, 2 (positions 0, 1, 2, 3) + // But we're expecting them at wrong positions + let xml = r#" + + + + + "#; + check(xml); + } } - #[test] - fn test_with_disabled_tab_stops() { - // Test with mixed tab-stop values - let xml = r#" + mod basic { + use super::*; + + xml_test!( + test_with_disabled_tab_stop, + r#" - "#; - check(xml); + "# + ); - // Test with all disabled except specific ones - let xml2 = r#" + xml_test!( + test_with_disabled_tab_stops, + r#" - "#; - check(xml2); + "# + ); } - #[test] - #[should_panic(expected = "Tab stop with tab-index=1 must have an 'actual' attribute")] - fn test_tab_stop_without_actual_panics() { - // Tab stops must have an actual value - let xml = r#" - - - - "#; - check(xml); + mod tab_group { + use super::*; + + // This test defines the expected behavior for tab-group + // Tab-group should create a nested tab context where inner elements + // have tab indices relative to the group + xml_test!( + test_tab_group_functionality, + r#" + + + + + + + + + "# + ); + + xml_test!( + test_sibling_groups, + r#" + + + + + + + + + + + + + + + "# + ); + + xml_test!( + test_nested_group, + r#" + + + + + + + + + + + "# + ); + + xml_test!( + test_sibling_nested_groups, + r#" + + + + + + + + + + + + + + + + + "# + ); } - #[test] - #[should_panic( - expected = "Non-tab stop (tab-stop=false) should not have an 'actual' attribute" - )] - fn test_non_tab_stop_with_actual_panics() { - // Non-tab stops should not have an actual value - let xml = r#" - - - - "#; - check(xml); - } + mod focus_trap { + use super::*; - #[test] - #[should_panic(expected = "Tab order mismatch at position")] - fn test_incorrect_tab_order_shows_xml_format() { - // This test intentionally has wrong expected order to demonstrate error reporting - // The actual tab order will be: tab-index=-1, 0, 1, 2 (positions 0, 1, 2, 3) - // But we're expecting them at wrong positions - let xml = r#" - - - - - "#; - check(xml); + xml_test!( + test_focus_trap_in_group, + r#" + + + + + + + + + + + "# + ); + + // This test defines the expected behavior for focus-trap + // Focus-trap should trap navigation within its boundaries + xml_test!( + test_focus_trap_functionality, + r#" + + + + + + + "# + ); + + xml_test!( + test_nested_groups_and_traps, + r#" + + + + + + + + + + + "# + ); } #[test]