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-Enhancement A new feature 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.

@0HyperCube
Copy link
Author

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.

Thanks for the code review @nicopap. I've had a read through of the RFC and I can't see anything that is directly related to pitfalls in the current, very basic document flow based tab navigation (other than a lot of missing features); although I would be very happy for you to add commits / PRs for anything.

@0HyperCube 0HyperCube requested a review from nicopap June 22, 2023 16:34
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.

Pretty good. I think there should be a way to toggle the navigation system. At least until we switch to an event-based system or allow key mapping.

Focused {
/// Focus has been reached through keyboard navigation and so a focus style should be displayed.
/// This is similar to the `:focus-visible` pseudo-class in css.
visible: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this a field of FocusState::Focused instead of a field of Focusable?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't really make sense to have an entity that is not focused, but at the same time has the focus as visible. The visibility of the focus is only applicable when the entity is actually focused.

I'm happy to change this if you prefer, but I don't see any compelling reasons why having this as a field of Focusable would be advantageous.

Copy link
Contributor

Choose a reason for hiding this comment

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

I approved the PR. I'll leave it up to you.

Personally I try to avoid boolean fields generally. It's a bad pattern. It especially makes little sense as enum field. It can be replaced by:

enum FocusState {
  FocusedVisible,
  FocusedInvisible,
  None,

Also I don't think visible is the appropriate term here. Semantically I think it means "last interaction was through a keyboard navigation interaction". Whether it's visible or not is up to the end user. Within bevy code, using "visible" is just plain misleading. I wouldn't hold the web standards as a reference for naming things, it's notoriously bad in this regard (eg: referer header).

Also I still am doubtful this is something we want for bevy. Can you give me the example of a game with tab-based keyboard navigation?

Copy link
Author

Choose a reason for hiding this comment

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

Can you give me the example of a game with tab-based keyboard navigation?

Games with form style interfaces (like username / password input boxes e.g. Veloren) will have at least have some kind of tab navigation.

Copy link
Author

Choose a reason for hiding this comment

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

Also if bevy UI is used to make a level editor, then tab navigation will be necessary for that.

crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
examples/ui/keyboard_navigation.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/keyboard_navigation.rs Outdated Show resolved Hide resolved
@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?

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-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it 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.

None yet

8 participants