Skip to content

Tab navigation skips non-visible entities#24769

Merged
alice-i-cecile merged 1 commit into
bevyengine:mainfrom
Cyannide:add_visibility_limit_to_tabnaviation
Jul 8, 2026
Merged

Tab navigation skips non-visible entities#24769
alice-i-cecile merged 1 commit into
bevyengine:mainfrom
Cyannide:add_visibility_limit_to_tabnaviation

Conversation

@Cyannide

@Cyannide Cyannide commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Objective

In most cases invisible components should not be part of the focus cycle.

Solution

Changed the behavior of tab cycling to skip non-visible entities based on InheritedVisiblity.

Testing

Tested on a local project, you can set components in one of the existing examples with Visibility::Hidden and the cycle will skip the hidden components.

@Cyannide Cyannide force-pushed the add_visibility_limit_to_tabnaviation branch from 23df688 to 192ebfe Compare June 26, 2026 21:47
@kfc35 kfc35 added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 27, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in UI Jun 27, 2026
@alice-i-cecile alice-i-cecile requested a review from viridia July 6, 2026 06:29
alice-i-cecile
alice-i-cecile reviewed Jul 6, 2026
View reviewed changes
Comment thread crates/bevy_input_focus/src/tab_navigation.rs Outdated
/// tab groups.
pub modal: bool,

/// Override global setting for only focusing entities that are visible.

@alice-i-cecile alice-i-cecile Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs could be clearer.

alice-i-cecile
alice-i-cecile reviewed Jul 6, 2026
View reviewed changes
Comment thread crates/bevy_input_focus/src/tab_navigation.rs Outdated
pub struct TabNavigationConfig {
/// Whether to only focus entities that are visible, globally.
/// Can be overridden per-[`TabGroup`] via [`TabGroup::focus_only_visible`].
pub focus_only_visible: bool,

@alice-i-cecile alice-i-cecile Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should default to true IMO. Just make a migration guide entry: this is much less surprising behavior.

alice-i-cecile
alice-i-cecile requested changes Jul 6, 2026
View reviewed changes

@alice-i-cecile alice-i-cecile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swap the default behavior, add a migration guide entry and do a pass on the docs and I think this will be good to go <3

@alice-i-cecile alice-i-cecile added M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@viridia

viridia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Do we even need to support the old behavior? Focusing invisible entities seems broken.

@viridia

viridia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Also, I wonder if it should also exclude Display::None. Although this is tricky because Display::None is UI-specific, whereas tab navigation isn't (a diagetic UI can work with 2d or 3d meshes, and could support directional navigation without involving Nodes).

@Cyannide

Cyannide commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Do we even need to support the old behavior? Focusing invisible entities seems broken.

Yes, I was thinking the same but didn't initially want to break things for people.

@Cyannide

Cyannide commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Also, I wonder if it should also exclude Display::None. Although this is tricky because Display::None is UI-specific, whereas tab navigation isn't (a diagetic UI can work with 2d or 3d meshes, and could support directional navigation without involving Nodes).

That would require a dependency on bevy_ui and like you said, the input focus is more generic.

@viridia

viridia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Also, I wonder if it should also exclude Display::None. Although this is tricky because Display::None is UI-specific, whereas tab navigation isn't (a diagetic UI can work with 2d or 3d meshes, and could support directional navigation without involving Nodes).

That would require a dependency on bevy_ui and like you said, the input focus is more generic.

Fair enough.

I should note that in my own work (using bevy_reactor), when I want a button or other ui element to disappear, I don't use either Visibility or Display to do it. Instead, I use a reaction on a ghost node - meaning that the widget is entirely despawned when I don't want to see it.

@alice-i-cecile

alice-i-cecile commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

I'd prefer to just cut the old behavior completely. Remember to update your title / PR description :)

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior and removed C-Feature A new feature, making something new possible labels Jul 6, 2026
@Cyannide

Cyannide commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

So remove the flag and just leave the sane behavior?

@Cyannide
Tab navigation skips non-visible entities.
001345d 
Tab navigation now skips entities that are not visible in the hierarchy. Previously, TabNavigation would focus entities regardless of their InheritedVisibility, so hidden UI elements could still receive focus when cycling with Tab.

Entities without an InheritedVisibility component (non-UI entities) are still considered visible and remain focusable.
@Cyannide Cyannide force-pushed the add_visibility_limit_to_tabnaviation branch from ebf2786 to 001345d Compare July 6, 2026 18:38
@Cyannide Cyannide changed the title Added focus_only_visible setting to TabNavigationPlugin. Tab navigation skips non-visible entities Jul 6, 2026
@Cyannide

Cyannide commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

I'd prefer to just cut the old behavior completely. Remember to update your title / PR description :)

Since this is classified a bug fix now, it shouldn't need the migration guide, right?

@Cyannide Cyannide requested a review from alice-i-cecile July 7, 2026 17:05
@alice-i-cecile

alice-i-cecile commented Jul 8, 2026

Copy link
Copy Markdown
Member

Yep :)

@alice-i-cecile alice-i-cecile removed the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 8, 2026
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 8, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 8, 2026
Merged via the queue into bevyengine:main with commit 4ff6565 Jul 8, 2026
43 of 44 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in UI Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes
+1 more reviewer
@viridia viridia viridia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

UI
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Cyannide @viridia @alice-i-cecile @kfc35