xamarin / Xamarin.Forms

Xamarin.Forms is no longer supported. Migrate your apps to .NET MAUI.
https://aka.ms/xamarin-upgrade
Other
5.62k stars 1.87k forks source link

[Shell] Unable to focus and unfocus search handler for Android #7956

Open JeysonGustavo opened 5 years ago

JeysonGustavo commented 5 years ago

Description

The event focus is not called when search bar got focus and the same issue happens with unfocus for Android. A similar bug has been reported in the follow link https://github.com/xamarin/Xamarin.Forms/issues/5768. Despite of the fact that issue n° 5768 is closed at this moment it still happens in Android. I've tested in the Xaminals example project and even updating the dll of the xamarin.forms to the last version the problem remains.

Steps to Reproduce

  1. Start with search hidden
  2. Tap a search icon (toolbaritem or whatever) to open the search (expanded)
  3. Override in the SearchHandler class the OnFocused event or try to give a name in the control of the Shell.SearchHandler tag in XAML, in the code behind (xaml.cs) create the method protected void Element_Focused(object sender, EventArgs e). Neither the event nor the OnFocused is called when the search bar got focus.

Expected Behavior

The Event in code behind or OnFocused be called.

Actual Behavior

Cannot

Basic Information

It can be tested in the Xaminals project example, link to download https://docs.microsoft.com/en-us/samples/xamarin/xamarin-forms-samples/userinterface-xaminals/

ShellSerchFocusIssue.zip

Screenshots

The screenshots is attached.

Reproduction Link

https://github.com/JeysonGustavo/ShellSearchBarFocusIssue

kingces95 commented 5 years ago

Verified the following BP is not hit. Also, just typing in to the search bar is busted on the reproduction. Expected to be able to type more than one character but actually the second character does not appear.

image

germancoines commented 3 years ago

Hi @JeysonGustavo, I'm facing the exact same issue you described. @samhouts, since the status of this issue is inactive and tagged as 'help wanted', I'm having a look at it. @rmarinho, I've seen you added the EventHandlers Focused and Unfocused for the Xamarin.Forms.SearchHandler back in May of 2019. Any thoughts of yours on this will be highly appreciated :)

Keep in touch.

germancoines commented 3 years ago

I realize some of the code from SearchHandler.cs is really similar to the one within the VisualElement.cs. @rmarinho, my guess is that you wanted to implement the same approach from there.

Having a look at some other Xamarin.Forms.Core Controls, I see they often extend (either directly or Indirectly) the View.cs class, which directly extends from VisualElement and has the Focus and Unfocused EventHandlers.

So this arises my next question: @StephaneDelcroix , @rmarinho, do you happen to know (or remember) the reason why the SearchHandler isn't extending the VisualElement class? Also, could that be a good approach?

germancoines commented 3 years ago

I've just seen that Test cases which use a SearchHandler seem to be adding and setting it on the CodeBehind. Also, One of the test cases has the XAML declaration for the SearchHandler commented out.

image

I'm going to try and implement two new test cases:

germancoines commented 3 years ago

I can confirm now that with the current code on xamarin/Xamarin.Forms from branch 5.0.0, the Focused and Unfocused EventHandlers don't get fired in case they are declared on XAML.

Draft PR: #14375

germancoines commented 3 years ago

My previous 'suggested approach' on extending the VisualElement class may not be valid (and also a dangerous approach, in terms of impact). Going to inspect how Shell treats SearchHandler related property changes and events to find a better solution.

germancoines commented 3 years ago

Okay, got a fix that at least triggers the OnIsFocusedPropertyChanged within the SearchHandler any time the AppCompatAutoCompleteTextView _textBlock has a change on its focus status (see eb30d5c )

Actually, it even makes the XAML event declaration for a SearchHandler work, as well as definitely triggering the OnFocus and OnUnfocus overridable functions (which is the original issue described above).

image

Going to run some tests before changing the PR status from Draft to Request review.

germancoines commented 3 years ago

@samhouts, @kingces95, @StephaneDelcroix, @rmarinho I've set the PR #14375 as Ready to Review. Is there anything else expected to be done on my end? I couldn't get this issue assigned myself.

germancoines commented 3 years ago

Even the original Store Test now properly triggers OnFocused / OnUnfocused with PR #14375

image