AccessKit integration #78

matthunz · 2024-06-20T21:36:35Z

Starts work on AccessKit integration (#14)

Connect accesskit-winit adapter
Add Dom::visit to traverse the dom top-down
Build initial AccessKit tree
Send tree changes from Dioxus updates to adapter

I think it's best to wait for more interactivity and focus system design before processing AccessKit events with the DOM 🤔 totally open to trying to add that here though

…ccesskit

nicoburns · 2024-06-23T09:15:41Z

@matthunz Have you tried actually using this with a screenreader?

nicoburns · 2024-06-23T08:54:23Z

packages/dioxus-blitz/src/window.rs

+                #[cfg(feature = "accesskit")]
+                adapter: accesskit_winit::Adapter::with_event_loop_proxy(&window, proxy.clone()),
+                #[cfg(feature = "accesskit")]
+                node_ids: Default::default(),
+                #[cfg(feature = "accesskit")]
+                id_to_node_ids: Default::default(),
+                #[cfg(feature = "accesskit")]


Perhaps these fields ought to be grouped together in a struct?

For sure, I like this a lot, those cfg flags are super ugly

blitz/packages/dioxus-blitz/src/accessibility.rs

Lines 7 to 13 in 0ce74dd

pub struct AccessibilityState {

/// Adapter to connect to the [`EventLoop`](`winit::event_loop::EventLoop`).

adapter: accesskit_winit::Adapter,

/// Next ID to assign an an [`accesskit::Node`].

next_id: u64,

}

nicoburns · 2024-06-23T08:56:11Z

packages/dioxus-blitz/src/window.rs

+    #[cfg(feature = "accesskit")]
+    node_ids: std::collections::HashMap<accesskit::NodeId, usize>,
+
+    #[cfg(feature = "accesskit")]


Perhaps the "accesskit id" ought to be a field on the node itself? Every node will have one, right?

This makes sense to me but would we need to move accesskit to Blitz core for that? For now I removed the field until we have support for accessibility events (that doesn't seem clear to me to implement yet unless you have any ideas 🤔 )

but would we need to move accesskit to Blitz core for that?

I don't think so? Node IDs are just usize atm, and are accessible from outside of the crate. We'll might want to switch to a newtype at some point, but I'm sure we'll make conversions to plain types available.

nicoburns · 2024-06-23T08:56:59Z

packages/dioxus-blitz/src/window.rs

+    #[cfg(feature = "accesskit")]
+    id_to_node_ids: std::collections::HashMap<usize, accesskit::NodeId>,
+
+    #[cfg(feature = "accesskit")]


I think all of these new fields ought to have comments documenting what they are/do.

nicoburns · 2024-06-23T08:58:46Z

packages/dioxus-blitz/src/window.rs

+            let name = element_data.name.local.to_string();
+            let role = match &*name {
+                "button" => accesskit::Role::Button,
+                "div" | "section" => accesskit::Role::Group,


AccessKit docs suggest that div ought to be Role::GenericContainer (https://docs.rs/accesskit/latest/accesskit/enum.Role.html#variant.GenericContainer)

Looks like there's a bunch that could easily be plugged in here (e.g. h1, h2 should have Heading role). Happy to defer that until later, but could we at least get a TODO in here?

Thanks for the tips 👍 I added those and some others I could think of here so far

blitz/packages/dioxus-blitz/src/accessibility.rs

Lines 62 to 78 in 0ce74dd

// TODO match more roles

let role = match &*name {

"button" => Role::Button,

"div" => Role::GenericContainer,

"header" => Role::Header,

"h1" | "h2" | "h3" | "h4" | "h5" | "h6" => Role::Heading,

"p" => Role::Paragraph,

"section" => Role::Section,

"input" => {

let ty = element_data.attr(local_name!("type")).unwrap_or("text");

match ty {

"number" => Role::NumberInput,

_ => Role::TextInput,

}

}

_ => Role::Unknown,

};

nicoburns · 2024-06-23T09:02:03Z

packages/dioxus-blitz/src/window.rs

@@ -308,6 +398,47 @@ impl<'a, Doc: DocumentLike> View<'a, Doc> {
        self.waker = None;
        self.renderer.suspend();
    }
+
+    #[cfg(feature = "accesskit")]
+    fn build_accessibility_tree(&mut self) {


Can we move this function (and probably build_node above too) into their own accesskit module?

nicoburns · 2024-06-23T09:06:07Z

packages/dom/src/document.rs

@@ -87,6 +92,8 @@ pub struct Document {
    pub(crate) layout_ctx: parley::LayoutContext<TextBrush>,

    pub(crate) hover_node_id: Option<usize>,
+
+    pub changed: HashSet<usize>,


Why is this a HashSet rather than a Vec? It never seems to be randomly queried, only ever iterated over?

My plan was to eventually have this be used for the fine-grained accessibility tree updates but I don't see clear path for that yet 🤔 we could totally switch this to a bool for now if we want to stick with the coarse updates for now.

Personally I think we can get away with coarse-grained for a little while before apps in Blitz start getting overly complex

Ah yes, HashSet for deduplicating insertions. Makes sense.

nicoburns · 2024-06-23T09:07:05Z

packages/dioxus-blitz/src/window.rs

+                        // TODO send fine grained accessibility tree updates.
+                        let changed = mem::take(&mut self.renderer.dom.as_mut().changed);
+                        if !changed.is_empty() {
+                            self.build_accessibility_tree()
+                        }


Seems like it wouldn't be too hard to send a TreeUpdate with just changed nodes here?

I think you're right this is how we should handle this, I was just having some trouble getting the hierarchy working. The issue I had was creating IDs for the accesskit elements so maybe re-using the Blitz element IDs here like you mentioned could help?

nicoburns · 2024-06-23T09:09:12Z

packages/dioxus-blitz/src/window.rs

+            node_builder.set_name(node.text_content());
+        }
+
+        let id = accesskit::NodeId(self.next_id);


Oh... we get to choose the AccessKit ids... Is there any reason not to use the dom's NodeId directly?

If we are able to map 1-1 from nodes (and text nodes) I'd say we should totally do this 👀

nicoburns · 2024-06-23T09:12:06Z

packages/dioxus-blitz/src/window.rs

+            accesskit_winit::WindowEvent::ActionRequested(_req) => {
+                // TODO
+            }


Might it be worth at least implementing "click" here (which of course is the only event we support generally. I think the code could just be copied from the regular click handler (skipping the bit which determines which node to generate the event for as we should be given a node id directly)

nicoburns · 2024-06-23T09:14:53Z

Cargo.toml

-euclid = { version = "0.22", features = ["serde"] }
-# mozbuild = "0.1.0"
-
-[dev-dependencies]


What's the reason for moving all of these deps from "dev-dependencies" to "dependencies"?

matthunz · 2024-06-29T19:05:41Z

@matthunz Have you tried actually using this with a screenreader?

Yeah it looks to be reading the elements OK so far, I posted this video in the Dioxus discord a little while back if you missed it

dioxus2.mp4

DataTriny · 2024-07-03T22:08:08Z

packages/dioxus-blitz/src/accessibility.rs

+            };
+
+            node_builder.set_role(role);
+            node_builder.set_name(name);


In this scope, the variable name seem to always contain the "HTML element type" which is not something the user should see. You could call NodeBuilder::set_html_tag (although we currently don't expose this property to assistive technologies) but you should not use it for the AccessKit node name.

DataTriny · 2024-07-03T22:20:01Z

Hello @matthunz, AccessKit dev here.

I've tried a modified version of the accessibility example to add more kind of nodes and my screen reader is already able to pick them up.
One thing you will have to tackle early on is the labelled_by relationship. I tried an h1 and a button and on both cases the text that my screen reader attaches to the node is the HTML element node. I assume that the actual textual content is a child node of the element, so you will have to put the text node's ID in the labelled_by list property on the parent container.

matthunz · 2024-07-08T20:53:01Z

Hey @DataTriny thanks for your insight!

I switched NodeBuilder::set_name to NodeBuilder::set_html_tag in 9ae0fb7 . If there is a plan to send this to the platform eventually I'd say we could keep it here until then 🤔

I also tried added some basic labelled_by relationships in a882185 . I'm hoping concatenating child text node ids into the parent elements labelled_by field should give the right result.

I'd also really like to connect to aria-labelled-by at some point 👀

nicoburns

Happy to accept this as an initial implementation and iterate :)

matthunz added 4 commits June 18, 2024 15:37

Connect the hot-reloading crate

d611f85

Apply template updates from the hot-reload CLI

e710cdc

Run cargo fmt

860e80d

Make hot-reload a non-default feature

46a48b4

matthunz marked this pull request as draft June 20, 2024 21:38

matthunz added 4 commits June 20, 2024 18:04

Merge with main

21e11b6

Fix panic on root node and clean up

b09b2f9

Merge with main

9228169

Import accesskit

b270203

matthunz force-pushed the accesskit branch from ffd2d0c to b270203 Compare June 20, 2024 23:38

matthunz added 10 commits June 20, 2024 20:14

Merge branch 'hot-reload' of https://github.com/matthunz/blitz into a…

e108b15

…ccesskit

Pipe accesskit events to View

a6c643c

Build example accesskit tree

f4d03f7

Add Document::visit method and build initial accesskit tree from View

dde3fcb

Create mapping from accesskit id to node id

dcbfb91

Send basic tree updates on dom change

5b4e544

Clean up and map ids to node ids

f2be9f3

Rebuild tree until observability is figured out

a419383

Clean up

d407e0b

Fix tree hierarchy

05b04a2

matthunz marked this pull request as ready for review June 21, 2024 20:22

nicoburns requested changes Jun 23, 2024

View reviewed changes

matthunz added 4 commits June 26, 2024 15:46

Create AccessibilityState struct and refactor

3d3e67d

Handle more accesskit roles

6a009f5

Move accessibility to new module

7f7e91b

Rename accesskit feature flag to accessibility

0ce74dd

matthunz force-pushed the accesskit branch from 4392aab to 0ce74dd Compare June 26, 2024 20:05

Merge branch 'main' of https://github.com/matthunz/blitz into accesskit

709d505

DataTriny suggested changes Jul 3, 2024

View reviewed changes

Merge with main

f657a96

matthunz force-pushed the accesskit branch from 68b2064 to f657a96 Compare July 3, 2024 22:39

matthunz added 3 commits July 3, 2024 19:03

Fix merge

c2f984b

Set element node name instead of html tag

9ae0fb7

Add labelled_by relationship for text nodes and elements

a882185

nicoburns approved these changes Jul 9, 2024

View reviewed changes

nicoburns merged commit 57f9f5b into DioxusLabs:main Jul 9, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AccessKit integration #78

AccessKit integration #78

matthunz commented Jun 20, 2024 •

edited

Loading

nicoburns commented Jun 23, 2024

nicoburns Jun 23, 2024

matthunz Jun 29, 2024 •

edited

Loading

nicoburns Jun 23, 2024

matthunz Jun 29, 2024

nicoburns Jul 9, 2024

nicoburns Jun 23, 2024

nicoburns Jun 23, 2024

nicoburns Jun 23, 2024

matthunz Jun 29, 2024 •

edited

Loading

nicoburns Jun 23, 2024

nicoburns Jun 23, 2024

matthunz Jun 29, 2024

nicoburns Jul 9, 2024

nicoburns Jun 23, 2024

matthunz Jun 29, 2024

nicoburns Jun 23, 2024

matthunz Jun 29, 2024

nicoburns Jun 23, 2024

nicoburns Jun 23, 2024

matthunz commented Jun 29, 2024

DataTriny Jul 3, 2024

DataTriny commented Jul 3, 2024

matthunz commented Jul 8, 2024

nicoburns left a comment

	pub struct AccessibilityState {
	/// Adapter to connect to the [`EventLoop`](`winit::event_loop::EventLoop`).
	adapter: accesskit_winit::Adapter,

	/// Next ID to assign an an [`accesskit::Node`].
	next_id: u64,
	}

	// TODO match more roles
	let role = match &*name {
	"button" => Role::Button,
	"div" => Role::GenericContainer,
	"header" => Role::Header,
	"h1" \| "h2" \| "h3" \| "h4" \| "h5" \| "h6" => Role::Heading,
	"p" => Role::Paragraph,
	"section" => Role::Section,
	"input" => {
	let ty = element_data.attr(local_name!("type")).unwrap_or("text");
	match ty {
	"number" => Role::NumberInput,
	_ => Role::TextInput,
	}
	}
	_ => Role::Unknown,
	};

AccessKit integration #78

AccessKit integration #78

Conversation

matthunz commented Jun 20, 2024 • edited Loading

nicoburns commented Jun 23, 2024

Choose a reason for hiding this comment

matthunz Jun 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthunz Jun 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthunz commented Jun 29, 2024

Choose a reason for hiding this comment

DataTriny commented Jul 3, 2024

matthunz commented Jul 8, 2024

nicoburns left a comment

Choose a reason for hiding this comment

matthunz commented Jun 20, 2024 •

edited

Loading

matthunz Jun 29, 2024 •

edited

Loading

matthunz Jun 29, 2024 •

edited

Loading