Skip to content

Commit

Permalink
Fix crash caused by accessibility being turned on and using a Modal
Browse files Browse the repository at this point in the history
Summary: When the ModalHostView is added as a child of whatever view holds it, if accessibility is turned on, Android will walk up to the root and then walk all children of the child and verify that they are indeed children of the root.  Since ModalHostView actually adds its children to a new ReactDialogViewGroup which has the Dialog as a parent, there is a disagreement about the tree deep in the bowels of View when it performs that walk.  The trick is to stop from adding the children of the ModalHostView when walking for accessibility.  The accessibility of those children views are properly handled by the hosting Dialog.

Reviewed By: andreicoman11

Differential Revision: D3230033

fb-gh-sync-id: 1e5ac334c996b1d5f50c75ded60805d8b871477a
fbshipit-source-id: 1e5ac334c996b1d5f50c75ded60805d8b871477a
  • Loading branch information
Dave Miller authored and Facebook Github Bot 7 committed Apr 27, 2016
1 parent 9a3a082 commit 57c40d9
Showing 1 changed file with 8 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

import javax.annotation.Nullable;

import java.util.ArrayList;

import android.app.Dialog;
import android.content.Context;
import android.content.DialogInterface;
Expand Down Expand Up @@ -99,6 +101,12 @@ public void removeViewAt(int index) {
mHostView.removeView(child);
}

@Override
public void addChildrenForAccessibility(ArrayList<View> outChildren) {
// Explicitly override this to prevent accessibility events being passed down to children
// Those will be handled by the mHostView which lives in the dialog
}

public void dismiss() {
if (mDialog != null) {
mDialog.dismiss();
Expand Down

4 comments on commit 57c40d9

@kylehendricks
Copy link

@kylehendricks kylehendricks commented on 57c40d9 Apr 27, 2016

Choose a reason for hiding this comment

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

@dmmiller Should the addChildrenForAccessibility call be forwarded to the DialogRootViewGroup?

@dmmiller
Copy link

Choose a reason for hiding this comment

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

No. That would basically cause the issue again. It was effectively doing that anyway because the calls to get children would get delegated to the DialogRootViewGroup. We need to stop the chain. The ModalHostView is really just an empty node in the tree. It is just there so that we can create a new tree rooted at the Dialog.

@kylehendricks
Copy link

Choose a reason for hiding this comment

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

@dmmiller
I was actually just about to open a PR for this right as you committed.

From what I could see, calling addChildrenForAccessibility was causing the ReactModalHostView to eventually get passed into ViewGroup.offsetRectBetweenParentAndChild which would obviously blow up because it itself doesn't actually exist in the view hierarchy. When I delegated that call into DialogRootViewGroup it seemed to make it so ReactModalHostView never got involved in this Accessibility stuff.

When I tested it with the delegation to DialogRootViewGroup, the crash was gone. I'm not actually sure which solution is correct but it seems like if we could delegate the call it would keep some of this accessibility functionality working.

As a side note, this is causing us a ton of crashes in our app right now. Do you think it would be possible to cherry-pick this into 0.25.0?

@dmmiller
Copy link

Choose a reason for hiding this comment

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

cc @mkonicek or @bestander about cherry picking into 25.

Accessibility seems to still work in my testing with it like this. I think if you delegate the call, the problem will still occur since they add the same children. The call to get children is already delegated.

Please sign in to comment.