-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Merged by Bors] - Integrate AccessKit #6874
Changes from 64 commits
0e07c28
8b67099
269293b
c78a81d
07c6ec8
8503824
6c8bc71
3465deb
26444b5
9a9cf83
7d7ac22
1cf5730
0fc80fc
ebaac9b
058264b
efd9d80
b86a2c3
b52e587
06fe868
c503915
79bebfa
f84cbf0
6ec2a87
647a408
2f09ebb
3aa8c8e
43db5ac
f466741
f276404
791fbe4
aac5116
9fee1ee
05b03d3
06714ea
7894849
eafe226
2677ae4
3b75e5e
69166c6
1571633
f00c893
6ed0c14
f67de8e
ca99b51
eb6a30d
3b25d00
7ddfea0
0ac0051
f9f251d
293b240
7771ddc
2b6a114
c6e36c1
a375b56
c4e95bc
2917a86
6ea06b6
03a89e3
fed306d
47e8d0e
d7a1011
0349cd7
d9a9adc
5841343
8d43a23
cc2ed5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
---|---|---|
|
@@ -7,6 +7,11 @@ pub mod prelude; | |
mod default_plugins; | ||
pub use default_plugins::*; | ||
|
||
pub mod a11y { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Module level docs would be super nice here. |
||
//! Integrate with platform accessibility APIs. | ||
pub use bevy_a11y::*; | ||
} | ||
|
||
pub mod app { | ||
//! Build bevy apps, create plugins, and read events. | ||
pub use bevy_app::*; | ||
|
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious: Is there a reason this is used for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.