-
Notifications
You must be signed in to change notification settings - Fork 2k
[Windows] Respect ClearButtonVisibility = ClearButtonVisibility.Never for <Entry>s
#23158
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
Merged
mattleibow
merged 6 commits into
dotnet:main
from
MartyIX:feature/2024-06-20-Hide-Entry-Clear-Button
Jul 10, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Binary file added
BIN
+134 KB
....Android.Tests/snapshots/android/ValidateEntryClearButtonVisibilityBehavior.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
26 changes: 26 additions & 0 deletions
26
src/Controls/tests/TestCases.HostApp/Issues/Issue23158.xaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| xmlns:local="clr-namespace:Maui.Controls.Sample.Issues" | ||
| x:Class="Maui.Controls.Sample.Issues.Issue23158" | ||
| x:DataType="local:Issue23158"> | ||
|
|
||
| <VerticalStackLayout Padding="30,0"> | ||
| <Button AutomationId="AddEntry" Text="Add entry dynamically" Margin="0,0,0,30" Clicked="AddEntryButton_Clicked"/> | ||
|
|
||
| <!-- First entry. --> | ||
| <Label Text="Entry.ClearButtonVisibility=Never"/> | ||
| <Entry x:Name="Entry1" ClearButtonVisibility="Never" Text="A"/> | ||
|
|
||
| <!-- Second entry. --> | ||
| <Label Text="Entry.ClearButtonVisibility=WhileEditing"/> | ||
| <Entry x:Name="Entry2" ClearButtonVisibility="WhileEditing" Text="B"/> | ||
|
|
||
| <!-- Third entry. See #23112. --> | ||
| <Label Text="Dynamically Entry.ClearButtonVisibility=Never + Focus"/> | ||
| <VerticalStackLayout x:Name="Entry3Container"/> <!-- Container for a dynamically added entry. --> | ||
|
|
||
| <Label AutomationId="TestInstructions" Margin="0,30,0,0" FontAttributes="Bold" | ||
| Text="Instructions: Click the top-most button and the third entry should not contain any clear button!"/> | ||
| </VerticalStackLayout> | ||
| </ContentPage> |
26 changes: 26 additions & 0 deletions
26
src/Controls/tests/TestCases.HostApp/Issues/Issue23158.xaml.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [XamlCompilation(XamlCompilationOptions.Compile)] | ||
| [Issue(IssueTracker.Github, 23158, "Respect Entry.ClearButtonVisibility on Windows", PlatformAffected.UWP)] | ||
| public partial class Issue23158 : ContentPage | ||
| { | ||
| public Issue23158() | ||
| { | ||
| InitializeComponent(); | ||
| } | ||
|
|
||
| private void AddEntryButton_Clicked(object sender, EventArgs e) | ||
| { | ||
| Entry entry = new Entry() | ||
| { | ||
| Text = "Some Text", | ||
| AutomationId = "Entry3" | ||
| }; | ||
|
|
||
| Entry3Container.Children.Add(entry); | ||
|
|
||
| // Intentionally, after the entry is added to its layout container. | ||
| entry.ClearButtonVisibility = ClearButtonVisibility.Never; | ||
| entry.Focus(); | ||
| } | ||
| } |
31 changes: 31 additions & 0 deletions
31
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue23158.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue23158 : _IssuesUITest | ||
| { | ||
| public override string Issue => "Respect Entry.ClearButtonVisibility on Windows"; | ||
|
|
||
| public Issue23158(TestDevice device) : base(device) | ||
| { | ||
| } | ||
|
|
||
| #if !MACCATALYST | ||
| [Test] | ||
| [Category(UITestCategories.Entry)] | ||
| public void ValidateEntryClearButtonVisibilityBehavior() | ||
| { | ||
| App.WaitForElement("TestInstructions"); | ||
|
|
||
| // Click the button to add dynamically Entry3. | ||
| App.Click("AddEntry"); | ||
|
|
||
| // Click the new entry to see if there is the clear button or not. No such button should be present. | ||
| App.Tap("Entry3"); | ||
|
|
||
| VerifyScreenshot(); | ||
| } | ||
| #endif | ||
| } | ||
Binary file added
BIN
+17 KB
...es.WinUI.Tests/snapshots/windows/ValidateEntryClearButtonVisibilityBehavior.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+172 KB
...estCases.iOS.Tests/snapshots/ios/ValidateEntryClearButtonVisibilityBehavior.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| #nullable enable | ||
| using Microsoft.UI.Xaml; | ||
| using Microsoft.UI.Xaml; | ||
| using Microsoft.UI.Xaml.Controls; | ||
| using Microsoft.UI.Xaml.Media; | ||
|
|
||
| namespace Microsoft.Maui.Platform | ||
| { | ||
|
|
@@ -9,8 +9,6 @@ public static class MauiTextBox | |
| const string ContentElementName = "ContentElement"; | ||
| const string PlaceholderTextContentPresenterName = "PlaceholderTextContentPresenter"; | ||
| const string DeleteButtonElementName = "DeleteButton"; | ||
| const string ButtonStatesName = "ButtonStates"; | ||
| const string ButtonVisibleStateName = "ButtonVisible"; | ||
|
|
||
| public static void InvalidateAttachedProperties(DependencyObject obj) | ||
| { | ||
|
|
@@ -39,15 +37,17 @@ static void OnVerticalTextAlignmentPropertyChanged(DependencyObject d, Dependenc | |
|
|
||
| var scrollViewer = element?.GetDescendantByName<ScrollViewer>(ContentElementName); | ||
| if (scrollViewer is not null) | ||
| { | ||
| scrollViewer.VerticalAlignment = verticalAlignment; | ||
| } | ||
|
|
||
| var placeholder = element?.GetDescendantByName<TextBlock>(PlaceholderTextContentPresenterName); | ||
| if (placeholder is not null) | ||
| { | ||
| placeholder.VerticalAlignment = verticalAlignment; | ||
| } | ||
| } | ||
|
|
||
| // IsDeleteButtonEnabled | ||
|
|
||
| public static bool GetIsDeleteButtonEnabled(DependencyObject obj) => | ||
| (bool)obj.GetValue(IsDeleteButtonEnabledProperty); | ||
|
|
||
|
|
@@ -60,56 +60,25 @@ public static void SetIsDeleteButtonEnabled(DependencyObject obj, bool value) => | |
|
|
||
| static void OnIsDeleteButtonEnabledPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs? e = null) | ||
| { | ||
| // TODO: cache the buttonStates and buttonVisibleState values on the textBox | ||
|
|
||
| var element = d as FrameworkElement; | ||
|
|
||
| VisualStateGroup? buttonStates = null; | ||
| VisualState? buttonVisibleState = null; | ||
| var deleteButton = element?.GetDescendantByName<Button>(DeleteButtonElementName); | ||
| if (deleteButton?.Parent is Grid rootGrid) | ||
| (buttonStates, buttonVisibleState) = InterceptDeleteButtonVisualStates(rootGrid); | ||
|
|
||
| var states = buttonStates?.States; | ||
| if (element is not null && states is not null && buttonVisibleState is not null) | ||
| if (d is not FrameworkElement element) | ||
| { | ||
| var isEnabled = GetIsDeleteButtonEnabled(element); | ||
| var contains = states.Contains(buttonVisibleState); | ||
| if (isEnabled && !contains) | ||
| states.Add(buttonVisibleState); | ||
| else if (!isEnabled && contains) | ||
| states.Remove(buttonVisibleState); | ||
| return; | ||
| } | ||
|
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. It feels like this is a workaround as well. So if the approach of this PR is ok, then it would be a replacement of a workaround for a workaround. |
||
| } | ||
|
|
||
| static (VisualStateGroup? Group, VisualState? State) InterceptDeleteButtonVisualStates(FrameworkElement? element) | ||
| { | ||
| // not the content we expected | ||
| if (element is null) | ||
| return (null, null); | ||
|
|
||
| // find "ButtonStates" | ||
| var visualStateGroups = VisualStateManager.GetVisualStateGroups(element); | ||
| VisualStateGroup? buttonStates = null; | ||
| foreach (var group in visualStateGroups) | ||
| { | ||
| if (group.Name == ButtonStatesName) | ||
| buttonStates = group; | ||
| } | ||
|
|
||
| // no button states | ||
| if (buttonStates is null) | ||
| return (null, null); | ||
| Button? deleteButton = element.GetDescendantByName<Button>(DeleteButtonElementName); | ||
|
|
||
| // find and return the "ButtonVisible" state | ||
| foreach (var state in buttonStates.States) | ||
| if (deleteButton is not null) | ||
| { | ||
| if (state.Name == ButtonVisibleStateName) | ||
| return (buttonStates, state); | ||
| if (GetIsDeleteButtonEnabled(element)) | ||
| { | ||
| deleteButton.RenderTransform = null; | ||
| } | ||
| else | ||
| { | ||
| // This is a workaround to move the button to be effectively invisible. It is not perfect. | ||
| deleteButton.RenderTransform = new TranslateTransform() { X = -int.MaxValue, Y = -int.MaxValue }; | ||
| } | ||
| } | ||
|
|
||
| // no button visible state | ||
| return (null, null); | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
macOS won't generate screen shots so I would just ignore this test on macOS
Do we need the 500 ms delays?
VerifyScreenshot already does a delay and retry
Uh oh!
There was an error while loading. Please reload this page.
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 don't think that the delay before VerifyScreenshot() is necessary I just wanted to be sure.
The previous delay, I'm not sure. It's very hard for me to tell. I just find it safer with the delay. I guess you are concerned with the CI test time, right?
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.
Given that I have a limited set of tries, I play it safe. :)
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 attempted to change it in 67b3766.
Unfortunately, for some reason I'm seeing locally
So I can't run the test locally.
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.
Ok, I think that running tests can be fixed by this line in my other PR: https://github.com/dotnet/maui/pull/23474/files#diff-349e2d81d37196ffda15df237504c8df756b681fedeb65bb511e636f260e4dd6R75