libservo: Allow the embedder to activate accessibility#42336
libservo: Allow the embedder to activate accessibility#42336delan merged 8 commits intoservo:mainfrom
Conversation
f5bfb00 to
56054ff
Compare
56054ff to
4e85ef3
Compare
1ed4818 to
435e8ce
Compare
| fn set_accessibility_active(&mut self, active: bool) { | ||
| self.accessibility_active = active; | ||
| for browsing_context in self.browsing_contexts.values_mut() { | ||
| if let Some(pipeline) = self.pipelines.get(&browsing_context.pipeline_id) { | ||
| let _ = pipeline | ||
| .event_loop | ||
| .send(ScriptThreadMessage::SetAccessibilityActive( | ||
| browsing_context.webview_id, | ||
| pipeline.id, | ||
| active, | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@mrobinson said:
Is the idea that this will be enabled or disabled per-WebView in the future? If so, the external-facing API should likely be on
WebViewin the API. If not (and I think that's a perfectly fine decision), then this code should probably iterate though eachEventLoopby iterating throughConstellation::event_loops()here.If you take the second approach, then in
ScriptThread(there is one of these per-EventLoop), iterate through allDocuments and enable / disable accessibility for every Layout.
There was a problem hiding this comment.
@mrobinson Does the latest change do what you had in mind?
There was a problem hiding this comment.
Yes! This is much closer. I offered a suggestion for a different approach, but maybe that isn't necessary here. I think it could come in handy when you want to add support disabling accessibility though.
6656b26 to
34fc937
Compare
| let _ = pipeline | ||
| .event_loop | ||
| .send(ScriptThreadMessage::SetAccessibilityActive(true)); | ||
| } |
There was a problem hiding this comment.
Another option here is to store whether or not accessibility is enabled in each ScriptThread and then seed every new Layout with that information. That would avoid having the possibility of any "in between" time where accessibility seems to be disabled -- maybe a layout happens without it event.
This would require modifying theEventLoop creation code in components/constellation/event_loop.rs. For instance it could be a member of InitialScriptState which is used to initialize the state of a new ScriptThread (one per EventLoop). Then in each ScriptThread you would store the state and ensure that new Layouts have a11y enabled.
There was a problem hiding this comment.
I think this is necessary, yeah. So now we have accessibility_active in:
Constellation, so that newScriptThreads have the right configurationInitialScriptState, to pass the value through fromConstellationto each newScriptThreadScriptThread, so that newLayoutThreads have the right configurationLayoutConfig, to pass the value through fromScriptThreadto each newLayoutThread
I added a temporary flag in LayoutThread just to have a place for the config to "land" until we add the AccessibilityTree class.
Does this look right to you? Is there somewhere we're storing the flag unnecessarily now? Admittedly I'm still getting my head around the relationships between each of these objects.
| fn set_accessibility_active(&mut self, active: bool) { | ||
| self.accessibility_active = active; | ||
| for browsing_context in self.browsing_contexts.values_mut() { | ||
| if let Some(pipeline) = self.pipelines.get(&browsing_context.pipeline_id) { | ||
| let _ = pipeline | ||
| .event_loop | ||
| .send(ScriptThreadMessage::SetAccessibilityActive( | ||
| browsing_context.webview_id, | ||
| pipeline.id, | ||
| active, | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Yes! This is much closer. I offered a suggestion for a different approach, but maybe that isn't necessary here. I think it could come in handy when you want to add support disabling accessibility though.
b366aec to
06eb1a1
Compare
06eb1a1 to
9db1350
Compare
mrobinson
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing. Great stuff! Just a couple tiny nits, but feel free to land this when addressed:
2e6959d to
24d67f1
Compare
Co-authored-by: Alice Boxhall <alice@igalia.com> Signed-off-by: delan azabani <dazabani@igalia.com>
Now, the constellation loops over its event_loops, and each script_thread loops over its documents. Signed-off-by: Alice Boxhall <alice@igalia.com>
Also adds accessibility_active fields in InitialScriptState and LayoutConfig, so ScriptThread and LayoutThread can be initialized with the correct accessibility_active state. Signed-off-by: Alice Boxhall <alice@igalia.com>
Signed-off-by: Alice Boxhall <alice@igalia.com>
Signed-off-by: delan azabani <dazabani@igalia.com>
Signed-off-by: delan azabani <dazabani@igalia.com>
Signed-off-by: delan azabani <dazabani@igalia.com>
24d67f1 to
2ccb910
Compare
Signed-off-by: delan azabani <dazabani@igalia.com>
in #42336, we added a [Servo](https://doc.servo.org/servo/struct.Servo.html)-global API for controlling whether accessibility is active. the idea was that when the embedder activates accessibility, all webviews and documents activate accessibility and start sending AccessKit tree updates, and vice versa when they deactivate. we found a problem with this approach in #42338. global activation of accessibility makes it too easy to accidentally cause a panic in AccessKit, and harder than it needs to be for embedders to learn that they are responsible for grafting each webview’s subtree into their main AccessKit tree as soon as accessibility is activated. this is due to an invariant of the AccessKit subtree API: if a subtree starts sending updates before the graft node is created, the program panics. this patch reworks accessibility activation to make it per-webview. by requiring embedders to explicitly activate accessibility for a webview, we can communicate the AccessKit invariant via our API docs. Testing: this patch includes an initial accessibility test in libservo Fixes: part of #4344, extracted from our work in #42338 --------- Signed-off-by: Alice Boxhall <alice@igalia.com> Signed-off-by: delan azabani <dazabani@igalia.com> Co-authored-by: Alice Boxhall <alice@igalia.com>
in servo#42336, we added a [Servo](https://doc.servo.org/servo/struct.Servo.html)-global API for controlling whether accessibility is active. the idea was that when the embedder activates accessibility, all webviews and documents activate accessibility and start sending AccessKit tree updates, and vice versa when they deactivate. we found a problem with this approach in servo#42338. global activation of accessibility makes it too easy to accidentally cause a panic in AccessKit, and harder than it needs to be for embedders to learn that they are responsible for grafting each webview’s subtree into their main AccessKit tree as soon as accessibility is activated. this is due to an invariant of the AccessKit subtree API: if a subtree starts sending updates before the graft node is created, the program panics. this patch reworks accessibility activation to make it per-webview. by requiring embedders to explicitly activate accessibility for a webview, we can communicate the AccessKit invariant via our API docs. Testing: this patch includes an initial accessibility test in libservo Fixes: part of servo#4344, extracted from our work in servo#42338 --------- Signed-off-by: Alice Boxhall <alice@igalia.com> Signed-off-by: delan azabani <dazabani@igalia.com> Co-authored-by: Alice Boxhall <alice@igalia.com>
this patch adds a Servo::set_accessibility_active() method that embedders can use to tell Servo to start building and sending accessibility trees to the platform, as long as the pref is enabled (#42333). doing so sets a global flag in the constellation, which is then propagated to the layout of all existing and future pipelines.
Testing: none yet, no functional change
Fixes: part of #4344