-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[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
base: main
Are you sure you want to change the base?
Changes from all commits
80d0547
36f1957
8f21605
2780371
44b8dbd
0a71d4e
5efc834
9fcf9bc
feaa537
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(); | ||
| } | ||
|
|
||
| [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"); | ||
| 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(); | ||
|
|
||
|
|
||
| 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.
|
||
| } | ||
|
|
||
| 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. |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
| } | ||
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 — This regression test clears the
SearchBarthrough a helperButton, so it does not exercise the native MacCatalyst clear affordance whose stale rendering is being fixed. Add coverage that enters text and activates/verifies the native clear button path, or expose/assert the native clear-button state from the handler, so the test fails for the currentHidden-toggle implementation and passes for the hierarchy-refresh fix.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.
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.