Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear focus if a platform view goes away #17381

Merged
merged 7 commits into from
Apr 16, 2020

Conversation

goderbauer
Copy link
Member

When a SemanticsNode with a platformViewId is removed from the SemanticsTree, we need to clear the a11y focus in the associated embedded platfromview if it had focus.

Fixes b/149886151.

@goderbauer
Copy link
Member Author

/cc @amirh

== platformViewsAccessibilityDelegate.getPlatformViewById(semanticsNodeToBeRemoved.platformViewId)) {
// If the currently focused a11y node is within a platform view that is
// getting removed: clear it's a11y focus.
sendAccessibilityEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about sending an event on behalf of the webview (e.g my concern is that it's internal state may think that it is still focused), do you know whether this is considered safe to do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, I don't have an authoritative answer to that question :(

In my experiments it all seems to work out. E.g. when you go back to the route with the webview by popping the route above it, focus behaves reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll live with that if it's the best we got 😕

The other approach I'd consider would be to see if we can make the webview send that event (e.g maybe by turning it invisible within the vd?)

Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

Thanks!

I'm ok with the change, we'll need either some test or a test exemption to land this.

@goderbauer
Copy link
Member Author

@amirh Are there any existing tests for the a11y integration of PlatformViews I can borrow from?

@amirh
Copy link
Contributor

amirh commented Mar 30, 2020

Unfortunately no, AFAIK there we currently have no tests for Android a11y :(

@googlebot

This comment has been minimized.

1 similar comment
@googlebot

This comment has been minimized.

@goderbauer goderbauer force-pushed the a11yfocusplatformview branch from 10eb6eb to ea22916 Compare April 16, 2020 04:23
@googlebot

This comment has been minimized.

1 similar comment
@googlebot

This comment has been minimized.

@goderbauer
Copy link
Member Author

A unit test has been added. PTAL @amirh

@goderbauer goderbauer requested a review from amirh April 16, 2020 17:25
@goderbauer
Copy link
Member Author

The "Mac Android AOT Engine" check passed on re-run: https://ci.chromium.org/p/flutter/builders/try/Mac%20Android%20AOT%20Engine/1015

Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM

@goderbauer goderbauer merged commit f4d6ce1 into flutter:master Apr 16, 2020
@goderbauer goderbauer deleted the a11yfocusplatformview branch April 16, 2020 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants