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]