Skip to content

Conversation

MarkCSmith
Copy link
Contributor

Avoid calling isAccessibilityFocused() on Android < Lollipop.

Summary:

Fixes issue #220.

Test Plan:

We reproduced a crash on an Android 4.1 emulator, applied this patch, and observed that the crash no longer occurred.

@michalchudziak
Copy link
Collaborator

Hey @MarkCSmith ! Thank you for this PR.

I wonder if removing a call of this.setupAccessibility(); at all on Android < Lollipop is the way to go. You claim that calling isAccessibilityFocused() is crashing an app. Perhaps we should omit just this call on these Android versions and just rely on event.getEventType() == AccessibilityEvent.TYPE_VIEW_SELECTED? Perhaps it'd resolve the crash and maintain this.setupAccessibility(); call?

Any thoughts?

@MarkCSmith
Copy link
Contributor Author

Hey @MarkCSmith ! Thank you for this PR.

I wonder if removing a call of this.setupAccessibility(); at all on Android < Lollipop is the way to go. You claim that calling isAccessibilityFocused() is crashing an app. Perhaps we should omit just this call on these Android versions and just rely on event.getEventType() == AccessibilityEvent.TYPE_VIEW_SELECTED? Perhaps it'd resolve the crash and maintain this.setupAccessibility(); call?

I think that approach would be fine. However, my understanding is that React Native 0.64.x drops support for Android 4.x. Maybe in that case it is no longer important to fix this issue. What do you think?

@BartoszKlonowski
Copy link
Member

In my opinion it is always worth to support all versions, no matter if they are supported on newer RN releases.
@MarkCSmith It would be great if you could rebase your branch so the conflicts are gone and we can revisit the fix.

Avoid calling isAccessibilityFocused() on Android < Lollipop.
Copy link
Member

@BartoszKlonowski BartoszKlonowski left a comment

Choose a reason for hiding this comment

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

Thank you, @MarkCSmith!

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
if (event.getEventType() == AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED ||
(event.getEventType() == AccessibilityEvent.TYPE_VIEW_SELECTED && this.isAccessibilityFocused())) {
this.setupAccessibility((int)mValue);
Copy link
Member

Choose a reason for hiding this comment

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

I allowed myself to rebase your changes @MarkCSmith and I adjusted them to the latest of setupAccessibility method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @BartoszKlonowski !

@BartoszKlonowski BartoszKlonowski merged commit 8cc6558 into callstack:master Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint error: Calling new methods (isAccessibilityFocused) on older versions
3 participants