Initial AccessKit support #2865

tronical · 2023-06-12T14:19:39Z

This change adds initial accessibility support for the winit backend
through use of AccessKit.

helper_crates/vtable/src/vrc.rs

ogoffart · 2023-06-12T14:54:43Z

internal/backends/winit/Cargo.toml

+accesskit = { version = "0.11.0" }
+accesskit_winit = { version = "0.14.0" }


Should we make it an optional feature?

In my opinion it should not be optional. Afaics it comes at no runtime cost until it is used. It does come at a compile time cost though. But by the time an end-user discovers that the application is not accessible it is IMO too late.

Another option: we could add it do the default features, so it's possible to opt out of it. How about that?

I'd have this as a default feature: That way users can opt out when needed.

Because of how heavy accessibility support is on Linux, a problem for which I haven't yet found a good solution, having a way to opt out is acceptable to me. I'm glad it's enabled by default.

internal/backends/winit/winitwindowadapter/accesskit.rs

internal/backends/winit/accesskit.rs

tronical · 2023-06-13T12:08:28Z

Looks like the CI tests the merge :(. That means I have to rebase this - sorry, that makes incremental review harder.

mwcampbell · 2023-06-13T12:56:42Z

Since we rebuild the accesskit node tree on any Slint item tree changes, we might as well just use the index in our vector of property trackers as node id.

I'm not sure I understand the implications of this. It's important that the same UI element (button, checkbox, list item, etc.) have the same node ID across tree updates, regardless of what elements may have been added or removed. I'm sorry if that wasn't clear already. Does your current solution mean that the same element might have different node IDs at different times?

tronical · 2023-06-13T13:01:35Z

Since we rebuild the accesskit node tree on any Slint item tree changes, we might as well just use the index in our vector of property trackers as node id.

I'm not sure I understand the implications of this. It's important that the same UI element (button, checkbox, list item, etc.) have the same node ID across tree updates, regardless of what elements may have been added or removed. I'm sorry if that wasn't clear already. Does your current solution mean that the same element might have different node IDs at different times?

For simple state changes (checkbox ticked or unticked) remains the same. But when say an item is added to a list view then we end up re-creating the entire tree.

What's the implication of replacing the entire tree and introducing new IDs? Or differently asked: How can I test the implication? :)

mwcampbell · 2023-06-13T13:07:44Z

When you add or remove items in a list view, I think it's not so bad for other items in the list view to have different node IDs. But if it means that items outside the list view get different node IDs as a side effect, then depending on the details of the platform accessibility API and the assistive technology being used, the result could be disorienting or at least distracting for the user. For example, if some other widget is focused when you add or remove items in a list, then the assistive technology (e.g. screen reader) will think that the focus changed when it actually didn't, and the user will get an unnecessary re-announcement of focus. I honestly don't know if there are worse side effects in practice; I just know it's important for widgets to have stable node IDs.

mwcampbell · 2023-06-13T13:37:53Z

The worst effect of changing an existing item's node ID that I can now think of is this: If a screen reader's cursor is on an item, but the keyboard focus is not, and the item's node ID changes, then the screen reader loses its place in the UI.

tronical · 2023-06-13T14:09:57Z

Yeah, I can imagine that if the focus is erratically changed that would be bad.

Suppose there's an "Add" button that adds items to a list and that it's okay for the user to press this button many times. So when the button is focused, and then pressed, an item is added to the list, but the button should remain focused.

What would happen right now with Slint is that when the button is pressed, the assistive tech would announce an unnecessary change of focus to a button that happens to be in the same location, with the same text, and the same (expected) state).

I tried this on the todo example and it "works". It isn't ideal, but it seems not too bad either.

mwcampbell · 2023-06-13T14:15:05Z

Yes, I agree that re-announcing focus is an acceptable bug, at least for now.

api/rs/slint/Cargo.toml

ogoffart · 2023-06-13T14:57:00Z

internal/backends/winit/accesskit.rs

+/// The AccessKit adapter tries to keep the given window adapter's item tree in sync with accesskit's node tree.
+///
+/// The entire item tree is mapped to accesskit's node tree. Any changes to an individual accessible item results
+/// in an access kit tree update with just changed nodes. Any changes in the tree structure result in a complete
+/// tree rebuild. This could be implemented more efficiently, but it requires encoding the item index, raw vrc pointer,
+/// and tree generation into the nodeid.
+///
+/// For unix it's necessary to inform accesskit about any changes to the position or size of the window, hence
+/// the `on_event` function that needs calling.
+///
+/// Similarly, when the window adapter is informed about a focus change, handle_focus_change must be called.
+/// Finally, when a component is destroyed, `unregister_component` must be called, which rebuilds the entire
+/// tree at the moment.


nice documentation! 👍

internal/backends/winit/accesskit.rs

mwcampbell · 2023-06-13T15:37:16Z

Unfortunately, focus handling is broken.

TreeUpdate::focus must reflect both whether the window itself is focused, and the focus within that window if any. Specifically:

If the window is not focused, focus must be None regardless of what item within the window might have focus.
If the window is focused but no item within the window has focus, then as far as AccessKit is concerned, the focus is on the window itself, and focus must be set to the root window node.
If the window is focused and an item in the window has focus, then focus must be set to that item's node ID.

The focus field of every tree update must follow these rules, regardless of whether the focus has changed since the previous tree update. A tree update with the latest value of focus must be fired when the window gains or loses focus, as well as when focus within the window changes.

I'm sorry I apparently didn't document this clearly enough. This is all implemented correctly in the example for the AccessKit winit adapter.

tronical · 2023-06-13T15:42:10Z

My bad. You’re right about the focus handling. I meant to implement it the way you describe. Will do :)

mwcampbell · 2023-06-14T11:17:59Z

With the latest commit, I get the following panic when running the todo example on Windows with a screen reader active:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', C:\Users\mattc\.cargo\registry\src\github.com-1ecc6299db9ec823\accesskit_consumer-0.15.0\src\tree.rs:297:55

I think you might have just uncovered a bug in AccessKit.

mwcampbell · 2023-06-14T12:03:27Z

What I've figured out so far is that the panic is occurring because, in a single tree update, a node is changed, then removed. Clearly AccessKit should be able to handle that without panicking. What I don't know yet is if the removal of the node is also a bug in AccessKit.

mwcampbell · 2023-06-14T13:27:05Z

While I believe I've found a couple of bugs in AccessKit, I also noticed some behavior in slint which seems problematic. I see that it's possible for register_component to be called, possibly several times, while the initial tree is being built. Clearing all_nodes and incrementing tree_generation while building the tree leads to a tree with nodes from multiple generations. I'm guessing this wasn't intended and might cause problems.

tronical · 2023-06-14T13:42:50Z

it's possible for register_component to be called, possibly several times, while the initial tree is being built

Ouch, indeed, our lazy creation of components bites us here. That would explain the problem you're seeing (and odd that I didn't see it with VoiceOver).

I'll add a guard for that.

mwcampbell · 2023-06-14T13:48:53Z

Or what if register_component and unregister_component didn't clear all_nodes and increment tree_generation until they were doing the actual update inside the timer callback? I just tried making that change locally, and it seems to work.

tronical · 2023-06-14T13:56:51Z

That's also a good idea. Initially I hadn't done that because the NodeId contained the tree generation and the moment we know the tree structure changed, we must bump the generation to invalidate any node id that we might see for example due to an incoming action request. But now that the node id encoding is simpler, what you propose makes indeed more sense :)

mwcampbell · 2023-06-14T14:01:02Z

Well, the node ID still contains the tree generation. Did you intend to remove that?

mwcampbell · 2023-06-14T14:06:33Z

But even with the node ID containing the tree generation as it does now, I don't see why you need to invalidate all node IDs immediately, before the timer runs. Consider this scenario:

register_component or unregister_component runs. It doesn't touch all_nodes or tree_generation; it only schedules the timer.
An action request is received with a node ID from the previous tree generation.
The previous tree generation is still in tree_generation, and the previous node is still in all_nodes. But note that all_nodes contains ItemWeak (indirectly), not ItemRc. So if the item referenced by the previous-generation node is still valid, the action can be performed. But if not, then item_rc_for_node_id will return None.

So as far as I can tell, the necessary invalidation is done anyway, without requiring register_component and unregister_component to immediately clear all_nodes and increment tree_generation.

tronical · 2023-06-14T14:10:20Z

I pushed a fix that implements this now. We'll still end up doing a couple of unnecessary tree rebuilds on startup. But maybe that's not too bad.

The todo example is pretty bad in terms of keyboard navigation - the popup on close has no keyboard focus and there's also no correct initial focus. I'll try to fix that separate from this PR.

tronical · 2023-06-14T14:11:58Z

But even with the node ID containing the tree generation as it does now, I don't see why you need to invalidate all node IDs immediately, before the timer runs. Consider this scenario:

register_component or unregister_component runs. It doesn't touch all_nodes or tree_generation; it only schedules the timer.

An action request is received with a node ID from the previous tree generation.

The previous tree generation is still in tree_generation, and the previous node is still in all_nodes. But note that all_nodes contains ItemWeak (indirectly), not ItemRc. So if the item referenced by the previous-generation node is still valid, the action can be performed. But if not, then item_rc_for_node_id will return None.

So as far as I can tell, the necessary invalidation is done anyway, without requiring register_component and unregister_component to immediately clear all_nodes and increment tree_generation.

Yep, your analysis is entirely correct :)

tronical · 2023-06-14T14:22:18Z

Well, the node ID still contains the tree generation. Did you intend to remove that?

No, I think it's still needed to ensure that the index in all_node that's also in NodeId is valid. Of course we could just do an out-of-bounds check when accessing it, but I'd rather not use it at all instead of performing an action on the wrong item (for example).

mwcampbell · 2023-06-14T14:27:19Z

OK, that last commit did fix the problem I was seeing on Windows.

But now I'm seeing something else that concerns me: The todo example seems to be triggering full-tree rebuilds much more often than it does. For example, toggling the checkbox of one of the todo-list items increments the tree generation, even though, as far as I can tell, doing this with the "hide done items" checkbox unchecked doesn't cause any items to be added or removed. More strangely, even just pressing Alt+F4 (which should close the window, but doesn't) causes the tree generation to be incremented.

mwcampbell · 2023-06-14T14:29:34Z

No, I think it's still needed to ensure that the index in all_node that's also in NodeId is valid. Of course we could just do an out-of-bounds check when accessing it, but I'd rather not use it at all instead of performing an action on the wrong item (for example).

That makes sense. In that case, we just need to make sure that the tree generation isn't incremented more often than is necessary.

tronical · 2023-06-14T14:42:15Z

OK, that last commit did fix the problem I was seeing on Windows.

But now I'm seeing something else that concerns me: The todo example seems to be triggering full-tree rebuilds much more often than it does. For example, toggling the checkbox of one of the todo-list items increments the tree generation, even though, as far as I can tell, doing this with the "hide done items" checkbox unchecked doesn't cause any items to be added or removed. More strangely, even just pressing Alt+F4 (which should close the window, but doesn't) causes the tree generation to be incremented.

Yeah, any conditional element for example would cause this. For example a checkbox may look like this:

    ...
    if (self.checked): Path { render checkbox mark as path }
    ...

And so toggling that state will trigger register_component and unregister_component.

mwcampbell · 2023-06-14T14:56:07Z

Invalidating all node IDs this often, when merely toggling a checkbox or even doing something that doesn't make any meaningful change to the UI at all (like my Alt+F4 example), is unacceptable. I'll see if I can come up with a way to give each item a node ID that is guaranteed to be unique and stable for the life of that item.

internal/backends/winit/winitwindowadapter.rs

tronical · 2023-06-14T15:12:45Z

Perhaps the simplest was would be to use a slab instead of a vector for the nodes. Then, when register_component as well as unregister_component is called, we trigger a timer-driven update. In that update, we first go through all existing nodes we reported earlier, try to upgrade the ItemWeak and if it's None, we delete the entry from the slab and must remember include the parent node in the TreeUpdate with update children. Then we walk Slint's item tree and we need to determine which items are new, so that we can create new nodes for them and update the parent to include them.

Finding new items could perhaps be done on an per-parent basis: We look at the children (accessible descendants) and for each we try to see if it's a known one by looking for it in the existing children: Vec<NodeId> that we've kept. It's quadratic, but just for the children per item. The search could be done by taking each nodeid, looking up in the slab, fetch the ItemWeak and compare it against the current ItemWeak. If it's the same, nothing is to be done. If it's new, then we build a new access kit node and allocate the id via the slab.

mwcampbell · 2023-06-14T15:29:31Z

That solution, using a slab, sounds good to me. I'll be happy to review it when it's ready.

mwcampbell · 2023-06-14T16:00:32Z

Actually I have an idea that I think will be simpler. Please give me some time to implement it and see if it works. I just accepted the CLA, so I can provide a patch.

mwcampbell · 2023-06-14T18:10:12Z

On the issue of stable node IDs, please look at my branch.

Basically, a node ID now consists of a component ID and the item index within the component. Component IDs are monotonically increasing. There are two hash maps, one from component pointers to component IDs, and another from component IDs to component weak references. A component ID is lazily assigned when a node is created for an item under that component. When a component is unregistered, its hash map entries (if any) are removed.

all_nodes is still a Vec, and the whole tree is still rebuilt whenever the structure changes. Perhaps these things can be made more efficient, but AccessKit can deal with updating nodes that haven't actually changed (even the whole tree), as long as node IDs are stable.

tronical · 2023-06-14T19:52:23Z

Nice idea!

Would you prefer creating a PR for this branch or shall I merge this PR and then you create a PR for the master branch?

mwcampbell · 2023-06-14T20:57:33Z

OK, now this looks good to me, bearing in mind that it's only the beginning of accessibility support.

mwcampbell · 2023-06-14T21:12:05Z

Wait, I found a bug: trying to handle an action request panics on Windows, because the AccessKit Windows adapter (usually) sends action requests from a non-main thread. To reproduce, run an example with the Narrator screen reader on Windows, and use Caps Lock+Left Arrow or Caps Lock+Right Arrow to move the Narrator cursor between items. By default, Narrator will sync the system focus with the Narrator cursor, so as soon as you reach a focusable item, you'll get the panic.

tronical · 2023-06-15T07:22:01Z

Good catch! I was mistakenly assuming that a send wrapper can always be cloned, even if the thread it wasn't created in. I'll push a fix (once I found one ;)

tronical · 2023-06-15T07:40:45Z

Alright, that works for me now. I noticed one interesting bug with Narrator: When I move the slint example window, the narrator's blue focus remains at the original position. It's almost as if something like set_root_window_bounds is missing for the Windows adapter.

mwcampbell · 2023-06-15T10:27:59Z

It's common for the Narrator highlight cursor to get out of sync when a window is moved; it doesn't only happen with AccessKit. I wouldn't worry about that.

tronical · 2023-06-15T10:41:10Z

Ah that’s good to know :). Alright, I’ll go ahead the merge this then (will squash some of my fix up commits).

mwcampbell · 2023-06-15T10:44:42Z

Yup, I don't see any more critical, i.e. fundamental, issues. Of course, there's more work to do to implement complete accessibility support, but this is a good start.

This change adds initial accessibility support for the winit backend through use of AccessKit.

The action requests on Windows typically come in from a non-main thread, so we'd try to clone the send wrapper in `do_action` in a non-main thread. That panics unfortunately - send-wrapper requires the clone to be done in the original thread. To work around this, wrap the send-wrapper in an Arc, which we can safely clone. Then inside the closure invoked from within the main thread, we can clone the send wrapper safely and then take out the Weak from it (safely as well).

CLAassistant · 2023-06-15T11:20:11Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ tronical
❌ mwcampbell
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

tronical · 2023-06-15T11:21:31Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ tronical
❌ mwcampbell
You have signed the CLA already but the status is still pending? Let us recheck it.

For the record of this, @mwcampbell signed the CLA version 1.1 before the change to CLA 2.0 today. So the bot's message here is safe to ignore.

tronical force-pushed the simon/accessibility branch from 0a7c6f0 to 59dfa0c Compare June 12, 2023 14:20

ogoffart reviewed Jun 12, 2023

View reviewed changes

hunger reviewed Jun 13, 2023

View reviewed changes

internal/backends/winit/accesskit.rs Outdated Show resolved Hide resolved

tronical force-pushed the simon/accessibility branch from f69cac2 to d4f61f9 Compare June 13, 2023 12:11

This was referenced Jun 13, 2023

Accessibility: support for screen readers #32

Closed

Accessibility: Avoid rebuilding the node tree when the Slint element tree structure changes #2873

Closed

ogoffart reviewed Jun 13, 2023

View reviewed changes

api/rs/slint/Cargo.toml Outdated Show resolved Hide resolved

ogoffart reviewed Jun 13, 2023

View reviewed changes

ogoffart approved these changes Jun 13, 2023

View reviewed changes

ogoffart approved these changes Jun 14, 2023

View reviewed changes

internal/backends/winit/winitwindowadapter.rs Outdated Show resolved Hide resolved

mwcampbell mentioned this pull request Jun 14, 2023

New scheme for stable node IDs #2880

Merged

tronical and others added 3 commits June 15, 2023 13:07

Initial AccessKit support

df43e4b

This change adds initial accessibility support for the winit backend through use of AccessKit.

New scheme for stable node IDs

ba1b2c0

tronical force-pushed the simon/accessibility branch from 4d2565e to 77c8240 Compare June 15, 2023 11:20

tronical merged commit 5efbbef into master Jun 15, 2023
12 of 13 checks passed

tronical deleted the simon/accessibility branch June 15, 2023 11:20

tronical linked an issue Jun 15, 2023 that may be closed by this pull request

Accessibility: support for screen readers #32

Closed

		accesskit = { version = "0.11.0" }
		accesskit_winit = { version = "0.14.0" }

Initial AccessKit support #2865

Initial AccessKit support #2865

Conversation

tronical commented Jun 12, 2023 • edited

ogoffart Jun 12, 2023

Choose a reason for hiding this comment

tronical Jun 12, 2023

Choose a reason for hiding this comment

tronical Jun 12, 2023

Choose a reason for hiding this comment

hunger Jun 13, 2023

Choose a reason for hiding this comment

mwcampbell Jun 13, 2023

Choose a reason for hiding this comment

tronical commented Jun 13, 2023

mwcampbell commented Jun 13, 2023

tronical commented Jun 13, 2023

mwcampbell commented Jun 13, 2023

mwcampbell commented Jun 13, 2023

tronical commented Jun 13, 2023

mwcampbell commented Jun 13, 2023

ogoffart Jun 13, 2023

Choose a reason for hiding this comment

mwcampbell commented Jun 13, 2023

tronical commented Jun 13, 2023

mwcampbell commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

tronical commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

tronical commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

tronical commented Jun 14, 2023

tronical commented Jun 14, 2023

tronical commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

tronical commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

tronical commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

tronical commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

mwcampbell commented Jun 14, 2023

tronical commented Jun 15, 2023

tronical commented Jun 15, 2023

mwcampbell commented Jun 15, 2023

tronical commented Jun 15, 2023

mwcampbell commented Jun 15, 2023

CLAassistant commented Jun 15, 2023 • edited

tronical commented Jun 15, 2023

tronical commented Jun 12, 2023 •

edited

CLAassistant commented Jun 15, 2023 •

edited