[Merged by Bors] - Integrate AccessKit #6874
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| [package] | ||
| name = "bevy_a11y" | ||
| version = "0.9.0" | ||
| edition = "2021" | ||
| description = "Provides accessibility support for Bevy Engine" | ||
| homepage = "https://bevyengine.org" | ||
| repository = "https://github.com/bevyengine/bevy" | ||
| license = "MIT OR Apache-2.0" | ||
| keywords = ["bevy", "accessibility", "a11y"] | ||
|
|
||
| [dependencies] | ||
| # bevy | ||
| bevy_app = { path = "../bevy_app", version = "0.9.0" } | ||
| bevy_derive = { path = "../bevy_derive", version = "0.9.0" } | ||
| bevy_ecs = { path = "../bevy_ecs", version = "0.9.0" } | ||
|
|
||
| accesskit = "0.10" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| //! Accessibility for Bevy | ||
|
|
||
| #![warn(missing_docs)] | ||
ndarilek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #![forbid(unsafe_code)] | ||
|
|
||
| use std::{ | ||
| num::NonZeroU128, | ||
| sync::{atomic::AtomicBool, Arc}, | ||
| }; | ||
|
|
||
| pub use accesskit; | ||
| use accesskit::{NodeBuilder, NodeId}; | ||
| use bevy_app::Plugin; | ||
| use bevy_derive::{Deref, DerefMut}; | ||
| use bevy_ecs::{ | ||
| prelude::{Component, Entity}, | ||
| system::Resource, | ||
| }; | ||
|
|
||
| /// Resource that tracks whether an assistive technology has requested | ||
| /// accessibility information. | ||
| /// | ||
| /// Useful if a third-party plugin needs to conditionally integrate with | ||
| /// `AccessKit` | ||
| #[derive(Resource, Default, Clone, Debug, Deref, DerefMut)] | ||
| pub struct AccessibilityRequested(Arc<AtomicBool>); | ||
|
|
||
| /// Component to wrap a [`accesskit::Node`], representing this entity to the platform's | ||
| /// accessibility API. | ||
| /// | ||
| /// If an entity has a parent, and that parent also has an `AccessibilityNode`, | ||
| /// the entity's node will be a child of the parent's node. | ||
| /// | ||
| /// If the entity doesn't have a parent, or if the immediate parent doesn't have | ||
| /// an `AccessibilityNode`, its node will be an immediate child of the primary window. | ||
| #[derive(Component, Clone, Deref, DerefMut)] | ||
| pub struct AccessibilityNode(pub NodeBuilder); | ||
|
|
||
| impl From<NodeBuilder> for AccessibilityNode { | ||
| fn from(node: NodeBuilder) -> Self { | ||
| Self(node) | ||
| } | ||
| } | ||
|
|
||
| /// Extensions to ease integrating entities with [`AccessKit`](https://accesskit.dev). | ||
| pub trait AccessKitEntityExt { | ||
|
There was a problem hiding this comment. Is this trait 100% needed and also required to be in the public interface? Seems like it could be an internal utility function. There was a problem hiding this comment. Yes. Other UIs needing to integrate with AccessKit need a standard way of mapping from entity IDs to accessibility nodes so every view of the accessibility tree is consistent. That ID needs to be non-zero, so for now I just add 1 to the entity ID. I'll rename it to There was a problem hiding this comment. Actually, with windows as entities now, it may be possible to make this an implementation detail. Hadn't looked at it in a while but initially it had to add 2 because 1 was reserved for the primary window ID. But with everything sharing the same ID space, I may be able to make it There was a problem hiding this comment. Unfortunately, this needs to be exposed for UI and other accessibility implementors to agree on an entity-to-AccessKit ID mapping. AccessKit IDs need to be non-zero so it just adds 1. There was a problem hiding this comment. Agreed: I think this is mandatory for bevy to define. |
||
| /// Convert an entity to a stable [`NodeId`]. | ||
| fn to_node_id(&self) -> NodeId; | ||
| } | ||
|
|
||
| impl AccessKitEntityExt for Entity { | ||
| fn to_node_id(&self) -> NodeId { | ||
| let id = NonZeroU128::new(self.to_bits() as u128 + 1); | ||
| NodeId(id.unwrap()) | ||
| } | ||
| } | ||
|
|
||
| /// Resource representing which entity has keyboard focus, if any. | ||
| #[derive(Resource, Default, Deref, DerefMut)] | ||
| pub struct Focus(Option<Entity>); | ||
|
|
||
| /// Plugin managing non-GUI aspects of integrating with accessibility APIs. | ||
| pub struct AccessibilityPlugin; | ||
|
|
||
| impl Plugin for AccessibilityPlugin { | ||
| fn build(&self, app: &mut bevy_app::App) { | ||
| app.init_resource::<AccessibilityRequested>() | ||
| .init_resource::<Focus>(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| use bevy_a11y::{ | ||
| accesskit::{NodeBuilder, Rect, Role}, | ||
| AccessibilityNode, | ||
| }; | ||
| use bevy_app::{App, Plugin}; | ||
|
|
||
| use bevy_ecs::{ | ||
| prelude::Entity, | ||
| query::{Changed, Or, Without}, | ||
| system::{Commands, Query}, | ||
| }; | ||
| use bevy_hierarchy::Children; | ||
|
|
||
| use bevy_render::prelude::Camera; | ||
| use bevy_text::Text; | ||
| use bevy_transform::prelude::GlobalTransform; | ||
|
|
||
| use crate::{ | ||
| prelude::{Button, Label}, | ||
| Node, UiImage, | ||
| }; | ||
|
|
||
| fn calc_name(texts: &Query<&Text>, children: &Children) -> Option<Box<str>> { | ||
|
There was a problem hiding this comment. It's not clear to me that we should be reading the text here, rather than using the There was a problem hiding this comment. I agree in principle, but I'd like to see these sorts of ideas pushed forward in a general UI overhaul. Put in terms more familiar to those of us who work with accessibility, it's better when designed in from the start vs. bolted on later. To be clear, I'm not critiquing bevy_ui for not being accessible from the start, particularly since bevy_ui is lacking quite a few things and accessibility is in good company. :) I'd just like to see us address these sorts of issues as part of a greater UI refactor of which accessibility is just one part. Without this groundwork, we wouldn't necessarily be able to meaningfully include accessibility as a design consideration when bevy_ui eventually gets more attention. There was a problem hiding this comment. Just curious: Is there a reason this is used for There was a problem hiding this comment. Mainly because I didn't want to take on fixing bevy_ui's shortcomings in this PR. I needed something to test with, and we don't currently have a way of describing UI widgets in any form. So I calculated a name/title and left it there. There was a problem hiding this comment. I see. And so the shortcoming you're talking about is that bevy_ui doesn't enforce If it did, would this function instead be used to extract the node's description? There was a problem hiding this comment. It's more that bevy_ui will eventually need something like Flutter's Semantics class to truly be accessible. Ideally, in that situation, we'll have clearer understandings of how and when we're able to auto-generate names/descriptions, the Semantics component will be nicely integrated with the editor so accessibility properties can be set directly, etc. And I'm trying to take a lighter touch with all those things right now because I don't know what the right answer is. Also, names and descriptions are very different things, and you don't necessarily want to scattershot the same property value everywhere so e.g. you don't double-announce a name because a screen reader might read a widget like "<name> <description>: button" or similar. Which properties to use and when is fairly complex and differs per widget, but in general setting names is universal and fairly low-hanging fruit. Something more complex I likely wouldn't auto-generate unless the widget semantics clearly call for it. Ideally, we'll use AccessKit's NodeBuilder or a supported subset of its schema as the core of our Semantics component, and come to consensus about how we automatically fill in properties vs. expecting developers to fill them in themselves. There was a problem hiding this comment. Makes sense, thanks for the explanation! |
||
| let mut name = None; | ||
| for child in children.iter() { | ||
| if let Ok(text) = texts.get(*child) { | ||
| let values = text | ||
| .sections | ||
| .iter() | ||
| .map(|v| v.value.to_string()) | ||
| .collect::<Vec<String>>(); | ||
| name = Some(values.join(" ")); | ||
| } | ||
| } | ||
| name.map(|v| v.into_boxed_str()) | ||
| } | ||
|
|
||
| fn calc_bounds( | ||
| camera: Query<(&Camera, &GlobalTransform)>, | ||
| mut nodes: Query< | ||
| (&mut AccessibilityNode, &Node, &GlobalTransform), | ||
| Or<(Changed<Node>, Changed<GlobalTransform>)>, | ||
| >, | ||
| ) { | ||
| if let Ok((camera, camera_transform)) = camera.get_single() { | ||
|
There was a problem hiding this comment. There may be more than one camera that meets the requirements of this query. This might need to change to iteration instead of using There was a problem hiding this comment. Can you please either point to a multi-camera UI example, or let me know how to determine which camera to use when calculating the correct translation? I'm trying to avoid taking responsibility for more of bevy_ui's rough edges than I absolutely have to, and I think I'd rather this fail with outliers for now, than have to make these types of decisions on my own. But the examples I've worked with all seem to use a single camera, so... |
||
| for (mut accessible, node, transform) in &mut nodes { | ||
| if let Some(translation) = | ||
|
There was a problem hiding this comment. style nit: This could use |
||
| camera.world_to_viewport(camera_transform, transform.translation()) | ||
| { | ||
| let bounds = Rect::new( | ||
| translation.x.into(), | ||
| translation.y.into(), | ||
| (translation.x + node.calculated_size.x).into(), | ||
| (translation.y + node.calculated_size.y).into(), | ||
| ); | ||
| accessible.set_bounds(bounds); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn button_changed( | ||
| mut commands: Commands, | ||
| mut query: Query<(Entity, &Children, Option<&mut AccessibilityNode>), Changed<Button>>, | ||
| texts: Query<&Text>, | ||
| ) { | ||
| for (entity, children, accessible) in &mut query { | ||
| let name = calc_name(&texts, children); | ||
| if let Some(mut accessible) = accessible { | ||
| accessible.set_role(Role::Button); | ||
| if let Some(name) = name { | ||
| accessible.set_name(name); | ||
| } else { | ||
| accessible.clear_name(); | ||
| } | ||
| } else { | ||
| let mut node = NodeBuilder::new(Role::Button); | ||
| if let Some(name) = name { | ||
| node.set_name(name); | ||
| } | ||
| commands | ||
| .entity(entity) | ||
| .insert(AccessibilityNode::from(node)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn image_changed( | ||
| mut commands: Commands, | ||
| mut query: Query< | ||
| (Entity, &Children, Option<&mut AccessibilityNode>), | ||
| (Changed<UiImage>, Without<Button>), | ||
| >, | ||
| texts: Query<&Text>, | ||
| ) { | ||
| for (entity, children, accessible) in &mut query { | ||
| let name = calc_name(&texts, children); | ||
| if let Some(mut accessible) = accessible { | ||
| accessible.set_role(Role::Image); | ||
| if let Some(name) = name { | ||
| accessible.set_name(name); | ||
| } else { | ||
| accessible.clear_name(); | ||
| } | ||
| } else { | ||
| let mut node = NodeBuilder::new(Role::Image); | ||
| if let Some(name) = name { | ||
| node.set_name(name); | ||
| } | ||
| commands | ||
| .entity(entity) | ||
| .insert(AccessibilityNode::from(node)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn label_changed( | ||
| mut commands: Commands, | ||
| mut query: Query<(Entity, &Text, Option<&mut AccessibilityNode>), Changed<Label>>, | ||
| ) { | ||
| for (entity, text, accessible) in &mut query { | ||
| let values = text | ||
| .sections | ||
| .iter() | ||
| .map(|v| v.value.to_string()) | ||
| .collect::<Vec<String>>(); | ||
| let name = Some(values.join(" ").into_boxed_str()); | ||
| if let Some(mut accessible) = accessible { | ||
| accessible.set_role(Role::LabelText); | ||
| if let Some(name) = name { | ||
| accessible.set_name(name); | ||
| } else { | ||
| accessible.clear_name(); | ||
| } | ||
| } else { | ||
| let mut node = NodeBuilder::new(Role::LabelText); | ||
| if let Some(name) = name { | ||
| node.set_name(name); | ||
| } | ||
| commands | ||
| .entity(entity) | ||
| .insert(AccessibilityNode::from(node)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// `AccessKit` integration for `bevy_ui`. | ||
| pub(crate) struct AccessibilityPlugin; | ||
|
|
||
| impl Plugin for AccessibilityPlugin { | ||
| fn build(&self, app: &mut App) { | ||
| app.add_system(calc_bounds) | ||
| .add_system(button_changed) | ||
| .add_system(image_changed) | ||
| .add_system(label_changed); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| use bevy_ecs::prelude::Component; | ||
| use bevy_ecs::reflect::ReflectComponent; | ||
| use bevy_reflect::std_traits::ReflectDefault; | ||
| use bevy_reflect::Reflect; | ||
|
|
||
| /// Marker struct for labels | ||
|
There was a problem hiding this comment. Nit: It might be good to indicate why this component should be used. Seems like it's mainly for the There was a problem hiding this comment. If you have any wording suggestions I'm open to them--brain is a bit fried ATM. Alternately I can just remove it in favor of attaching a custom There was a problem hiding this comment. Also, I feel like a label is a fairly common UI widget type, and we're going to want this or something like it at some point anyway. So it isn't really an accessibility thing, more a general widget I probably introduced before it's time thing. There was a problem hiding this comment. I don't have any wording suggestions off the top of my head. If it's meant to be part of a larger widget ecosystem, then maybe we come back to this (proper documentation) when we're ready to fully implement it? I imagine its usage and design could change a lot between now and when we truly define a "label" widget. |
||
| #[derive(Component, Debug, Default, Clone, Copy, Reflect)] | ||
| #[reflect(Component, Default)] | ||
| pub struct Label; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does add a non-trivial number of new dependencies. On linux we now have 358 dependencies, up from 291. That is 61 new deps: a 23% increase.
Obviously accessibility is important. But numbers that big deserve increased scrutiny, even for important features. Where are these deps coming from? What purpose do they serve? Do we need them? What are the compile time implications? What do the dep counts look like on other platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also binary-size implications, especially on WASM. I'm guessing right now WASM is largely unaffected, as these deps appear to be coming from the platform adapters and it doesn't look like there is a WASM adapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the practical / technical issues mentioned above, this is also an optics issue. Bevy is already considered to be "big" and that is a turn off for some people. Substantial dep count increases won't help our case here.
It is definitely hard to add new features without adding new deps. Bevy needs to add new features, therefore our deps will go up. But for all of the reasons listed in this thread we should be working extra hard to drive these numbers down.