Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UI keyboard navigation #8882

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

0HyperCube
Copy link

@0HyperCube 0HyperCube commented Jun 18, 2023

Objective

Add some very simple keyboard navigation to bevy_ui, based on the order that the entities appear within the hierarchy.

Solution

The Focus resource (from the bevy_a11y crate) can be changed with tab, shift+tab or by clicking on an entity. Entities that are visible and have the new Focusable component can be navigated to. To aid styling, a new Focusable component has is_focused and is_focus_visible functions. Focus is visible when keyboard (or in future gamepad) navigation is used.


Changelog

Added

  • FocusState component containing the current focus state of the entity to aid with styling.

Changed

  • Focus resource now also has a focus_visible property reflecting the :focus-visible css pseudo-class.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets labels Jun 18, 2023
@alice-i-cecile
Copy link
Member

@ndarilek I'd like your opinion on how this can integrate with AccessKit :)

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

1 similar comment
@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@0HyperCube
Copy link
Author

I'd like your opinion on how this can integrate with AccessKit :)

It looks like bevy would need to send the relevant focused entity with the TreeUpdate event.

Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started! This will be nice for both accessibility and general UX.

Some initial thoughts:

  • We will probably need to sync the "UI focus" with the "Access Kit" focus -- both ways. So a keyboard focus change will need to update the info for the screenreader and the screenreader navigation should also adapt the UI style. @ndarilek should know more details about this.
  • We shouldn't hard-code keybindings for this. Especially Tab is a common keybinding used in games, so this might collide with other controls. We can keep these values as defaults, but this should be configurable.
  • Inside menus, games often have more ways to control the UI. For example, using the arrow keys and gamepad input. I'm not sure if we should have these relatively common use cases built-in as well, but at some point we should at least add the possibility for the user to add this. Otherwise, we might get some interactions that don't properly update the focus state and screenreader info if the devs don't know about the details.

These things don't necessarily need to be addressed in this PR, but I think we should investigate how easily they could be integrated in order to not block ourselves in the future.

@0HyperCube
Copy link
Author

I have added access kit integration.

@0HyperCube
Copy link
Author

We shouldn't hard-code keybindings for this. Especially Tab is a common keybinding used in games, so this might collide with other controls. We can keep these values as defaults, but this should be configurable.

I can't really think of any other keybindings for keyboard navigation (other than arrow keys - but they are seperate), but I agree that we would need some API for stopping the tab key from modifying the UI if it has modified the game. How would you suggest allowing this to be configured @TimJentzsch?

For example, using the arrow keys and gamepad input. I'm not sure if we should have these relatively common use cases built-in as well, but at some point we should at least add the possibility for the user to add this. Otherwise, we might get some interactions that don't properly update the focus state and screenreader info if the devs don't know about the details.

I agree that arrow key navigation is very important for lists or for a row of buttons in a dialogue menu. In order to handle arrow key navigation, the game would have to declare a node as being a navigable row or column. Do you have any thoughts on what the API could look like?

@TimJentzsch
Copy link
Contributor

TimJentzsch commented Jun 18, 2023

How would you suggest allowing this to be configured?

Maybe a FocusSettings resource could work. It could store the key bindings for navigation (if we want them to be configurable) and perhaps a bool that can be used to disable the behavior if necessary. I'm curious to hear what other people think about this.

In order to handle arrow key navigation, the game would have to declare a node as being a navigable row or column. Do you have any thoughts on what the API could look like?

This is probably hard to solve for the general case... although it would be nice if we could somehow provide this functionality if needed. We also have to keep internationalization in mind -- in the future, the locale could affect how the UI is rendered, e.g. in some languages the text is layed out vertical so menus might be layed out horizontally (automatically via flexbox) and the other arrow keys would make more sense for navigation.
I can't think of a nice API of the bat, but maybe we can at least make it easy for the devs (or plugins) to implement their own system on top of this. E.g. triggering an event that can change the focus to a given entity.

@github-actions
Copy link
Contributor

You added a new example but didn't update the readme. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@nicopap
Copy link
Contributor

nicopap commented Jun 18, 2023

Can confirm it's hard to solve for the general case. This is what https://lib.rs/crates/bevy-ui-navigation does. And there even was An RFC (RFC41) which I had to close due to feature creep on my side.

@0HyperCube
Copy link
Author

I didn't realise that navigation could be so complex!

I'm sorry that I duplicated some of your code; I managed to completely miss it when searching for keyboard navigation. But perhaps some very basic built in tab navigation is still a useful stopgap measure until the more complete implementation is added?

@nicopap
Copy link
Contributor

nicopap commented Jun 18, 2023

I'm taking a look at your PR and I think it solves a few of the problems in my impl! 😅 Given the time I spent on this, I'm a bit ashamed…

I'll take a more careful look at the code tomorrow, but it seems like a good start. and not victim of the feature creep bevy-ui-navigation has.

@alice-i-cecile
Copy link
Member

Yeah, my view is that now that we've taken a look at the problem space more broadly we're well equipped to get an incremental start in and build from there :)

@ndarilek
Copy link
Contributor

It's been a while since I did the AccessKit work but I believe I added a Focus resource which stores the entity of the widget with keyboard focus, which should correctly update the tree with the focused widget.
Note that this is keyboard focus. I.e. mousing over a widget should not update this resource. I suspect keyboard and gamepad are interchangeable here.
Thanks for caring about accessibility.

@alice-i-cecile
Copy link
Member

I suspect keyboard and gamepad are interchangeable here.

They are :) The element focused should always be the same, the only difference is how you navigate between focused elements.

@Hellzbellz123
Copy link

Hellzbellz123 commented Jun 19, 2023

i dont know how feasable this actually is, but the way i would solve this is ordering the buttons by the global transform, larger global transform goes at the end of the list and as you press tab it slowly goes through the list of buttons, top too bottom, left too right, as far as im aware most ui goes this way, if buttons are on the same Y plane then the X plane is used for ordering, buttons shouldnt be on top of each other yes?

would this work for 3d menus? my brain says it would but im not sure

@ndarilek
Copy link
Contributor

Might be worth looking at Godot's APIs/handling of this.

I believe, and it's been a while since I've used Godot, that it has both an auto-placement API as well as a way to explicitly tag up/down/left/right/next/previous focus from any given widget. So if the auto layout algorithm does something that makes no sense, you can explicitly tag widgets to receive focus when d-pad/tab/shift-tab controls are used on a widget-by-widget basis. That's what I'd suggest here. An auto-layout algorithm will probably get it right some large amount of time, but there will always be corner cases it misses or handles poorly.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

This needs some work. Notably to avoid iterating over all entities.

I would suggest also taking a glance at RFC 41, the discussion on the RFC and the ui-navigation API. There is a lot of edge cases involved, and it would be a shame to miss the ones we already uncovered.

I'd be happy to open a pr on 0HyperCube/bevy. I can cook up something tomorrow. Please tell me if you'd like that.

crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
@0HyperCube
Copy link
Author

It's been a while since I did the AccessKit work but I believe I added a Focus resource which stores the entity of the widget with keyboard focus, which should correctly update the tree with the focused widget. Note that this is keyboard focus. I.e. mousing over a widget should not update this resource.

@ndarilek This PR currently uses the Focus resource from the bevy_a11y crate. This is updated when keyboard navigation occurs or the the user clicks on a button, which is the same as how it works in HTML.

@nicopap nicopap self-requested a review June 23, 2023 19:20
Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

If it's not too much effort, I think this would be a good candidate for automated tests.
Maybe one that simulates a Tab press and then asserts that the focused entity is the next element and one that simulates Shift+Tab and asserts the previous element.

Otherwise, this looks good to me :)

crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Show resolved Hide resolved
Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

Nicely done!

@TimJentzsch
Copy link
Contributor

@0HyperCube Looks like cargo fmt is failing in CI

@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 25, 2023
@mockersf
Copy link
Member

Could the existing component Interaction and the new Focusable be merged into one? Focus seems like a kind of interaction

@mockersf
Copy link
Member

not a fan of mixing keyboard and mouse navigation / focus / interaction with the same components, this often have surprising side effects.

In the example, if I click on one of the button, then move my mouse to another and press space, the first button gets clicked with no indication it's going to be the one selected

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 26, 2023
@0HyperCube
Copy link
Author

not a fan of mixing keyboard and mouse navigation / focus / interaction with the same components, this often have surprising side effects.

In the example, if I click on one of the button, then move my mouse to another and press space, the first button gets clicked with no indication it's going to be the one selected

This is the standard behaviour on the web @mockersf; although I'm open to suggestions to improve this behaviour.

@0HyperCube
Copy link
Author

Could the existing component Interaction and the new Focusable be merged into one? Focus seems like a kind of interaction

We could potentially do something like:

struct Interactable {
    focus_state: FocusState,
    hovered: bool,
    clicked: bool,
}

However, note that not all interactable UI nodes should be keyboard focusable. In HTML this can be done with tabindex="-1" which is used on dialogues (before the <dialog> tag) and to provide keyboard navigation for complex custom widgets where focus needs to be controlled by code.

@0HyperCube 0HyperCube requested a review from mockersf June 29, 2023 16:14
@musjj
Copy link

musjj commented Apr 2, 2024

Any chance that this could make it to 0.14?

@alice-i-cecile alice-i-cecile added S-Needs-SME Decision or review from an SME is required and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 22, 2024
@ConnorBP
Copy link

Any chance that this could make it to 0.14?

I would love to see this get added

@ConnorBP
Copy link

Trying to use some of the design from this in the lastest stable bevy 0.14.2, and it would seem that setting Focus() to some UI node (such as a text box UI element, or button) is causing some sort of race condition with the a11y accesskit node tree (Focused node id not in node list panic)

myapp::ui::navigation: setting focus state to Focusable { focus_state: Focused { visible: true } } for ent Some(Entity { index: 4, generation: 1 })
thread 'main' panicked at C:\Users\conno.cargo\registry\src\index.crates.io-6f17d22bba15001f\accesskit_consumer-0.22.0\src\tree.rs:36:13:
Focused id #4294967300 is not in the node list

@ndarilek
Copy link
Contributor

Do you have a minimal repro? Hoping to do an accesskit improvement pass this cycle and would like to address this.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants