From 0777cc26aa82afff3a73fb7596c29f2a0c14d514 Mon Sep 17 00:00:00 2001 From: Chris Berger Date: Mon, 26 May 2025 13:32:10 -0600 Subject: [PATCH] Navigate through TabGroups in order. (#19218) # Objective Fixes #19156 ## Solution There was a call to sort by `TabIndex` after iterating through all the `TabGroup`s. Simply removing that line of code made the new test case pass (navigating through `TabIndex` within different `TabGroup`s and kept everything else working as expected as far as I can tell. I went ahead and broke the `focusable` vec down into per-group vecs that get sorted by `TabIndex` and appended to the main vec in group sorted order to maintain the sorting that was being done before, but respecting the `TabGroup` sorting that was missing in the issue that this PR addresses. ## Testing I added a new unit test that reproduced the issue outlined above that will run in CI. This test was failing before deleting the sort, and now both unit tests are passing. --- crates/bevy_input_focus/src/tab_navigation.rs | 79 ++++++++++++++++--- 1 file changed, 67 insertions(+), 12 deletions(-) diff --git a/crates/bevy_input_focus/src/tab_navigation.rs b/crates/bevy_input_focus/src/tab_navigation.rs index a5fe691458..60df130ae0 100644 --- a/crates/bevy_input_focus/src/tab_navigation.rs +++ b/crates/bevy_input_focus/src/tab_navigation.rs @@ -221,7 +221,7 @@ impl TabNavigation<'_, '_> { action: NavAction, ) -> Result { // List of all focusable entities found. - let mut focusable: Vec<(Entity, TabIndex)> = + let mut focusable: Vec<(Entity, TabIndex, usize)> = Vec::with_capacity(self.tabindex_query.iter().len()); match tabgroup { @@ -229,7 +229,7 @@ impl TabNavigation<'_, '_> { // We're in a modal tab group, then gather all tab indices in that group. if let Ok((_, _, children)) = self.tabgroup_query.get(tg_entity) { for child in children.iter() { - self.gather_focusable(&mut focusable, *child); + self.gather_focusable(&mut focusable, *child, 0); } } } @@ -245,9 +245,12 @@ impl TabNavigation<'_, '_> { tab_groups.sort_by_key(|(_, tg)| tg.order); // Search group descendants - tab_groups.iter().for_each(|(tg_entity, _)| { - self.gather_focusable(&mut focusable, *tg_entity); - }); + tab_groups + .iter() + .enumerate() + .for_each(|(idx, (tg_entity, _))| { + self.gather_focusable(&mut focusable, *tg_entity, idx); + }); } } @@ -255,8 +258,14 @@ impl TabNavigation<'_, '_> { return Err(TabNavigationError::NoFocusableEntities); } - // Stable sort by tabindex - focusable.sort_by_key(|(_, idx)| *idx); + // Sort by TabGroup and then TabIndex + focusable.sort_by(|(_, a_tab_idx, a_group), (_, b_tab_idx, b_group)| { + if a_group == b_group { + a_tab_idx.cmp(b_tab_idx) + } else { + a_group.cmp(b_group) + } + }); let index = focusable.iter().position(|e| Some(e.0) == focus.0); let count = focusable.len(); @@ -267,31 +276,36 @@ impl TabNavigation<'_, '_> { (None, NavAction::Previous) | (_, NavAction::Last) => count - 1, }; match focusable.get(next) { - Some((entity, _)) => Ok(*entity), + Some((entity, _, _)) => Ok(*entity), None => Err(TabNavigationError::FailedToNavigateToNextFocusableEntity), } } /// Gather all focusable entities in tree order. - fn gather_focusable(&self, out: &mut Vec<(Entity, TabIndex)>, parent: Entity) { + fn gather_focusable( + &self, + out: &mut Vec<(Entity, TabIndex, usize)>, + parent: Entity, + tab_group_idx: usize, + ) { if let Ok((entity, tabindex, children)) = self.tabindex_query.get(parent) { if let Some(tabindex) = tabindex { if tabindex.0 >= 0 { - out.push((entity, *tabindex)); + out.push((entity, *tabindex, tab_group_idx)); } } if let Some(children) = children { for child in children.iter() { // Don't traverse into tab groups, as they are handled separately. if self.tabgroup_query.get(*child).is_err() { - self.gather_focusable(out, *child); + self.gather_focusable(out, *child, tab_group_idx); } } } } else if let Ok((_, tabgroup, children)) = self.tabgroup_query.get(parent) { if !tabgroup.modal { for child in children.iter() { - self.gather_focusable(out, *child); + self.gather_focusable(out, *child, tab_group_idx); } } } @@ -397,4 +411,45 @@ mod tests { let last_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::Last); assert_eq!(last_entity, Ok(tab_entity_2)); } + + #[test] + fn test_tab_navigation_between_groups_is_sorted_by_group() { + let mut app = App::new(); + let world = app.world_mut(); + + let tab_group_1 = world.spawn(TabGroup::new(0)).id(); + let tab_entity_1 = world.spawn((TabIndex(0), ChildOf(tab_group_1))).id(); + let tab_entity_2 = world.spawn((TabIndex(1), ChildOf(tab_group_1))).id(); + + let tab_group_2 = world.spawn(TabGroup::new(1)).id(); + let tab_entity_3 = world.spawn((TabIndex(0), ChildOf(tab_group_2))).id(); + let tab_entity_4 = world.spawn((TabIndex(1), ChildOf(tab_group_2))).id(); + + let mut system_state: SystemState = SystemState::new(world); + let tab_navigation = system_state.get(world); + assert_eq!(tab_navigation.tabgroup_query.iter().count(), 2); + assert_eq!(tab_navigation.tabindex_query.iter().count(), 4); + + let next_entity = + tab_navigation.navigate(&InputFocus::from_entity(tab_entity_1), NavAction::Next); + assert_eq!(next_entity, Ok(tab_entity_2)); + + let prev_entity = + tab_navigation.navigate(&InputFocus::from_entity(tab_entity_2), NavAction::Previous); + assert_eq!(prev_entity, Ok(tab_entity_1)); + + let first_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::First); + assert_eq!(first_entity, Ok(tab_entity_1)); + + let last_entity = tab_navigation.navigate(&InputFocus::default(), NavAction::Last); + assert_eq!(last_entity, Ok(tab_entity_4)); + + let next_from_end_of_group_entity = + tab_navigation.navigate(&InputFocus::from_entity(tab_entity_2), NavAction::Next); + assert_eq!(next_from_end_of_group_entity, Ok(tab_entity_3)); + + let prev_entity_from_start_of_group = + tab_navigation.navigate(&InputFocus::from_entity(tab_entity_3), NavAction::Previous); + assert_eq!(prev_entity_from_start_of_group, Ok(tab_entity_2)); + } }