-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implement accessibility APIs via AccessKit #2294
Conversation
a2a6ce8
to
b0f2ae5
Compare
@emilk Are there any particular milestones you want this work to reach before I take the PR out of draft status? |
I wonder if I should make AccessKit an optional feature in egui-winit as it is in egui. To do that, I may need to refactor my changes to egui-winit, to initialize the AccessKit winit adapter separately. But by doing this, only eframe would be opinionated about whether to use AccessKit, and I suppose I'd be able to revert my modifications to egui_glium and egui_glow. |
@mwcampbell I'll take a proper look as soon as I find time. I have a very busy week, but I'll do my best to squeeze this in! And thanks so much for working on this 🙏 My general sense is that I would prefer it all to be optional in every crate, but I don't yet have a sense of how much work that is. But this is all pulling in a lot more dependencies: https://github.com/emilk/egui/pull/2294/files#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87e |
I just modified accesskit_winit so it can be used without requiring an |
Even if you do, I think a |
I decided that putting the egui_winit state, or even just the I could make AccessKit an optional feature even in eframe, though enabled by default. On the one hand, I'm apprehensive about making it too easy to disable accessibility; I wouldn't want to make it easy for a developer to disable accessibility just because they're idly reducing the size of their executable or trying to improve hypothetical performance, leading to a situation where a disabled person can't use an application they need for their job. On the other hand, I don't want to be so dogmatic about forcing accessibility onto everyone that it leads to a backlash. |
let clicked_elsewhere = response.clicked_elsewhere(); | ||
let ctx_impl = &mut *self.write(); | ||
let memory = &mut ctx_impl.memory; | ||
let input = &mut ctx_impl.input; | ||
|
||
// We only want to focus labels if the screen reader is on. |
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.
I think we should keep this behavior. Tab to select the next button is what I ususally want from a GUI. Selecting labels is only interesting if the screen reader is on
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.
Please see the corresponding change I made in the label widget. Short version: the label widget now adjusts its sense setting based on whether the built-in screen reader is on.
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 I see. Why did you opt to handle that behavior in Label
rather than here?
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.
Since the label widget is the one special case, I thought it made sense to encapsulate the special logic in that widget. And this way, the AccessKit integration code can just look at sense.focus
without having to know about the label special case. Does that make sense?
Preparation for #2294 to make that a smaller diff.
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 is looking really nice!
I would still like for accesskit
to be an optional (but perhaps default) feature of eframe
. Seems like it should be fairly easy.
I made another PR, introducing UserEvent
, to reduce the diff of this PR and to lessen the chance for merge conflicts for others working in eframe
in parallel.
Preparation for #2294 to make that a smaller diff.
4112f17
to
74856e9
Compare
@emilk I rebased on your recent PR, and now AccessKit is an optional feature even in eframe. You seem unsure about whether it should be enabled by default in eframe. What are your concerns about enabling it by default? To explain why I think accessibility should be the default at least in the highest-level framework, I'll share an anecdote: My best friend, who is totally blind, bought a piece of hardware that had a companion app to set it up. He couldn't independently use that app, because it was Qt-based, and it was shipped without Qt's optional accessibility support. So apparently the first person to find out that accessibility was missing was the end-user, and you can imagine the layers such a bug report would have to go through before the app developer would even look at it, let alone ship a fix. Now imagine that he had to use the app to do his job. That's why I think that requiring application developers to enable accessibility is a mistake. I can appreciate the reasons for disabling it by default in egui and even egui-winit; properly supporting accessibility in a custom egui integration requires some extra work. But in eframe, it should just work by default. |
@mwcampbell my only concern is build times and other potential build problems from extra dependencies, especially since AccessKit is currently only working on Windows, yet is pulled in on all platforms. But, you make a very good point about it being enabled by default, so let's keep it like that! |
I'm eager to minimize all possible barriers to implementing accessibility, so I'm willing to do work to minimize AccessKit's impact on build time, but not necessarily immediately. For now, I need to focus on getting it working at all, across platforms. BTW, I'm currently working on a macOS implementation, but don't yet have anything usable to show. |
@emilk I need to write changelog entries for this PR. To acknowledge the funding that Google has provided for both this integration work and the development of AccessKit itself, I propose including something like the following in the entry in the main egui changelog: Special thanks to the Google Fonts team for funding both this integration and the development of AccessKit itself. Is this OK? I already cleared it with my main contact at Google. |
2d97639
to
bb8c0ba
Compare
I'm reconsidering my previous changes to |
This is now blocked on #2342. |
…s and an important fix for navigation order on macOS
ce8c94d
to
6dd2118
Compare
I've decided it's time to take this PR out of draft status. There are, of course, more widget types to support, and more things to polish. But I believe the foundation is now solid, and for the sake of keeping the scope of a single PR manageable, I won't do any more work on this one, except for review feedback. I've done some more refactoring. Most notably, AccessKit support is now lazy. That is, an egui context now only starts generating AccessKit tress once the integration enables AccessKit support in that context. This way, the integration can defer AccessKit initialization until there is actually a request for accessibility info, e.g. from a screen reader. So, even when the AccessKit feature is enabled, the performance impact for users that don't need accessibility is negligible. The downside, at least for egui-winit and eframe, is that the integration has to return a placeholder tree at first, then push a real one on the next repaint. The relevant code in eframe uses |
Preparation for #2294 to make that a smaller diff.
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 looks amazing - I only have a few small nits - fix those and I'll merge!
#[cfg(feature = "accesskit")] | ||
accesskit_update: _, // not currently implemented |
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.
#[cfg(feature = "accesskit")] | |
accesskit_update: _, // not currently implemented | |
#[cfg(feature = "accesskit")] | |
accesskit_update: _, // not currently implemented |
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.
The current spacing is what rustfmt wants. If I change it, rustfmt just puts it back.
let clicked_elsewhere = response.clicked_elsewhere(); | ||
let ctx_impl = &mut *self.write(); | ||
let memory = &mut ctx_impl.memory; | ||
let input = &mut ctx_impl.input; | ||
|
||
// We only want to focus labels if the screen reader is on. |
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 I see. Why did you opt to handle that behavior in Label
rather than here?
Co-authored-by: Emil Ernerfeldt <[email protected]>
Co-authored-by: Emil Ernerfeldt <[email protected]>
@emilk I think that covers everything. Thanks for taking time to review. Looking forward to getting this merged. Also, do you think it would be a good idea to publish egui 0.20 when this is merged? |
@mwcampbell merged! 🍾 🥳 Amazing work! Yes, I'm try to find time to publish |
Preparation for emilk#2294 to make that a smaller diff.
This PR makes egui accessible with assistive technologies such as screen readers. In contrast with egui's existing built-in screen reader, this PR uses AccessKit to implement platform accessibility APIs that are used by existing screen readers and other assistive technologies.
AccessKit is only implemented for Windows so far. I will focus on a Mac implementation over the next week or two.
We should discuss what level of completeness this branch needs to reach before being merged upstream. I haven't yet implemented support for all egui widgets. In general, this branch supports the widgets that were already accessible via
WidgetInfo
.Closes #167.