Refactor PasswordBoxAssist.Password to use behaviors (issue 2930)

[nicolaihenriksen]

Fixes #2930

UPDATE: The modified UI tests run fine locally but seem to fail in the GitHub pipeline. I have no idea why.

As mentioned in the bug, there are certain scenarios where the current implementation falls short. This PR refactors the PasswordBoxAssist to use Microsoft.Xaml.Behaviors in order to handle the binding of the password. The code is heavily inspired by the MahApps.Metro implementation of such a behavior but adjusted to fit the needs of MDIX. One difference being that the revealed password is actually editable in the MDIX implementation whereas in MahApps it is only temporarily displayed (while mouse is down), and as such is read-only.

Important: This introduces a dependency, and as such, the MaterialDesignThemes.nuspec may need to be updated accordingly. I am no nuget-master, so I am unsure what the correct modifications would be.

Important: Since a lot of code was copied (and slightly adjusted) from the MapApps repository, there may be some copyright/disclaimer stuff that needs to be added somewhere?

Possible issue: Microsoft.Xaml.Behaviors has a different set of compatible target frameworks than what MDIX has. This was giving me some issues, but I managed to get it running locally. Not sure if there is an issue or not.

Improvements:

• Binding of the password now works in all cases (fingers crossed)
• Focus and text selection of the "reveal" TextBox in the reveal styles has been greatly improved, see GIFs below. Since behaviors were introduced, I was able to refactor this and have better control of what is happening.

Focus and text selection before:

Note that the I-beam is not visible immediately when revealing the password (although typing still appends to the text). Also note that text selection is "forgotten" when toggling between revealed/hidden.

Focus and text selection now:

[Keboo]

Sorry I missed your earlier comment about wanting help with the syntax here. I would set it up to allow for a range of value. Assuming the library is following semantic versioning (which it appears they are) we should be good to tell consumers anything from this version up to (but excluding) a 2.0 release. Like this:

Suggested change

	<dependency id="Microsoft.Xaml.Behaviors.Wpf" version="1.1.31" />
	<dependency id="Microsoft.Xaml.Behaviors.Wpf" version="[1.1.31, 2.0)" />

[nicolaihenriksen]

I was not really clear about what I meant here...

I think your suggestion to limit between a minimum version and a max version is reasonable. I think my original value meant that it should be "equal to or greater than" (ie. no upper limit). If you want a specific version, I think you put a single version in the hard brackets if I remember correctly.

Anyways, my concern was/is this:

We probably don't have a hard requirement on the latest version, we could compile against an older version in order to reach a broader audience (if they are locked into an older version of the same dependency for some reason). As such, I thought we could just put the smallest version in there which we know works.
However, my concern is regarding what the nuspec file says vs. what we actually compile against. If those two don't match (which dependabot would make sure was the case if we opted for an early version in the nuspec), I am under the impression that consumers of the library potentially would need a "binding redirect" even if they only have MDIX as a nuget package. I don't know this for sure, but I base the assumption on the fact that installing the nuget package, I would assume the nuget package manager simply pulls in the dependency in the (minimum) version that is spec'ed in the nuspec. This means the user ends up with an older version on disk, but MDIX compiled against (and thus expecting) a newer version; I believe this needs to be mitigated with a binding redirect.

You probably know more than I do about this, so please enlighten me :-)

[Keboo]

This is a great question. After pulling the docs for it I realized I was wrong to request an upper bound on the version as the recommendation from MS says to avoid that. I think we can also likely set our NuGet dependency to use a lower bound of [1.1.3](https://www.nuget.org/packages/Microsoft.Xaml.Behaviors.Wpf/1.1.3) and no upper bound (this was a mistake on my part with my earlier suggestion).

However on to your other question about how referenced assemblies work. First it is worth mentioning that the way .NET Framework runtime and the .NET Core Runtime resolve dependencies is a little different (largely around the use of the GAC; Global Assembly Cache). It is a common misconception that the version of the assembly that you compile against will be the same version of the assembly that your run against. When loading NuGet dependencies it uses the lower common dependency version. For projects that wish to have a newer version of the library that can explicitly reference it with the version that they would like, as long as this version doesn't cause an error with other packages (or the error is suppressed) the project will then load that NuGet dependency. This then means that NuGet's assemblies (anything under the lib directory in the NuGet that matches the target framework) will be copies to the projects output directory (ie. bin\Debug or similar). When the project is launched, the runtime then takes over and attempts to resolve the assembly (details in those two links above). However, for most NuGet packages, this ends up simply resolving to the assembly that is alongside the running application (the same one that was copied from the NuGet package). As long as there are no errors resolving the public API members being accessed, it is able to use whatever version of the library it finds.

[nicolaihenriksen]

@Keboo Thanks for the explanation. I am seemingly not very good at communicating my concern here :-) Reading the docs, I think they just confirm what I am trying to say (but failing to communicate). I am aware that you don't necessarily run against the same version of the dependent assembly as you compiled against; but it will attempt to bind with the exact version you compiled against unless "told otherwise" (eg. <bindingRedirect> in App.Config or another configuration file).

Say we have a project, P, which has 2 dependencies: A and B. Both of these assemblies have a dependency on C, but with different minimum versions (A requires min C.v1, and B requires min C.v2). Since A and B are compiled against different versions of C, project P needs to make a decision on which version of C to use. Typically, this ends up being handled by a <bindingRedirect>. In this example, the redirect would probably say that "for any requested version of C between v1 and v2, use v2". This would mean that dependency A, when resolving its dependency C will get C.v2 although it was compiled against and therefore is requesting C.v1. This is at least my understanding, and also something I often times need to deal with in my job.

Back to my concern; which is a slight variation of the above.
Say we have project P, which references MDIX. The MDIX nuget package indicates that the min required version of Microsoft.Xaml.Behaviors.Wpf is v1.1.3, however MDIX is compiled against v1.1.37 (latest). My concern is here that the nuget package manager will put v1.1.3 in the lib folder and copy that to the bin folder when compiling. To my understanding, the application P then needs a <bindingRedirect> in a configuration file in order to resolve Microsoft.Xaml.Behaviors.Wpf to v.1.1.3 when requesting v1.1.37 (which it was compiled against); this seems odd when simply pulling in a single nuget package.
This is why I believe we should also compile against the min version we state in the nuspec, and therefore avoid dependabot updates. Aagain, I am not 100% sure which version the nuget package manager pulls down and puts in the lib/bin folders, my assumption is that it is the one stated in the nuspec, but I could be wrong.

[nicolaihenriksen]

@Keboo I think the code in the PR is in a pretty good state now; pending any further review comments from you of course. However, the UI tests still fail on the PR, but the whole suite runs fine locally; I have no clue what is wrong...

[Keboo]

I will take a look at the failing tests to see what is going wrong

[nicolaihenriksen]

I will take a look at the failing tests to see what is going wrong

Seems I forgot to "save all" before committing the rename changes. Thanks for pushing the update. My UI test(s) still fail though...

[nicolaihenriksen]

@Keboo Looking at the produced screenshots of the failing test (Artifact "Screenshots-622"), it seems like the test window is minimized?!

[Keboo]

Yea.... it appears to either be offscreen or minimized. Will have to look more into it.

[nicolaihenriksen]

@Keboo I managed to get the UI test to pass in the pipeline by adding this little loaded event handler to the window:

private void BoundPasswordBoxWindow_OnLoaded(object sender, RoutedEventArgs e)
{
    Activate();
    Topmost = true;
    Topmost = false;
    Focus();
}

I really have no idea why that is necessary though.

[Keboo]

The temp windows that are created by XamlTest are being set to Topmost="True" so i am not surprised that change was required.

[nicolaihenriksen]

The temp windows that are created by XamlTest are being set to Topmost="True" so i am not surprised that change was required.

Could it be because this test actually uses "its own window"? There were other tests also doing that, so I didn't think twice about adding it. It could possibly be re-written to not require its own window, I just struggled getting a good handle to the ViewModel in order to test the binding updates when changing the value in the VM.

[Keboo]

Yea the custom window is exactly right. I should probably look at putting something in place so this automatically gets set on the window by default.