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

Implement accessibility APIs via AccessKit #2294

Merged
merged 32 commits into from
Dec 4, 2022
Merged

Conversation

mwcampbell
Copy link
Contributor

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.

crates/egui/Cargo.toml Outdated Show resolved Hide resolved
@mwcampbell
Copy link
Contributor Author

@emilk Are there any particular milestones you want this work to reach before I take the PR out of draft status?

@mwcampbell
Copy link
Contributor Author

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.

@emilk
Copy link
Owner

emilk commented Nov 14, 2022

@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

@mwcampbell
Copy link
Contributor Author

I just modified accesskit_winit so it can be used without requiring an EventLoopProxy and a specific user event type. So I should be able to refactor my changes to eframe to get back to requiring only RequestRepaintEvent. The downside of such a change is that I'll have to put either egui_winit::State or its egui_input field behind a mutex, since an AccessKit ActionHandler implementation may be called from any thread. That might still be preferrable to the changes that I currently have to make to plumb the user event type that accesskit_winit used to require.

@emilk
Copy link
Owner

emilk commented Nov 14, 2022

I just modified accesskit_winit so it can be used without requiring an EventLoopProxy and a specific user event type. So I should be able to refactor my changes to eframe to get back to requiring only RequestRepaintEvent

Even if you do, I think a pub enum UserEvent { RequestRepaint } still makes sense.

@mwcampbell
Copy link
Contributor Author

I decided that putting the egui_winit state, or even just the egui_input field of that state, behind a mutex was just as invasive a change as the previous change I made to the user event type, so I decided not to bother with that. However, I did make AccessKit an optional feature in egui-winit, which allowed me to revert my previous changes to egui_glium and egui_glow. So now, only eframe is opinionated about using AccessKit.

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.
Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Owner

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?

Copy link
Contributor Author

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?

emilk added a commit that referenced this pull request Nov 16, 2022
Preparation for #2294
to make that a smaller diff.
Copy link
Owner

@emilk emilk left a 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.

emilk added a commit that referenced this pull request Nov 16, 2022
Preparation for #2294
to make that a smaller diff.
@mwcampbell
Copy link
Contributor Author

@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.

@emilk
Copy link
Owner

emilk commented Nov 16, 2022

@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!

@mwcampbell
Copy link
Contributor Author

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.

@mwcampbell
Copy link
Contributor Author

@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.

@mwcampbell
Copy link
Contributor Author

I'm reconsidering my previous changes to DragValue. I now believe that automatically switching into edit mode when the widget receives keyboard focus is the correct behavior. But it means that because the button itself can never receive keyboard focus, the current code for incrementing and decrementing the value with the keyboard doesn't work. I'm going to address that in a separate issue and PR, then rebase this branch.

@mwcampbell
Copy link
Contributor Author

This is now blocked on #2342.

@mwcampbell
Copy link
Contributor Author

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 egui::Context::request_repaint to request a repaint. In theory, a screen reader could decide how to read a window that has just appeared or received focus based on the (lack of) content in the placeholder tree. But so far, this isn't happening on either Windows or macOS. And there's precedent; Chromium's accessibility implementation has to return a placeholder tree for the web content area at first, then get a real one from the renderer process. I think lazy initialization is critical for wide acceptance of AccessKit, so I'm going to go with this compromise.

@mwcampbell mwcampbell marked this pull request as ready for review November 30, 2022 21:00
emilk added a commit that referenced this pull request Dec 2, 2022
Preparation for #2294
to make that a smaller diff.
Copy link
Owner

@emilk emilk left a 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!

crates/egui/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +403 to +404
#[cfg(feature = "accesskit")]
accesskit_update: _, // not currently implemented
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "accesskit")]
accesskit_update: _, // not currently implemented
#[cfg(feature = "accesskit")]
accesskit_update: _, // not currently implemented

Copy link
Contributor Author

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.

crates/egui/src/context.rs Outdated Show resolved Hide resolved
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.
Copy link
Owner

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?

crates/egui/src/widgets/drag_value.rs Outdated Show resolved Hide resolved
crates/egui/src/response.rs Outdated Show resolved Hide resolved
examples/hello_world/src/main.rs Outdated Show resolved Hide resolved
@mwcampbell
Copy link
Contributor Author

@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?

@emilk emilk merged commit e1f348e into emilk:master Dec 4, 2022
@emilk
Copy link
Owner

emilk commented Dec 4, 2022

@mwcampbell merged! 🍾 🥳

Amazing work!

Yes, I'm try to find time to publish egui 0.20 this week!

@mwcampbell mwcampbell deleted the accesskit branch December 4, 2022 18:30
JohannesProgrammiert pushed a commit to JohannesProgrammiert/egui that referenced this pull request Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility (A11y)
2 participants