Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jonahwilliams
Copy link
Contributor

The first step towards fixing flutter/flutter#14187

  • The new message channel event will be dispatched from the existing a11y service
  • The handlers for ACTION_CLICK_EVENT automagically handles a11y clicks > lollipop. The existing work around for a11y clicks is now guarded behind a version flag.

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.

@jonahwilliams
Copy link
Contributor Author

cc @goderbauer I investigated and found the call to performAction is before the semantics tree is updated. Since the announcement is correct, this leads me to believe that the accessibility service can figure out that the checkbox/switch/radio is going to change states with the given semantics information it already has

@jonahwilliams
Copy link
Contributor Author

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);
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can mA11yFocusedObject be null?

Copy link
Contributor

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.)

Copy link
Contributor Author

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

Copy link
Contributor

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: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this stay?

Copy link
Contributor Author

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

  1. AccessibilityService interprets gesture -> dispatch semantics action -> framework sends tap message -> tap event fired

or

  1. AccessibilityService interprets gesture -> ignored by performAction -> gesture interpreted by framework -> tap event fired

Copy link
Member

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?

Copy link
Contributor Author

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

@jonahwilliams
Copy link
Contributor Author

@goderbauer friendly ping!

Copy link
Member

@goderbauer goderbauer left a 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);
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants