Skip to content

Conversation

@plewm
Copy link
Contributor

@plewm plewm commented Feb 6, 2025

Description of Change

Linked Issues

#2443

PR Checklist

Additional information

@plewm
Copy link
Contributor Author

plewm commented Feb 6, 2025

@dotnet-policy-service agree

@plewm
Copy link
Contributor Author

plewm commented Feb 6, 2025

This solution is not yet perfect. I could not find out yet, why the keypress event is also raised with key action up, although the key is long pressed. Perfect would be to have a solution for the long touch by spacebar as well. Maybe this also can be done in another PR.

In this implementation only normal touches can be done via space bar and holding the space bar pressed results in a lot of normal touches/clicks.

@TheCodeTraveler TheCodeTraveler marked this pull request as draft February 6, 2025 15:33
@plewm
Copy link
Contributor Author

plewm commented Feb 7, 2025

@dotnet-policy-service agree

@dotnet-policy-service agree

@plewm
Copy link
Contributor Author

plewm commented Feb 17, 2025

Can someone please verify those builds?

@plewm plewm marked this pull request as ready for review February 19, 2025 12:07
@plewm
Copy link
Contributor Author

plewm commented Feb 26, 2025

@ne0rrmatrix  do you have any idea, why that one test is failing. It should not have anything todo with my change or am I mistaken?

@ne0rrmatrix
Copy link
Member

@ne0rrmatrix  do you have any idea why that one Test is Failing. It should not have anything todo with my change or am I mistaken?

That test failing is completely unrelated to your PR. You can safely ignore it.

@plewm
Copy link
Contributor Author

plewm commented Feb 26, 2025

okay so, can anyone approve that than?

@plewm
Copy link
Contributor Author

plewm commented Mar 7, 2025

So how is this proceeding now? What can I do to get that thing merged finally?

@bijington
Copy link
Contributor

Your original comment said this solution isn't perfect does this mean you still want to improve it?

I do wonder whether we should add a mechanism to configure this new functionality in case keyboard support for a touch behavior isn't desired in all scenarios. What do you think?

@plewm
Copy link
Contributor Author

plewm commented Mar 7, 2025

Well the reason for introducing this at all is a new ruling of the EU that requires accessibility for all Apps starting middle of the year. Our App will also be tested by a specific 3rd party because of that. So in general, KeyBoard access needs to be available 100% in my opinion.
For the overall usecase the solution I introduced, would be fine for most cases. An ideal solution would also provide a long touch functionality for the touchbehavior, because that is a feature of the normal touch functionality.
I tried that out, but the keyDown event is just getting fired over and over and the properties of the event to detect, if it was hold or pushed again are not working as expected.

In regards to that my suggestion is, take that solution here for now, merge that to be fine for the general accessibility usecase and create another thread/issue for the long touch.

@bijington
Copy link
Contributor

Thanks. I'm not debating the value this adds to apps. I was debating whether developers would expect and always want something called TouchBehavior to provide keyboard support. To be honest I'd prefer a name like InteractionBehavior but that is a big breaking change.

My thought was we could introduce a property EnableKeyboardSupport, default it to true and handle your event subscriptions based on that property value. It would give developers a way to turn it off in case they don't need it.

As for the KeyDown being called continuously I have seen this recently while dealing with keyboard support for something else. Was this on iOS or macOS that you saw it? Let me see if I can dig out the issue

@plewm
Copy link
Contributor Author

plewm commented Mar 7, 2025

Well the KeyBoard will not interfere with the standard behavior, so it should not create any issues for those cases I think, but we can still discuss that usecase. What do you think would be a proper case where you absolutely want to forbid KeyBoard usage for such view component?

The KeyDown problem I found was happening on Android for me, but build on Mac. I did not research the iOS implementation deeper, cause the KeyBoard access there just worked as it was supposed to.

@bijington
Copy link
Contributor

I am happy to go with your suggestion of not including the property. It would become a much bigger PR if iOS, macOS and Windows already support this functionality. Thank you for the discussion

@bijington
Copy link
Contributor

As a quick further follow-up have you tried using any of the accessibility options on Android before trying this fix? I am not entirely familiar with TouchBehavior but I can see there is explicit support in the Android implementation to make use of the AccessibilityManager

@plewm
Copy link
Contributor Author

plewm commented Mar 10, 2025

you can take look into the bug ticket. I already explained the problem with the current implementation there:

#2443

The problem results exactly from that accessibilityManager and how it is used.

@bijington
Copy link
Contributor

you can take look into the bug ticket. I already explained the problem with the current implementation there:

#2443

The problem results exactly from that accessibilityManager and how it is used.

Sorry I missed that detail in the original report. If I am reading this correctly is the addition of this keyboard handling the right fix or should we be looking to improve how we are using the accessibilityManager? Also you mentioned that keyboard control works if TalkBack is enabled, what happens when you have both your keyboard handling and TalkBack enabled? You don't get 2 attempts to handle the interaction?

@ne0rrmatrix
Copy link
Member

you can take look into the bug ticket. I already explained the problem with the current implementation there:
#2443
The problem results exactly from that accessibilityManager and how it is used.

Sorry I missed that detail in the original report. If I am reading this correctly is the addition of this keyboard handling the right fix or should we be looking to improve how we are using the accessibilityManager? Also you mentioned that keyboard control works if TalkBack is enabled, what happens when you have both your keyboard handling and TalkBack enabled? You don't get 2 attempts to handle the interaction?

@bijington I believe the issue is about using the accessibility manager with touch. Looking at the bug report and the linked google docs it looks like touch should not use accessibility manager the way we are.

@plewm
Copy link
Contributor Author

plewm commented Mar 12, 2025

@bijington You will also not have 2 (clicks) registered. Its either if you have TalkBack on a double tap needed or with keyboard the space button. The google docs say to my understanding, that you should not depend on the accessibilityManager state at all to init different workflows.
My fix removes the keyBoard action from the accessibilityManager workflow. I think that is the more correct approach.

@bijington
Copy link
Contributor

@plewm thank you for this. Apologies for the delay in getting back to you on this.

Also it was great to meet you the other day at MAUI Day! I hope you enjoyed the day and managed to travel back safely.

Copy link
Contributor

@bijington bijington 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!

@plewm
Copy link
Contributor Author

plewm commented Mar 19, 2025

@bijington yes it was a nice event and also good to meet some people working on that framework here in person. Some of us stayed a little longer in London as well. We had a lot of fun :)

regarding the pipeline issue again...we cant merge yet because of the failing test of the "validationBehavior". How are fixing that? I would really like to get that thing on track here :D

@bijington
Copy link
Contributor

We have a flaky test :(. I have kicked off another build but don't worry it won't impact this being merged

@plewm
Copy link
Contributor Author

plewm commented Mar 20, 2025

@bijington uh that looks perfect now. we are ready to merge 🎉

who will be initiating the merge process? I can't do that it seems. I still have no button for that.
I first thought that the failing test was responsible, that I did not see the merge button.

anyways, thanks for bearing with me here :)

@bijington
Copy link
Contributor

Sorry I thought I had hit the auto merge button. It'll be done shortly after I post this comment. Thanks again for the help

@bijington bijington merged commit a473d5e into CommunityToolkit:main Mar 20, 2025
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Touchbehavior Keyboard Access Click not working on Android

3 participants