Add optional AccessKit integration for text layout and PlainEditor
#166
Conversation
|
I'm aware that this may soon be invalidated by @dfrg's planned change from character indices to gap indices. But I thought I should still get this PR open first, since I have something that more or less works. The part I'm least sure about is how to expose geometry, particularly the bounding rectangles for the runs. I'm not sure if I'm doing the right thing for layouts where the dominant direction is RTL. But then, I'm also not sure if vello_editor's code for rendering the selection and cursor works in that case. Another thing I need confirmation on: Does |
| // Simple struct to hold the state of the renderer | ||
| pub struct ActiveRenderState<'s> { | ||
| // The fields MUST be in this order, so that the surface is dropped before the window | ||
| // The fields MUST be in this order, so that the surface and AccessKit adapter are dropped before the window |
There was a problem hiding this comment.
I don't think this is true for wgpu any more, since the Arc<Window> is also owned by the wgpu Surface.
This might still be correct for accesskit, however. I'd have to dig into it.
There was a problem hiding this comment.
The AccessKit adapter definitely doesn't own the window. The platform backends are supposed to deal gracefully with the adapter outliving the window, but still, the adapter should be destroyed first. I'm not so sure about the surface. I'd think that if the surface owned an Arc<Window>, we'd see Arc<Window> as a parameter on RenderSurface, as I did with softbuffer's Surface when experimenting with that yesterday.
There was a problem hiding this comment.
Internally, wgpu stores the raw window in a boxed form. But yeah, this specific comment doesn't matter too much.
| } | ||
| } | ||
| for id in ids_to_remove { | ||
| self.run_paths_by_access_id.remove(&id); |
There was a problem hiding this comment.
Presumably it's a choice for simplicity to not reuse these ids?
There was a problem hiding this comment.
An AccessKit node ID should be the stable identity of one and only one UI element for as long as that element lives. So no, AccessKit IDs should generally not be reused for different elements. This maps easily to Masonry widget IDs, which themselves aren't reused as far as I know. In Parley, the closest thing we have to a stable identity for a given run is the tuple of its line index and run index. We don't want the mappings between these run paths to accumulate indefinitely over the life of a layout, so that's why I prune them.
There was a problem hiding this comment.
Hmm. I guess that makes sense as a pragmatic choice. It's not like there's any shortage of 64 bit ids.
My concern is also that if you e.g. have:
{a bit of text}[0]
{with some importance}[1]
(where {}[id] marks a run with id id)
and then add a newline before the word "text", you end up with:
{a bit of}[0]
{text}[1]
{with some importance}[2]
That is, the second run has been completely transformed in content.
I really don't have a good suggestion here, unfortunately. I imagine your best chance of doing better is with something like the editor abstraction, so you know what has changed. But that's an awful lot of bookkeeping work needed.
There was a problem hiding this comment.
You're completely right about the effects of inserting text. I'm not sure how much this matters. Of the platform backends implemented so far, this only makes a difference at all on Windows, where UIA clients can have long-lived references to text range objects, and in our implementation, those objects point to specific run IDs. I don't know if any UIA implementation really handles this ideally.
There was a problem hiding this comment.
It's also worth noting that the egui implementation, which has been my de-facto reference implementation until now, has the same problem with the insertion scenario. So the egui implementation is really no better. It uses line indices as the basis of run node IDs. It just doesn't have an explicit mapping, since it generates AccessKit node IDs via hashing.
|
I've now addressed all of @DJMcNab's comments. Should I wait for anyone else in particular to review this before merging? |
… already transitively depend on hashbrown
…ate(), to make sure we get the right indices for lines with a mix of item types. Also collect the runs and their offsets into a Vec and sort them into text order first, now that I understand run ordering better.
|
It should probably get a once-over from @dfrg, but I think this can land even if we don't hear from him today. |
9dbcb6e to
78423cf
Compare
|
I got two approvals, but I'm still inclined to wait and see if @dfrg, who seemingly has the most expertise about text and parley specifically, can look at this soon. But if anyone else thinks this should be landed immediately, I can do that. |
|
The code generally LGTM. I think there’s room for improvement if we adjust some of the layout data structures to be more amenable to accesskit integration but that’s something to keep in mind for future work so I’m happy to land this in current form. Thanks! |
This is a draft because some functionality (mostly around word/line selection) is still missing. I also commented out the fix from #163 because the `skip_mandatory_break` flag is now gone and I haven't dug into the logic from that PR. Submitting this now so that people can take a look and see if it fixes some of the known bugs, particularly around newline handling. note: #166 should probably land first
This is an optional feature in Parley.
vello_editordemonstrates how to use it.