[Windows] Respect `ClearButtonVisibility = ClearButtonVisibility.Never` for `<Entry>`s by MartyIX · Pull Request #23158 · dotnet/maui

MartyIX · 2024-06-20T11:47:14Z

Description of Change

The style interception code works sometimes but not always.

Relevant XAML template in WinUI 3 is here.

Issues Fixed

Fixes #23112
Related to #13714

PR that introduced the modified code: #3444

MartyIX · 2024-06-20T11:49:12Z

+				}
+				else
+				{
+					deleteButton.RenderTransform = new UI.Xaml.Media.TranslateTransform() { Y = -1000 };


It would be better to take into account parent's height, I guess. Or perhaps parent window height. Or something.

What happens if you have entries in a scroll view... Will it be a floating button somewhere?

First, one can modify src\Controls\samples\Controls.Sample.Sandbox\MainPage.xaml like this:

<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui" xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" x:Class="Maui.Controls.Sample.MainPage" xmlns:local="clr-namespace:Maui.Controls.Sample"> <VerticalStackLayout Background="Lightgray"> <ScrollView BackgroundColor="Gray" WidthRequest="300" HeightRequest="300" HorizontalScrollBarVisibility="Always" VerticalScrollBarVisibility="Always"> <Entry ClearButtonVisibility="Never" Text="Entry at your service" HeightRequest="30" Margin="0,1500,0,0" /> </ScrollView> </VerticalStackLayout> </ContentPage>

to test the issue. And yes, you are right that the button was visible there.

I modified it like this 627e084. I'm aware it's still a workaround but in practice, it should work well.

An additional step can be to set deleteButton.Opacity = 0. But I don't find it good as it costs some performance.

My conclusion is that this PR attempts to replace one workaround for another one (it is IMHO better). Hopefully, the WinUI team will provide a way to hide the button easily one day - that would be the best thing.

MartyIX · 2024-06-20T11:51:23Z

-				else if (!isEnabled && contains)
-					states.Remove(buttonVisibleState);
+				return;
 			}


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.

MartyIX · 2024-06-20T11:58:52Z

Could anyone run tests for this PR just to know if they pass or not?

MartyIX · 2024-06-27T20:22:59Z

@mattleibow What do you think please?

mattleibow · 2024-06-28T06:17:09Z

/rebase

mattleibow · 2024-06-28T06:18:03Z

/azp run

azure-pipelines · 2024-06-28T06:18:23Z

Azure Pipelines successfully started running 3 pipeline(s).

MartyIX · 2024-07-02T07:13:04Z

@mattleibow Does it look better to you now?

mattleibow · 2024-07-02T10:22:22Z

/azp run

azure-pipelines · 2024-07-02T10:22:41Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow · 2024-07-02T10:34:54Z

Are you able to add a UI test? I have a feeling this one is going to break randomly when we update wasdk.

MartyIX · 2024-07-02T12:08:45Z

Are you able to add a UI test?

I added 280f81c as the first commit so that one can easily check that the test fails without the new "workaround".

The test is partially done according to #23112 and its Steps to Reproduce.

I have a feeling this one is going to break randomly when we update wasdk.

Yes, I can imagine that.

mattleibow · 2024-07-02T13:19:04Z

/azp run

azure-pipelines · 2024-07-02T13:19:24Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow · 2024-07-02T13:19:30Z

Thanks for the test, now to bend CI to our will and ask it to do the basic job of running the tests properly.

MartyIX · 2024-07-03T09:00:44Z

So I can see that a screenshot must be added. Not sure if some other test genuinely failed.

mattleibow · 2024-07-03T19:20:15Z

You can download the screenshots from the devops test attachments. Just go to the failed test and the attachments tab. Should be there. It is the one without the nav bars and status bars.

mattleibow · 2024-07-03T19:29:22Z

I think the layout will not really work on mobile:

Android	iOS

https://dev.azure.com/xamarin/public/_build/results?buildId=118577&view=ms.vss-test-web.build-test-results-tab&runId=2906687&resultId=100119&paneView=attachments

But it seems to be right for Windows:

I wonder if it will work if you have jus a flay vertical stack layout?

MartyIX · 2024-07-04T13:56:12Z

Added a Windows snapshot. No snapshot for macOS so far.

PureWeen · 2024-07-04T17:15:10Z

+
+		await Task.Delay(500);
+
+		VerifyScreenshot();


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

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?

Given that I have a limited set of tries, I play it safe. :)

I attempted to change it in 67b3766.

Unfortunately, for some reason I'm seeing locally

Severity Code Description Project File Line Suppression State Error (active) NETSDK1047 Assets file 'D:\maui\artifacts\obj\Controls.TestCases.HostApp\project.assets.json' doesn't have a target for 'net8.0-ios/ios-arm64'. Ensure that restore has run and that you have included 'net8.0-ios' in the TargetFrameworks for your project. You may also need to include 'ios-arm64' in your project's RuntimeIdentifiers. Controls.TestCases.HostApp (net8.0-ios) C:\Program Files\dotnet\sdk\8.0.302\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets 266

So I can't run the test locally.

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

azure-pipelines · 2024-07-06T09:29:02Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow · 2024-07-10T04:48:31Z

Trying the tests on Windows Core unpackaged again... Probably not related since the packaged version ran. But just to be safe. Might just merge if it fails again and see if main fixes it.

mattleibow · 2024-07-10T05:10:45Z

Well, now it is green so whatever CI.

dotnet-policy-service Bot added the community ✨ Community Contribution label Jun 20, 2024

MartyIX commented Jun 20, 2024

View reviewed changes

MartyIX marked this pull request as ready for review June 20, 2024 11:49

MartyIX requested a review from a team as a code owner June 20, 2024 11:49

MartyIX requested review from jfversluis and tj-devel709 June 20, 2024 11:49

MartyIX commented Jun 20, 2024

View reviewed changes

MartyIX mentioned this pull request Jun 20, 2024

Calling Focus() on an Entry befor it has been displayed causes ClearButton to be displayed permanently #23112

Closed

github-actions Bot force-pushed the feature/2024-06-20-Hide-Entry-Clear-Button branch from f2eeab9 to 5193b28 Compare June 28, 2024 06:17

MartyIX force-pushed the feature/2024-06-20-Hide-Entry-Clear-Button branch from 83dc2c1 to 627e084 Compare June 28, 2024 11:48

PureWeen added the area-controls-entry Entry label Jun 28, 2024

MartyIX force-pushed the feature/2024-06-20-Hide-Entry-Clear-Button branch 2 times, most recently from 8cc9c2e to 5785a0a Compare July 2, 2024 12:05

MartyIX force-pushed the feature/2024-06-20-Hide-Entry-Clear-Button branch from 5785a0a to b16a52d Compare July 3, 2024 19:50

MartyIX force-pushed the feature/2024-06-20-Hide-Entry-Clear-Button branch 2 times, most recently from 0968321 to 4f014f0 Compare July 4, 2024 13:55

PureWeen reviewed Jul 4, 2024

View reviewed changes

MartyIX added 6 commits July 6, 2024 10:09

Add UI test

e562568

Hide clear button

0e6d9d4

Improve

e71a106

re-do test

24df556

Android & iOS & Windows snapshots

7f83071

feedback

67b3766

MartyIX force-pushed the feature/2024-06-20-Hide-Entry-Clear-Button branch from 4f014f0 to 67b3766 Compare July 6, 2024 08:37

PureWeen requested review from Foda and removed request for jfversluis and tj-devel709 July 6, 2024 15:01

MartyIX mentioned this pull request Jul 7, 2024

[Windows] Make WebView.CaptureAsync() work #23474

Merged

Foda approved these changes Jul 9, 2024

View reviewed changes

mattleibow approved these changes Jul 10, 2024

View reviewed changes

mattleibow merged commit 79f4c50 into dotnet:main Jul 10, 2024

MartyIX deleted the feature/2024-06-20-Hide-Entry-Clear-Button branch July 10, 2024 06:27

mattleibow mentioned this pull request Sep 3, 2024

Resolved Text Jump Issue in Entry Control with HorizontalTextAlignment Set to End #24485

Merged

github-actions Bot locked and limited conversation to collaborators Sep 8, 2024

Conversation

MartyIX commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Issues Fixed

Uh oh!

MartyIX Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

mattleibow Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

MartyIX Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartyIX Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

MartyIX commented Jun 20, 2024

Uh oh!

MartyIX commented Jun 27, 2024

Uh oh!

mattleibow commented Jun 28, 2024

Uh oh!

mattleibow commented Jun 28, 2024

Uh oh!

azure-pipelines Bot commented Jun 28, 2024

Uh oh!

MartyIX commented Jul 2, 2024

Uh oh!

mattleibow commented Jul 2, 2024

Uh oh!

azure-pipelines Bot commented Jul 2, 2024

Uh oh!

mattleibow commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartyIX commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattleibow commented Jul 2, 2024

Uh oh!

azure-pipelines Bot commented Jul 2, 2024

Uh oh!

mattleibow commented Jul 2, 2024

Uh oh!

MartyIX commented Jul 3, 2024

Uh oh!

mattleibow commented Jul 3, 2024

Uh oh!

mattleibow commented Jul 3, 2024

Uh oh!

MartyIX commented Jul 4, 2024

Uh oh!

PureWeen Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

MartyIX Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartyIX Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

MartyIX Jul 6, 2024

Choose a reason for hiding this comment

Uh oh!

MartyIX Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

azure-pipelines Bot commented Jul 6, 2024

Uh oh!

mattleibow commented Jul 10, 2024

Uh oh!

mattleibow commented Jul 10, 2024

Uh oh!

Reviewers

Assignees

Labels

MartyIX commented Jun 20, 2024 •

edited

Loading

MartyIX Jun 28, 2024 •

edited

Loading

mattleibow commented Jul 2, 2024 •

edited

Loading

MartyIX commented Jul 2, 2024 •

edited

Loading

MartyIX Jul 4, 2024 •

edited

Loading