-
-
Notifications
You must be signed in to change notification settings - Fork 301
Integrate AccessKit #238
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
Integrate AccessKit #238
Conversation
|
@ndarilek thank you very much for this PR! Could you give me some hints about how I can test it on MacOS or Windows? UI accessibility is a really new topic for me, but I'll be glad to become more educated about it. |
879a24b to
8956b27
Compare
src/lib.rs
Outdated
| for window in new_windows.iter() { | ||
| let context = EguiContext::default(); | ||
| if let Some(adapters) = &adapters { | ||
| println!("Got adapters"); |
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.
Not sure if it's ready for review or not, but we should get rid of println! statements (or we can replace them with log::debug!)
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.
ah, nvm, didn't notice the "WIP" tag in the commit :)
but feel free to leave the ones you can find useful for debugging as log::debug
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.
Oh heh, totally forgot I'd opened a PR for this. No worries.
Yes, thanks for flagging the printlns, I needed something to output directly to my screen reader without the rest of the debug/info-level logging coming along with it. :) Will definitely remove those from the final PR.
Does the rest look OK from the egui side, though? In particular, I didn't know if I needed to copy accesskit_update from egui differently. It definitely won't work under 0.15 due to this but I'm still not entirely clear on how/where properties are picked up from egui. Hopefully I got it right, I'd like to merge it early in the 0.16 release cycle if possible.
Thanks.
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.
Cool, ty! :)
Does the rest look OK from the egui side, though?
Yeah, there's something in the code I'm not so sure about.
Here, you've added creation of new EguiContext for which you enable accesskit, but then the context isn't passed anywhere, so it just gets destroyed when it leaves the scope.
You probably want to insert the context variable to the window entity:
commands.entity(window).insert(context).
I didn't know if I needed to copy
accesskit_updatefrom egui differently.
Looks good to me so far, even though I don't particularly know about Bevy's ManageAccessibilityUpdates, so I can't confirm that with confidence. 🙃
Another thing I'd probably like this PR to add is a new example (if that makes sense). I assume activating accessibility features requires some setup in the user code as well? If so, that's something for which an example would be useful.
I'd also appreciate if you could provide some instructions on how to test this on Windows or MacOS. Maybe they can go as comments in the example code. I'd be happy to play around with it and test it, but I barely know anything about accessibility features unfortunately.
src/lib.rs
Outdated
|
|
||
| app.add_systems( | ||
| PostUpdate, | ||
| update_accessibility |
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.
Let's add this system to the EguiPostUpdateSet::PostProcessOutput set, and configure the whole set to run before(AccessibilitySystem::Update).
|
I believe I've addressed all your PR feedback and, thanks to your catch of me failing to add the context to the window, this now works under Linux! (It wasn't before and I thought it might be a Bevy issue.) There's not really anything someone has to know/do to use accessibility support, so I don't think it needs any specific examples. Egui widgets just work automatically. I'd like to add additional accessibility helpers, but those would be in egui itself so documented there. Testing is a bit weird right now. It works under Linux/Orca and maybe macOS/VoiceOver. VO is a bit complicated to use and I don't regularly, but I think that pressing cmd-f5 should bring it up. There are lots of review commands, but you should be able to at least tab through the interface, arrow around controls, etc. Unfortunately Windows won't work at all until Bevy 0.16 is released due to the above-linked PR--Windows is a bit finicky about how its accessibility tree is created and that was broken in 0.15. That's why I'm a bit uncertain about macOS--I think the 0.15 breakage is specific to Windows but I don't have a mac at hand to test. I know there are accessible testing tools that let you introspect the accessibility tree so you can see the structure without having to run a screen reader. I don't remember what those are right now but I'll look tomorrow. Thanks for all the feedback. |
|
I had to push some changes due to merge conflicts and also fixed some little things around system configs and made them public. |
|
I've merged the PR, but I'll have to disable the functionality for some time unfortunately, as there's an |
|
Understood, getting the various bevy/egui/accesskit/winit versions to line up on their respective release cycles has definitely been a pain point. Thanks for at least merging it--good to not be maintaining this PR on my own. :) |
This integrates Bevy's AccessKit setup with egui's AccessKit integration so egui UIs are accessible.
I haven't been able to test this on Linux, where it may not work since accesskit integration is feature-gated there. In theory it should just pull in dependencies and more or less be a no-op since an access technology will never request accessibility, but I can't say for sure and would appreciate help testing.
Thanks!