-
Notifications
You must be signed in to change notification settings - Fork 89
feat!: Multiple tree support #655
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
Conversation
|
CC @delan, @lukewarlow It would be amazing if you were able to give this a try in Servo. I have only performed limited testing to ensure nodes from subtrees would correctly be exposed on Windows, macOS and Linux. Bigger and more complex trees would be needed to validate this. |
3dd4748 to
783c1e8
Compare
|
thanks @DataTriny! we’ll need a bit of time to write a minimal accessibility tree builder in Servo, since there is no support for that yet, but we’ll try to test this patch as soon as we can. will update next week :) |
|
update: working on it in servo/servo#41173. the combined tree shows up in elevado, but not in accerciser or orca just yet, but that’s almost certainly a bug on our end. |
|
We've been testing this with an initial prototype in Servo. We take the DOM and layout information and construct an approximation of an accessibility tree, sending that via IPC to the servoshell application where its forwarded to AccessKit as a tree update. We currently have a minimal patch applied to egui to support this, but this should be straighforward to upstream once the subtree API lands in an AccessKit release. Overall, we don’t see any major concerns with the API as it stands, and everything we've tested so far is working well. We can’t say much about performance yet because the current tree builder in Servo is still very naïve, and similarly we can’t properly test focus handling since Servo doesn’t have a proper focus model for web contents in place at the moment. That said, the combined tree is showing up correctly in Elevado, Accerciser, and the macOS Accessibility Inspector. The root application contents (built with egui) are also still being exposed to screen readers like Orca, VoiceOver, Narrator, and NVDA. In Orca at least, doing a Say All reads through our very rough web contents too, and we’d expect to be able to achieve the same behaviour with the other screen readers as well. Actions from both subtrees are coming in via winit, and we’re able to distinguish which subtree they belong to without issue. |
|
This is quite a disruptive change internally, but I don't think it's more disruptive than it needs to be. And more importantly, the changes to the public API are quite small and clean. So this design looks good to me. @DataTriny Unless there's some part of this that you're not happy with, I'd be fine with taking it out of draft status and merging it. |
|
Thanks @lukewarlow for the feedback! @mwcampbell only two items I'd like to make sure we agree upon:
It unfortunately has to be. I tried to think of as many edge cases as possible and I've covered them with tests. Let me know if you find others. |
|
I've just rebased on main branch. @mwcampbell If you agree with the choices I made as described previously, feel free to merge this. |
|
I do think those design limitations are acceptable. Merging. |
Adds support for nested trees to the schema and all existing adapters.
Tree updates must now include the
tree_idfield which indicates to which tree this update applies. The main tree is identified byTreeId::ROOT. Subtrees have to use an UUID (preferrably version 4 to avoid collisions between trees from different sources). The common crate re-exports theUuidtype from the uuid crate, but users will have to enable extra features on the uuid crate in order to generate UUIDs.Graft nodes are nodes which host a subtree. They are defined by setting
Node::tree_idon them. Graft nodes cannot have regular children. They can point to a subtree that does not exist yet. When a graft node is removed or itstree_idproperty is cleared, its subtree is removed unless another graft node is provided as part of the same update, in which case the subtree is reparented.The keyboard focus is now determined by starting at the root of the main tree: if the tree-local focus point to a regular node then this node gets the focus, otherwise if the node is a graft then search continues into the subtree.
ActionRequestnow usestarget_nodeandtarget_treeto indicate on which node to apply an action.Internally, adapters now refer to nodes using a combination of local node ID (provided by the user in tree updates) and tree index. Tree indices are 32 bit unsigned integers that map to
TreeIds.Prior art
In the past we used to use
NonZeroU128as ID for nodes in the schema. We switched tou64as it's easier to work with in other programming languages and for serialization. However UUIDs are well established and in fact schemars has support for them so this should be less of an issue.Chromium does not allow nodes to refer to nodes in different trees so it is not possible, for instance, to indicate that a node is labelled by another node that is in a different tree. The same applies here. I don't think it is that useful in practice and it would add a lot of complexity.
Alternative to #641
This design fits into the existing schema and adapter model rather than building on top of it, no conditional compilation required to work with every adapter. It should also be more flexible around tree identification, hopefully allowing completely independant providers to expose their subtree(s) without a central registrar.