-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] Fixed keyboard flickering issue with SearchHandler #30166
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
[Android] Fixed keyboard flickering issue with SearchHandler #30166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the keyboard flickering issue by updating the SearchHandler.Query only when its new value differs from the current text field value. The changes prevent redundant updates on both iOS and Android platforms.
- On iOS, the UITextField is updated only if its current text differs from the new query.
- On Android, the EditText is similarly updated only when necessary.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/SearchHandlerAppearanceTracker.cs | Adds a null and equality check before updating UITextField.Text to prevent unnecessary updates. |
src/Controls/src/Core/Compatibility/Handlers/Shell/Android/SearchHandlerAppearanceTracker.cs | Implements an equivalent check by comparing _editText.Text with the new query value to avoid redundant assignments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain that with these changes, TextTransform
won't work. (bit of context here #30084 (comment))
Could you also verify if the keyboard issue occurs with InputView's (Entry
, Editor
, and SearchBar
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@bhavanesh2001 / @PureWeen, yes, after these changes, TextTransform is not working as expected. Before version I have now fixed it, but the issue reoccurs when TextTransform is applied, because the transformed text is reassigned to the control, which triggers the same keyboard flickering problem. I tested this behavior with For now, I have resolved the original issue, excluding the TextTransform scenario, which was a regression introduced in
|
3508f0f
to
b2fdb9d
Compare
@@ -106,12 +106,17 @@ void UpdateCharacterSpacing() | |||
|
|||
void UpdateText() | |||
{ | |||
string newText = _searchHandler.Query ?? string.Empty; | |||
|
|||
if (_editText.Text == newText) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think skipping it like this is okay, this is what we do with InputView's as well.
_editText.Text = _searchHandler.Query ?? string.Empty; | ||
|
||
UpdateTextTransform(); | ||
_editText.Text = _searchHandler.UpdateFormsText(newText, _searchHandler.TextTransform); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the Android Entry
implementation, and I think we can simplify the cursor management here by using SetTextKeepState()
.
Also I think we should call TextTransform only inside this method.
So we can update the method like this:
void UpdateText()
{
string newText = _searchHandler.UpdateFormsText(_searchHandler.Query ?? string.Empty, _searchHandler.TextTransform);
if (_editText.Text == newText)
{
return;
}
_editText.SetTextKeepState(newText);
}
This doesn’t cover an edge case, but that’s fine, our current implementation doesn’t handle it either.
For reference:
maui/src/Controls/src/Core/Platform/Android/Extensions/EditTextExtensions.cs
Lines 57 to 62 in b6d3543
if (oldText != newText) | |
{ | |
// This update is happening while inputting text into the EditText, so we want to avoid | |
// resettting the cursor position and selection | |
editText.SetTextKeepState(newText); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated it as suggested.
src/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellSearchView.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/SearchHandlerAppearanceTracker.cs
Show resolved
Hide resolved
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellPageRendererTracker.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bhavanesh2001 makes a lot of valid points
I think what we need to do for this work is synchronize code we've already written for InputView to the SearchHandler
All of these Same APIs already exist on Entry so we should be just running it all through the same methods and process
Each of the "Update methods" inside the SearchHandlerAppearanceTracker basically maps to the equivalent of an EntryHandler.
Can we just make it so SearchHandler implements ITextInput explicitly? (so we don't add any new APIs) and then we basically just reuse all the logic that's already inside the mappers to the SearchHandler?
The AfterText method on ShellSearchView.cs should mimic
void OnTextChanged(object? sender, TextChangedEventArgs e) inside EntryHandler.Android.cs
UpdateText inside SearchHandlerAppearanceTracker should mimic Entry.MapText
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Regression PR
Root Cause of the issue
Description of Change
Test Case
Issues Fixed
Fixes #30072
Tested the behaviour in the following platforms
Screenshot
Before-Fix.mp4
After-Fix.mp4