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

fix(android): ListView tap not working after setting children as focusable #10522

Merged
merged 8 commits into from Apr 19, 2024

Conversation

CatchABus
Copy link
Contributor

PR Checklist

What is the current behavior?

We had reports that android ListView itemTap event stopped working at core 8.7 and later.
The problem seemed to be that ListView proxy views became focusable due to some a11y calls, thus breaking native item click listener. A thread in StackOverflow explains why ListView does not operate well if list items become focusable: https://stackoverflow.com/questions/11610023/click-is-not-working-on-the-listitem-listview-android

What is the new behavior?

Views will become focusable or non-focusable only when needed.

This is a continuation of the previous fix related to WebView accessibility focus: 92b2ff8

@farfromrefug Let me know if there's room for any improvements.

@cla-bot cla-bot bot added the cla: yes label Apr 18, 2024
@CatchABus CatchABus changed the title fix(android): ListView tap not working after setting layout children as focusable fix(android): ListView tap not working after setting children as focusable Apr 18, 2024
@farfromrefug
Copy link
Collaborator

farfromrefug commented Apr 18, 2024

@CatchABus only issue I see is that it is not synced with isUserInteractionEnabled.if accessibility is disabled then focusable seems to not be linked to isUserInteractionEnabled
EDIT: Thinking about this more. So if i understand correctly the issue with ListView is that isUserInteractionEnabled is bound in N to setFocusable, and setFocusable breaks itemTap event?
So what i personally see:

  • this PR breaks setFocusable sync with accessibilityEnabled/isUserInteracationEnabled for other views (which was broken already in my first PR)
  • we need a way to "prevent" setFocusable call on ListView content view WITHOUT changing any behavior when views are outside ListView => the fix should be in ListView component
  • i am starting to wonder is setting default to false for accessibilityEnabledProperty here is not the right way to set the default. Doing so seems to implies a call to accessibilityEnabledProperty.setNative for all views with the default (which i seem to have thought it would not)
  • just as a remark i remove itemTap event in CollectionView because it was simply not consistent. Now users simply needs to use tap event on templates. Same results, less complications

@CatchABus
Copy link
Contributor Author

CatchABus commented Apr 18, 2024

@CatchABus only issue I see is that it is not synced with isUserInteractionEnabled.if accessibility is disabled then focusable seems to not be linked to isUserInteractionEnabled

We still have the following scenario. We have this view:

<stacklayout></stacklayout>

Even though isUserInteraction is true by default, the native view of this layout will still be non-focusable on initialization.
So we have a lack of sync regardless of accessible value.
What we might need to check this correctly is:

this.isUserInteractionEnabled && this.nativeViewProtected.isFocusable()

So if we keep isUserInteractionEnabled and considering all the points above, it's possible to reduce changes to:

[accessibilityEnabledProperty.setNative](value) {
        // ensure `accessibilityEnabled=false` does not disable focus for view with `isUserInteractionEnabled=true`
        this.nativeViewProtected.setFocusable(!!value || this.isUserInteractionEnabled && this.nativeViewProtected.isFocusable());
        if (value) {
            updateAccessibilityProperties(this);
        }
    }

What do you think?

@CatchABus CatchABus marked this pull request as draft April 18, 2024 07:21
@CatchABus CatchABus marked this pull request as ready for review April 18, 2024 07:37
@farfromrefug
Copy link
Collaborator

@CatchABus You are right. Feels like a lot of things need to be fixed there.
So what about in the way you did it, store isFocusable default (in createNativeView cause initNativeView is too late as it can be modified by us), then use that default to know if we want to setFocusable when setting isUserInteractionEnabled/accessibilityEnabled to true (or anywhere else really). It could even be cached on a native class bases like Ammar did it for padding?
I still believe we should not go through accessibilityEnabledProperty.setNative for the default value. Will check this

@CatchABus CatchABus marked this pull request as draft April 18, 2024 08:26
@CatchABus CatchABus marked this pull request as ready for review April 18, 2024 15:36
@NathanWalker NathanWalker merged commit 03268cc into NativeScript:main Apr 19, 2024
3 checks passed
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.

None yet

3 participants