Accessibility for text #615

mwcampbell · 2024-09-27T00:56:30Z

This adds the necessary AccessKit properties and child nodes to expose detailed information about text widgets. It also supports the SetTextSelection action.

mwcampbell · 2024-09-27T00:57:52Z

I'm doing this as a draft for now because it's a bit quick and dirty, doesn't yet expose the bounding rectangles of runs and the relative positions and widths of characters (though AccessKit is designed to degrade gracefully in the absence of this information), isn't optimized, and might violate layering in a way we don't want.

mwcampbell · 2024-09-27T00:58:56Z

Forgot to mention: This is known to work on Windows, when editing or moving in a textbox and when using NVDA's review commands within a prose widget like the one in the mason example.

DJMcNab

I've not run this, but at first glance this code looks really good!

One thing which does concern me is that any accessibility events targeted at the individual text runs won't get routed correctly. I think it is reasonable to not have a one-to-one widget to accessibility node mapping, but I don't know how to represent that in our architecture (without a side-channel mapping of accessibility ids to widgets).

masonry/src/text/selection.rs

masonry/src/widget/prose.rs

DJMcNab · 2024-09-27T08:35:47Z

masonry/src/text/layout.rs

@@ -501,6 +516,89 @@ impl<T: AsRef<str> + Eq> TextLayout<T> {
            &self.layout,
        );
    }
+
+    pub fn accessibility(&mut self, ctx: &mut AccessCtx, parent_node: &mut NodeBuilder) {


As you predicted, this using AccessCtx is a slight layering break. I think for expedience this is fine, although I think we could also pass in the individual fields (I think only tree_update is used?)

mwcampbell · 2024-09-27T12:55:18Z

FOrtunately, AccessKit never sends action requests to the individual runs.

I'll address the rest of the comments later.

mwcampbell · 2024-09-27T13:12:58Z

Also to clarify a previous comment, this code should be as functional on macOS and Linux as it is on Windows. I just haven't tested on those platforms yet.

mwcampbell · 2024-09-27T13:26:21Z

I wonder if I should move this out of draft and plan to land it without filling in the positions of the runs and characters for now. FWIW, one non-Rust toolkit, and its flagship application, has been using AccessKit without providing that info, and that has been in the hands of real users for a full year now.

mwcampbell · 2024-09-27T13:31:12Z

Also, I left in a TODO comment speculating about adding a trailing newline to the run node if it's at the end of a line that's not the last line in the layout. But if the prose in the mason example is a reliable indication, the last (or only) run in the last line of a paragraph that's not the last paragraph in the text already does include the trailing \n, and blank paragraphs have a single run with nothing but the \n. Are there any caveats I need to watch out for here?

mwcampbell · 2024-10-03T16:13:29Z

Taking this out of draft because we agreed we'd land this PR without bounding boxes for runs and positions/widths of characters.

PoignardAzur

Overall, I'm not very comfortable with this code being added as-is. Aside from the layering break, which shouldn't be too hard to address, this is very logic-intensive code, so to speak, with what seems like a lot of implicit invariants and no comments documenting them.

I'm especially uncomfortable with the three hashmap fields. They seem like the could be prone to cache invalidation errors if we're not careful.

I'm not sure if there's another solution, though. As I understand it, this code performs a one-to-may mapping of "text widget" to "accesskit nodes" that can't really be avoided. Although character_lengths_by_access_id seems like information that accesskit should already store for us?

masonry/src/text/layout.rs

PoignardAzur · 2024-10-05T11:27:19Z

masonry/src/text/layout.rs

+                    ctx.tree_update.nodes.push((last_id, last_node.build()));
+                    parent_node.push_child(last_id);


This could probably be done in the caller. TextLayout::Accessibility could return a Vec<NodeBuilder>.

I don't like returning a Vec because that requires an intermediate allocation. I'll implement Daniel's suggestion of passing in the TreeUpdate rather than the AccessCtx.

masonry/src/text/selection.rs

mwcampbell · 2024-10-12T16:41:08Z

I believe I've now adequately addressed all review feedback.

jaredoconnell

I'm not going to leave an approval since I'll let the prior reviewers determine if their concerns were addressed. VoiceOver on mac seems to work fine with this branch, but I am not an expert on screen readers. Test selection seems to be properly handled by VoiceOver.

mwcampbell · 2024-10-15T12:23:41Z

@PoignardAzur Please review again when you have time. Thanks.

PoignardAzur · 2024-10-15T16:14:32Z

Sorry, I was low on energy these last few days. Will review tomorrow.

PoignardAzur

Overall, I'm still uncomfortable with the architecture and the number of invariants it seems to maintain, but I don't have any alternatives.

I don't see any blockers. This can be merged as soon as the code is rebased.

PoignardAzur · 2024-10-16T09:15:51Z

masonry/src/text/selection.rs

+            if !(range.contains(&offset)
+                || (offset == range.end
+                    && (affinity == Affinity::Upstream
+                        || line_index == self.layout.layout.len() - 1)))


I wonder if we could add a function in Parley which does this check.

masonry/src/text/selection.rs

masonry/src/widget/prose.rs

PoignardAzur · 2024-10-16T09:42:03Z

masonry/src/widget/prose.rs

+        node.set_read_only();
+        self.text_layout.accessibility(ctx.tree_update, node);


Any reason the same code isn't used in Label::accessibility?

Yes, we ought to do this for labels too. I'd like to wait and do that after I do an AccessKit refactor so we don't have to also set the name property on the label. I don't think that should block merging this PR.

…indows adapter apparently doesn't check whether the node claims to support the action.

…is supported. Support that action in Prose.

…end of the text. That was especially fragile because it assumed that the parent node has the run nodes as its direct children, and those were its only children. These things might change soon.

…, as we do in response to a mouse event

…to panic in release builds

masonry/src/text/selection.rs

Co-authored-by: Olivier FAURE <[email protected]>

mwcampbell · 2024-10-16T12:33:59Z

@PoignardAzur Requesting final approval after my last changes.

PoignardAzur

LGTM.

DJMcNab reviewed Sep 27, 2024

View reviewed changes

mwcampbell marked this pull request as ready for review October 3, 2024 16:13

PoignardAzur reviewed Oct 5, 2024

View reviewed changes

mwcampbell force-pushed the text-access branch from 1775a4e to a5264d3 Compare October 12, 2024 14:23

jaredoconnell reviewed Oct 12, 2024

View reviewed changes

PoignardAzur approved these changes Oct 16, 2024

View reviewed changes

mwcampbell added 14 commits October 16, 2024 07:02

Accessibility for text

af5b4d1

Forgot to add SetTextSelection as a supported action. The AccessKit W…

29b8878

…indows adapter apparently doesn't check whether the node claims to support the action.

Revise the logic for determining whether the SetTextSelection action …

1e66df7

…is supported. Support that action in Prose.

Code style fix requested in code review

85d838d

Add comments explaining and defending the hash maps

4e148a1

Add comments to TextLayout::accessibility

6c2606a

Eliminate access_position_from_offset's fragile special case for the …

72e7bbb

…end of the text. That was especially fragile because it assumed that the parent node has the run nodes as its direct children, and those were its only children. These things might change soon.

Add a range check to the outer loop in access_position_from_offset

3aa1566

More useful unrecoverable error handling

b1b1673

Fix off-by-one error in new range checks

fa69f24

Don't pass AccessCtx into text accessibility methods

d77cddc

Make the text selection visible when it's set via an AccessKit action…

da97531

…, as we do in response to a mouse event

Remove extraneous &mut

1adcd12

Remove unnecessary event target checks

08de4bf

mwcampbell added 2 commits October 16, 2024 07:02

Make access_position_from_offset return an Option so it doesn't have …

f77cc61

…to panic in release builds

Fix to work with the latest text refactor

627b27b

mwcampbell force-pushed the text-access branch from d0107d2 to 627b27b Compare October 16, 2024 12:14

PoignardAzur reviewed Oct 16, 2024

View reviewed changes

masonry/src/text/selection.rs Outdated Show resolved Hide resolved

mwcampbell and others added 2 commits October 16, 2024 07:22

Avoid nested conditionals

3377cb2

Co-authored-by: Olivier FAURE <[email protected]>

fmt after suggested change

f743392

PoignardAzur approved these changes Oct 16, 2024

View reviewed changes

mwcampbell added this pull request to the merge queue Oct 16, 2024

Merged via the queue into main with commit c73beee Oct 16, 2024
17 checks passed

mwcampbell deleted the text-access branch October 16, 2024 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility for text #615

Accessibility for text #615

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

DJMcNab left a comment

DJMcNab Sep 27, 2024 •

edited

Loading

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Oct 3, 2024

PoignardAzur left a comment

PoignardAzur Oct 5, 2024

mwcampbell Oct 12, 2024

mwcampbell commented Oct 12, 2024

jaredoconnell left a comment

mwcampbell commented Oct 15, 2024

PoignardAzur commented Oct 15, 2024

PoignardAzur left a comment

PoignardAzur Oct 16, 2024

PoignardAzur Oct 16, 2024

mwcampbell Oct 16, 2024

mwcampbell commented Oct 16, 2024

PoignardAzur left a comment

		ctx.tree_update.nodes.push((last_id, last_node.build()));
		parent_node.push_child(last_id);

		node.set_read_only();
		self.text_layout.accessibility(ctx.tree_update, node);

Accessibility for text #615

Accessibility for text #615

Conversation

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

DJMcNab Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Sep 27, 2024

mwcampbell commented Oct 3, 2024

PoignardAzur left a comment

Choose a reason for hiding this comment

PoignardAzur Oct 5, 2024

Choose a reason for hiding this comment

mwcampbell Oct 12, 2024

Choose a reason for hiding this comment

mwcampbell commented Oct 12, 2024

jaredoconnell left a comment

Choose a reason for hiding this comment

mwcampbell commented Oct 15, 2024

PoignardAzur commented Oct 15, 2024

PoignardAzur left a comment

Choose a reason for hiding this comment

PoignardAzur Oct 16, 2024

Choose a reason for hiding this comment

PoignardAzur Oct 16, 2024

Choose a reason for hiding this comment

mwcampbell Oct 16, 2024

Choose a reason for hiding this comment

mwcampbell commented Oct 16, 2024

PoignardAzur left a comment

Choose a reason for hiding this comment

DJMcNab Sep 27, 2024 •

edited

Loading