-
Notifications
You must be signed in to change notification settings - Fork 6k
Add Android accessibility bridge support for "longPress" message and handlers for LONG_CLICK/CLICK #5081
Add Android accessibility bridge support for "longPress" message and handlers for LONG_CLICK/CLICK #5081
Conversation
|
cc @goderbauer I investigated and found the call to |
…to semantics service
|
per discussion offline, updated the accessibility bridge to dispatch both types of click events only when a semantic service event is received. This does not fix the staleness issue on Lollipop and below, but it does make the feedback from tap and long press events work consistently across android versions |
| if (mA11yFocusedObject == null) { | ||
| return; | ||
| } | ||
| AccessibilityEvent e = obtainAccessibilityEvent(mA11yFocusedObject.id, AccessibilityEvent.TYPE_VIEW_LONG_CLICKED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we obtain the id from the event? The focus might have changed between clicking the button and dispatching this event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to obtain a semantics node's id from the framework side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is, but it's not easy. We used to do this for the "scroll happened" event, which was removed in flutter/flutter#14536. Maybe the old code their is inspiration how to get the id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Investigated this a bit, the issue is that the prior code relied on the node in question being a semantic boundary. In the case of the gesture detectors/inkwell this doesn't seem to be the case, so actually finding the semantics node takes some serious spaghetti.
At this point, the more general solution is seeming worse and worse.
Unless we're really set against it, I think the approach of using the currently focused node is actually just as good/bad as trying to find the correct semantics node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we could use the render object, grab the center and then pass the offset back to the AccessibilityBridge. in theory hit testing the semantics objects there would give us the correct id, thoughts? @goderbauer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's no good then. currently focused object seems like the best bet to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can mA11yFocusedObject be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(You should assume that it is possible that the framework side is hostile and will call this API willy nilly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what you mean, I am already checking if it is null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, so you are. I was only looking at the very line here, my bad!
| return false; | ||
| } | ||
| switch (action) { | ||
| case AccessibilityNodeInfo.ACTION_CLICK: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this stay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can stay but it doesn't currently change any behavior, either
- AccessibilityService interprets gesture -> dispatch semantics action -> framework sends tap message -> tap event fired
or
- AccessibilityService interprets gesture -> ignored by performAction -> gesture interpreted by framework -> tap event fired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So neither 1 or 2 ever calls performAction?
What about overlapping elements? What happens when you have accessibility focus on the bottom one and you "click" it. Will the click simulated by AccessibilitySdervice not end up on the top element if performAction is bypassed? I know that's how it worked prior to Oreo where performAction is always ignored. But I thought with Oreo this situation now behaves correctly because performAction is used and routes the click to the correct element on the bottom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see what you mean. In newer versions of Android removing this would cause all events to end up on top. Added back the old logic here for regular clicks
|
@goderbauer friendly ping! |
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| if (nodeId == null) { | ||
| return; | ||
| } | ||
| AccessibilityEvent e = obtainAccessibilityEvent(nodeId, AccessibilityEvent.TYPE_VIEW_LONG_CLICKED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just be calling sendAccessibilityEvent(int virtualViewId, int eventType) here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The first step towards fixing flutter/flutter#14187
I did investigate a message channel approach for regular click events, but this did not fix staleness issues and is strictly worse when the Accessibility Bridge correctly handles these gestures. I don't yet have a solution to making all a11y clicks work for lollipop and below.