Support the AccessKit focus and blur actions #596
Conversation
|
It certainly would be better for the framework to support this automatically for any focusable widget. I'll see if there's an easy way to do that in the current architecture. |
|
To let the framework automatically handle the AccessKit focus and blur actions, it seems to me that we should add a flag to |
|
Yeah, I think that makes sense. Don't forget to reset that flag in the I know that our logic for focus has not been fully reasoned about, so a short-term change to unlock this functionality should be fine. (In particular, I don't think we know how setting a custom 'tab-index' will work) |
mwcampbell
changed the title
|
OK, now the framework generically supports this for any widget in the focus chain. |
masonry/src/passes/event.rs
Outdated
| if ctx.is_in_focus_chain() && !ctx.is_disabled() && !ctx.is_focused() { | ||
| ctx.request_focus(); | ||
| } | ||
| return; |
There was a problem hiding this comment.
There's an argument to be made that this return should be conditional only on ctx.is_in_focus_chain(), because that's when the corresponding implicit focus action was added.
However, I think that's probably YAGNI
| match event.action { | ||
| accesskit::Action::Focus => { | ||
| if ctx.is_in_focus_chain() && !ctx.is_disabled() && !ctx.is_focused() { | ||
| ctx.request_focus(); |
There was a problem hiding this comment.
I think we should probably also call ctx.set_handled() here. See #masonry>Missing event set_handled calls
masonry/src/passes/accessibility.rs
Outdated
| @@ -111,6 +111,10 @@ fn build_access_node(widget: &dyn Widget, state: &WidgetState, scale_factor: f64 | |||
| if state.clip.is_some() { | |||
| node.set_clips_children(); | |||
| } | |||
| if state.in_focus_chain { | |||
| node.add_action(accesskit::Action::Focus); | |||
| node.add_action(accesskit::Action::Blur); | |||
There was a problem hiding this comment.
I presume it's meaningful to have the blur action available on an already unfocused widget?
There was a problem hiding this comment.
No, it's not. But can I check whether the widget is focused without passing global state into this function?
There was a problem hiding this comment.
As far as I can see, the global state is available in the calling context, so passing it in is fine.
I'd probably shift this code around so that the functionality to set all of these properties is passed the AccessCtx.
There was a problem hiding this comment.
OK, I think I just did what you suggested.
…s has access to AccessCtx, like the widgets themselves
|
OK, I think my last push addresses all remaining review comments. |
|
@DJMcNab @PoignardAzur Can you please give this one more look before I merge? |
|
Will do tomorrow. |
| fn set_common_properties(ctx: &mut AccessCtx) { | ||
| if ctx.is_hot() { | ||
| ctx.current_node().set_hovered(); | ||
| } | ||
| if state.is_disabled { | ||
| node.set_disabled(); | ||
| if ctx.is_disabled() { | ||
| ctx.current_node().set_disabled(); | ||
| } | ||
| if state.is_stashed { | ||
| node.set_hidden(); | ||
| if ctx.is_stashed() { | ||
| ctx.current_node().set_hidden(); | ||
| } | ||
| if state.clip.is_some() { | ||
| node.set_clips_children(); | ||
| if ctx.widget_state.clip.is_some() { | ||
| ctx.current_node().set_clips_children(); | ||
| } | ||
| if ctx.is_in_focus_chain() && !ctx.is_disabled() { | ||
| ctx.current_node().add_action(accesskit::Action::Focus); | ||
| } | ||
| if ctx.is_focused() { | ||
| ctx.current_node().add_action(accesskit::Action::Blur); | ||
| } |
There was a problem hiding this comment.
Is there a reason this is split out from build_access_node?
There was a problem hiding this comment.
... #596 (comment)
Essentially, build_access_node doesn't have the AccessCtx, which means it would either need to duplicate the logic of the context methods or not have this functionality implemented properly.
Co-authored-by: Olivier FAURE <[email protected]>
I believe this is necessary to activate and deactivate input focus with TalkBack on Android. Assistive technologies on other platforms can also request the focus action, though none of the AccessKit backends support the blur action yet.