[RFC] New TextBox style

[nicolaihenriksen]

This draft PR is intended as a place for discussing the approach I am currently taking for the rework of the styles using SmartHint (TextBox being the first one). Sorry about the long write-up! Make sure you have a couple of Mountain Dews at hand before you dive in 😄

Scope

I think the scope of this discussion should not deal with the SmartHint as such, but mainly focus on the layout of the TextBox style.

What am I trying to solve/address

There have been a number of issues opened with elements inside the TextBox not aligning nicely (vertically). The current implementation uses a bunch of converters with "magic numbers" applied in order to attempt to align elements nicely for all/most scenarios. The new style should aim to get rid of all/most of these converters, and ideally not use any magic numbers.

Basic idea

Capture all the elements that should align with each other (vertically) in a single grid.

The blue box in the image above represents the single grid encapsulating all the elements of the TextBox that need to align vertically with each other. The red boxes marks each of the individual elements.
The challenge is that these elements, in various ways, can have their size individually controlled from the calling code. So the basic idea is to simply have VerticalAlignment=Center on all the elements with the red boxes, so they vertically align to the middle of the blue box regardless of their individual sizes. The blue box is the element that respects the TextBox.VerticalContentAlignment property so that it can align top/center/bottom. This works well for all variations of VerticalContentAlignment with the Stretch case, when the TextBox is also multi-line, being the exception. This is an edge case that I think we need to discuss, because I know people have different opinions on what they expect in this scenario.

The edge case

The edge case in question is, as mentioned, when the TextBox is multi-line (i.e. AcceptsReturn=True) and VerticalContentAlignment=Stretch. In this case the element containing the text that the user enters is expected to align to the top but be allowed to stretch downwards to fill up the space. In order to do this, the blue box would then have VerticalAlignment=Stretch applied to it, and ,as a minimum, the red box with the user content should also have VerticalAlignment=Stretch to fill the space vertically.
But what about the other elements? Should they also top align? And if so, they should probably (vertically) align nicely with the first row of text entered by the user? While this may seem like a good approach at first glance, it comes with a lot of issues to deal with. What if the icons/buttons have been set with non-default sizes, or the FontSize has been increased/decreased? Aligning such that those elements vertically center around the first row of text can become a complex calculation per element (i.e. red box). If calculations are not done, the result could look something like this (fixed height set on the TextBox in this example, but that is irrelevant) where you can see there really is no vertical alignment between the elements from the red boxes:

Personally I don't like this. If we were to align the elements vertically around the first row of text, I suspect we would end up with a bunch of complex converters, and possibly even some negative margins and ClipToBounds=False just to achieve what we want.

My suggestion is to always keep all other "red box" elements, apart from the actual user content (and possibly prefix-/suffix-texts) at VerticalAlignment=Center inside of the blue box. This would give a result similar to this.

Personally I don't think the prefix-/suffix-text should top-align in this scenario, but since they are TextBlock elements which inherit the same FontSize, making those align nicely with the first row of text would probably be doable (or even come for free).

So what am I hoping to achieve with this RFC?

I hope that I have provided enough details above, along with the code in the branch, to give the reader an understanding of how I intend to structure the new TextBox styles. I am looking for comments/feedback/pushback/critique; all is welcome.

This is just a first step, because the second issue that I want to address, is actually the placement of the SmartHint. This is also done using a bunch of converter magic with magic numbers and a lot of bindings making it complex to understand what is actually happening. But I doubt that one thing can be fixed without also making changes to the other, so I will most likely end up making a PR with changes for all the styles that use the SmartHint 😨 In that regard, should such a PR do the same as I am currently doing in this PR and create a parallel implementation rather than modifying the existing one? That definitely makes it easier to compare them, but is probably not ideal to actually push to master branch. Ideas?

If you run the code from the branch, you will open the demo app on the "Smart Hint" page where I have added the new TextBox style at the top for comparison with the default ones below it.

[Keboo]

So looked it over on my stream tonight.

To summarize my answers to some of the key areas. For the "blue box" elements, I am thinking that I agree that centering the leading icon, trailing icon, and clear button makes sense. I could see possibly doing an attached property to let someone control that, but that is not a requirement. I think I like the prefix/suffix text staying in line with the first line of the text; though I am also not sure a practical use-case for using them with multi-line text boxes. Looking over the MD3 spec, they don't even have examples that use multiple lines and those elements (in fact the multi-line examples also don't have the icons or clear buttons). It may be worth considering hiding those elements by default when AcceptsReturn=True. Obviously, this doesn't solve the problem (as I think we want to still leave the possibility of people using these together), but it does strongly hint to people the proper usage of the control and might mitigate some bug reports.

On the question about doing things side by side, I am fine with that while this is in progress. I will often just keep a copy of the released Demo app running to compare but having them side by side for now works. Before we merge, we will flatten down and remove the old one.

I also really liked getting the ripple stuff pulled up. I suspect this is going to reduce the number of elements in the template which should net a nice perf bump as well. Great work so far!

[nicolaihenriksen]

I also really liked getting the ripple stuff pulled up. I suspect this is going to reduce the number of elements in the template which should net a nice perf bump as well. Great work so far!

I can't really take credit for that part, it is also in the existing template 😄 So this initial work/RFC here was just to get a bit of clarity on how to proceed. Based on your feedback and your stream I think I have enough to go on. I will close this PR and continue working on other branches.

I have added a PR (#3470) with some preparation changes (i.e. demo app and some converter cleanup) which I hope to merge. I will then build on top of that for the SmartHint refactor (including the style changes from this PR). The intention is to keep the last part free of the "preparation stuff" to make the review a bit cleaner.