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

Return null from ComposeAccessible.getAccessibleContext when the node has been removed #865

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

m-sasha
Copy link

@m-sasha m-sasha commented Oct 8, 2023

The long story of this PR is this:

I reproduced the printed exception in JetBrains/compose-multiplatform#3728 and noticed that we don't report to the accessibility system anywhere when a node has been removed. So I thought it wasn't surprising that it would continue calling functions on it, which would then throw that exception.

So I set out to report node addition/removal by correctly firing AccessibleContext.ACCESSIBLE_CHILD_PROPERTY property change events. When that didn't do anything, I realized that ComposeAccessibleComponent doesn't have the child/parent hierarchy set up correctly, and thought that perhaps this is confusing the system.

Specifically rootAccessible in AccessibilityControllerImpl (which is only the root for a single SemanticsOwner, so it's not really the root) doesn't report having a parent, even though ComposeSceneAccessibleContext returns it from getAccessibleChild. So I fixed that. But unfortunately that didn't help either.

Then I decided to see what exactly the system does when it receives an ACCESSIBLE_CHILD_PROPERTY property change event. Turns out that AXChangeNotifier (the accessible listener on macOS) doesn't even listen to that event, and its (native) ptr is null altogether for some reason.

So then I just gave up and implemented this PR. It simply marks ComposeAccessibles as removed when they are removed, and their getAccessibleContext then returns null. In CAccessibility.java, all the getter functions check the return value of getAccessibleContext and return null if it returns null.

If we want to properly report ACCESSIBLE_CHILD_PROPERTY events and have the parents set up correctly, I can submit a separate PR, or just keep it stashed for now.

Proposed Changes

When the semantics node of a ComposeAccessible is removed, mark it as removed, and have it return null from getAccessibleContext.

Fixes: JetBrains/compose-multiplatform#3728

@m-sasha m-sasha requested a review from Walingar October 8, 2023 20:24
@m-sasha m-sasha force-pushed the m-sasha/fix-layoutnode-exception branch from 9d0dc12 to 5f5e336 Compare October 8, 2023 20:26
@@ -91,7 +91,15 @@ internal class ComposeAccessible(

val composeAccessibleContext: ComposeAccessibleComponent by lazy { ComposeAccessibleComponent() }

override fun getAccessibleContext(): AccessibleContext {
var removed = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I right that tree will be recreated after a while and ComposeAccessible will be collected? Otherwise it looks like a leak

Copy link
Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean. The node is marked as removed=false when it's no longer in the tree. I'm not storing it anywhere it wasn't before.

sun.lwawt.macosx.CAccessible will probably keep storing a reference to it, but only until some other node becomes "active" (it only stores one Accessible).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I am talking about sun.lwawt.macosx.CAccessible, so I hope that ComposeAccessible won't be stored there forever

Copy link
Collaborator

@Walingar Walingar left a comment

Choose a reason for hiding this comment

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

But anyway I also investigated this behavior when working on the issue with non-working accessibility in popups. I thought that the issue is that we are not notifying system about tree changes. But system doesn't even listen for the events.
So, for me this solution works for now, but I am afraid about possible leak

@m-sasha m-sasha merged commit c3684fa into jb-main Oct 10, 2023
3 checks passed
@m-sasha m-sasha deleted the m-sasha/fix-layoutnode-exception branch October 10, 2023 12:54
mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants