-
Notifications
You must be signed in to change notification settings - Fork 2k
[Mac]Clear button not hidden when text is cleared #34737
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
Changes from 9 commits
80d0547
36f1957
8f21605
2780371
44b8dbd
0a71d4e
5efc834
9fcf9bc
feaa537
a4adaad
e025752
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 34422, "SearchBar clear button still appears on MacCatalyst after clearing input", PlatformAffected.macOS)] | ||
| public class Issue34422 : ContentPage | ||
| { | ||
| readonly SearchBar _searchBar; | ||
|
|
||
| public Issue34422() | ||
| { | ||
| _searchBar = new SearchBar | ||
| { | ||
| Placeholder = "Search...", | ||
| AutomationId = "TestSearchBar" | ||
| }; | ||
|
|
||
| var addTextButton = new Button | ||
| { | ||
| Text = "Add Text", | ||
| AutomationId = "AddTextButton" | ||
| }; | ||
|
|
||
| addTextButton.Clicked += (s, e) => | ||
| { | ||
| _searchBar.Text = "Search text"; | ||
| }; | ||
|
|
||
| var clearButton = new Button | ||
| { | ||
| Text = "Clear SearchBar Text", | ||
| AutomationId = "ClearButton" | ||
| }; | ||
|
|
||
| clearButton.Clicked += (s, e) => | ||
| { | ||
| _searchBar.Text = string.Empty; | ||
| }; | ||
|
|
||
| Content = new VerticalStackLayout | ||
| { | ||
| Padding = new Thickness(20), | ||
| Spacing = 10, | ||
| Children = | ||
| { | ||
| _searchBar, | ||
| addTextButton, | ||
| clearButton | ||
| } | ||
| }; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue34422 : _IssuesUITest | ||
| { | ||
| public Issue34422(TestDevice device) : base(device) { } | ||
|
|
||
| public override string Issue => "SearchBar clear button still appears on MacCatalyst after clearing input"; | ||
|
|
||
| [Test, Order(1)] | ||
| [Category(UITestCategories.SearchBar)] | ||
| public void SearchBarClearButtonShouldBeVisibleWithText() | ||
| { | ||
| App.WaitForElement("TestSearchBar"); | ||
| App.Tap("TestSearchBar"); | ||
| App.Tap("AddTextButton"); | ||
| VerifyScreenshot(); | ||
| } | ||
|
Comment on lines
+15
to
+21
|
||
|
|
||
| [Test, Order(2)] | ||
| [Category(UITestCategories.SearchBar)] | ||
| public void SearchBarClearButtonShouldDisappearAfterClearingInput() | ||
| { | ||
| // First add text so the clear button appears | ||
| App.WaitForElement("TestSearchBar"); | ||
| App.Tap("TestSearchBar"); | ||
| App.Tap("AddTextButton"); | ||
| App.Tap("ClearButton"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Regression Prevention and Test Coverage — This regression test clears the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this fix does not use the remove/re-add approach for the native clear button, the proposed test coverage is not applicable to the current implementation. |
||
| VerifyScreenshot(); | ||
| } | ||
|
devanathan-vaithiyanathan marked this conversation as resolved.
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,6 @@ protected override MauiSearchBar CreatePlatformView() | |
|
|
||
| _editor = searchBar.GetSearchTextField(); | ||
|
|
||
|
devanathan-vaithiyanathan marked this conversation as resolved.
|
||
|
|
||
| return searchBar; | ||
| } | ||
|
|
||
|
|
@@ -154,6 +153,9 @@ internal static void MapSelectionLength(ISearchBarHandler handler, ISearchBar se | |
| public static void MapCancelButtonColor(ISearchBarHandler handler, ISearchBar searchBar) | ||
| { | ||
| handler.PlatformView?.UpdateCancelButton(searchBar); | ||
| if (handler is SearchBarHandler searchBarHandler) | ||
| handler.PlatformView?.UpdateClearButtonVisibility(!string.IsNullOrEmpty(searchBar.Text)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Handler Mapper and Property Patterns — The clear-button visibility refresh is wired through
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the suggestion, the Clear Button remains visible while the SearchBar is focused, which is not required. Therefore, not taking this suggestion, as the current implementation works correctly. |
||
|
|
||
|
devanathan-vaithiyanathan marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
153
to
159
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Handler Mapper and Property Patterns - This wires the MacCatalyst clear-button refresh through
devanathan-vaithiyanathan marked this conversation as resolved.
|
||
| internal static void MapSearchIconColor(ISearchBarHandler handler, ISearchBar searchBar) | ||
|
|
@@ -277,6 +279,7 @@ void OnEditingChanged(object? sender, EventArgs e) | |
| if (Handler is SearchBarHandler handler) | ||
| { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only updates the MacCatalyst clear button from native editing callbacks. The failing repro clears the managed |
||
| handler.UpdateCancelButtonVisibility(); | ||
| handler.PlatformView?.UpdateClearButtonVisibility(!string.IsNullOrEmpty(VirtualView?.Text)); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,6 +99,24 @@ public static void UpdateFont(this UISearchBar uiSearchBar, ITextStyle textStyle | |
| textField.UpdateFont(textStyle, fontManager); | ||
| } | ||
|
|
||
| internal static void UpdateClearButtonVisibility(this UISearchBar uiSearchBar, bool hasText) | ||
| { | ||
| if (OperatingSystem.IsMacCatalyst()) | ||
| { | ||
| var clearButton = uiSearchBar.GetClearButton(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This relies on private KVC ( |
||
|
|
||
| if (clearButton != null) | ||
| { | ||
| var shouldHide = !hasText; | ||
|
|
||
| if (clearButton.Hidden != shouldHide) | ||
| { | ||
| clearButton.Hidden = shouldHide; | ||
|
devanathan-vaithiyanathan marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Logic and Correctness — Toggling the private clear button's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the clear button only needs its visibility updated in this scenario, removing and re-adding it to the hierarchy is not required. The current visibility toggle approach is sufficient for the fix.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] iOS/macCatalyst Platform Specifics - Setting the private clear
devanathan-vaithiyanathan marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public static void UpdateVerticalTextAlignment(this UISearchBar uiSearchBar, ISearchBar searchBar) | ||
| { | ||
| uiSearchBar.UpdateVerticalTextAlignment(searchBar, null); | ||
|
|
@@ -460,5 +478,7 @@ static UITextPosition GetSelectionEnd(UITextField textField, UITextPosition star | |
| var end = textField.GetPosition(start, endOffset - startOffset); | ||
| return end ?? start; | ||
| } | ||
| internal static UIButton? GetClearButton(this UISearchBar searchBar) => | ||
| searchBar.GetSearchTextField()?.ValueForKey(new NSString("clearButton")) as UIButton; | ||
|
devanathan-vaithiyanathan marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Performance-Critical Path / Memory Leak Prevention — |
||
| } | ||
| } | ||
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.
[major] Regression Prevention and Test Coverage — These new screenshot assertions are strict
VerifyScreenshot()calls, but the PR branch still fails both Issue34422 MacCatalyst screenshots with visual baseline mismatches. The verified passing candidate refreshed the Mac baselines and used a small rendering tolerance (for exampleVerifyScreenshot(tolerance: 1)). Please update this assertion and the matching assertion later in the file so the regression test can actually pass reliably while still checking the clear-button state.