Fix TextBox horizontal scrollbar position when VerticalContentAlignment is set to Top

[database64128]

Will most likely be replaced by PR #3934.

---

Since #3486, if you have VerticalContentAlignment="Top" set on a TextBox, the horizontal scrollbar will not be at the bottom of the TextBox.

I'm demonstrating this bug by making some changes to the first outlined TextBox in the demo app:

• Added VerticalContentAlignment="Top"
• Disabled text wrapping.
• Enabled horizontal scrollbar.

I'm keeping these changes in the PR, because the second textbox already has the content centered and text wrapping enabled. Let me know if you prefer keeping the first TextBox as-is.

Before the fix:

After the fix:

The fix is fairly straightforward. VerticalContentAlignment was mistakenly bound to VerticalAlignment. The ScrollViewer should always stretch to fill the whole space.

I also tested the fix with VerticalContentAlignment="Bottom", and everything still looks as expected:

For context, my app uses a TextBox to display logs. #3486 broke my TextBox in 2 ways:

• Without AcceptsReturn="True", the TextBox now defaults to centering the text, which is certainly not what you want for displaying logs. I didn't have AcceptsReturn="True" on my TextBox before, because it already has IsReadOnly="True". This is kind of a breaking change in behavior, but I'm OK with keeping it as-is.
• After adding VerticalContentAlignment="Top", I encountered this exact bug, which this PR fixes.

[database64128]

It seems the failing test TextBox_MultiLineAndFixedHeight_RespectsVerticalContentAlignment is obsolete following the refactor in #3486. It was initially added for #3161, but my changes don't cause a regression there, as you can see from my screenshots.

In fact, my PR is very much doing the same thing as #3162, the PR fixing #3161. Looks like I missed the VerticalAlignment="Center" in SmartHint. I'm pushing a change to remove both that line and the test.

Let me know if you want this handled in another way.

[database64128]

After removing VerticalAlignment="Center" from SmartHint, the hint is now correctly positioned at the top of the text area, no matter what VerticalContentAlignment value the TextBox has.

[database64128]

/cc @nicolaihenriksen, the author of both #3162 and #3486.

[nicolaihenriksen]

@database64128 The fact that the horizontal scrollbar in some cases was not anchored at the bottom is definitely a bug, no discussion! Good catch and thanks for fixing it. (edit: not entirely sure this was actually a bug at all anymore, possibly more an accepted side-effect of the change. See my comments below for details).

Without AcceptsReturn="True", the TextBox now defaults to centering the text...

This was very much an intentional change, because there are SO MANY different configurations that you can use the TextBox in, I/we decided to cater for the most common use-case; non-multiline. So in cases where you want it multi-line, you will need to add AcceptsReturn=True, that was the trade-off as I recall it.

So when making changes like you have, changing the default position of the hint, please ensure that you check all the various combinations with TextFieldAssist.LeadingIcon, TextFieldAssist.TrailingIcon, TextFieldAssist.PrefixText, TextFieldAssist.SuffixText, font-size, and the various alignment options on the icons, prefix/suffix and hint text. The demo app has a page called Smart Hint which demonstrates the non-multiline variants with the ability for you to control a bunch of the mentioned properties and see the effect of the change for the various controls using the SmartHint. I suspect the change to the vertical alignment of the hint may cause some of that not to work properly anymore. Just a hunch though.

As a sidenote, Stretch is an unfortunate vertical alignment when mixing icons and text. Because for the TextBox, the text will top-align, while icons/buttons etc. will center-align.

[nicolaihenriksen]

@database64128 I have just tested your branch, and as I suspected, the TextBox behaves very strangely in some of the cases. In particular the case where you set a custom height (which some people do), and play around with the VerticalContentAlignment. As mentioned, the Smart Hint page gives you a way of testing a lot of these settings in combination.

It seems the failing test TextBox_MultiLineAndFixedHeight_RespectsVerticalContentAlignment is obsolete following the refactor in #3486. It was initially added for #3161, but my changes don't cause a regression there, as you can see from my screenshots.

The test is not obsolete (running green on main branch), your changes do indeed cause the regression (particularly because of the use of fixed height).

I am afraid that this change breaks more than it fixes.

[nicolaihenriksen]

@database64128 After reading your description thoroughly again, I believe that for your particular use-case, the only change you would need in your app, is to add AcceptsReturn=True on your TextBox. Since it is a log-viewer, you probably also want the IsReadOnly=True set.

Or am I missing something obvious?

[database64128]

After reading your description thoroughly again, I believe that for your particular use-case, the only change you would need in your app, is to add AcceptsReturn=True on your TextBox. Since it is a log-viewer, you probably also want the IsReadOnly=True set.

Or am I missing something obvious?

I did try that. With just AcceptsReturn=True and no VerticalContentAlignment="Top", the hint text is centered in the log viewer instead of at the top of the text area. The animation transitioning from empty to non-empty looks excessive and ridiculous, because the hint text has to travel a long distance.

This is the current (and expected) look when the log viewer is empty:

This is with just AcceptsReturn=True:

I actually just shipped a new release with the broken horizontal scrollbar position, because the first screenshot looked so much better than the second.

The log viewer in question: https://github.com/database64128/youtube-dl-wpf/blob/aea8d298dc36a6190660573a828bffaa55d98721/YoutubeDl.Wpf/Views/HomeView.xaml#L411-L421

[nicolaihenriksen]

@database64128 But if you want the hint (and icons etc.) to rest at the top, you definitely should set VerticalContentAlignment=Top as well as AcceptsReturn=True (since it is a multi-line text box).

So apparently, I still don't fully grasp the issue you're facing 🤔

[database64128]

But if you want the hint (and icons etc.) to rest at the top, you definitely should set VerticalContentAlignment=Top as well as AcceptsReturn=True (since it is a multi-line text box).

So apparently, I still don't fully grasp the issue you're facing 🤔

With both set, the horizontal scroll bar is at the wrong position.

[nicolaihenriksen]

@database64128 That definitely looks like a bug. However, I think the changes in this PR breaks a lot of other stuff, so let me see if I can work out a way to fix it without causing a regression.

Do you have a fixed height set on the TextBox or does it just fill its available space?

[database64128]

Do you have a fixed height set on the TextBox or does it just fill its available space?

Thanks. It's the latter. The log viewer is on a row with Height="*".

[database64128]

changing the default position of the hint

@nicolaihenriksen I just reverted that particular change. Can you please take another look?

In particular the case where you set a custom height (which some people do), and play around with the VerticalContentAlignment. As mentioned, the Smart Hint page gives you a way of testing a lot of these settings in combination.

With that change reverted, the Smart Hint page now looks as expected to me.

Edit: Nevermind, it's still broken in some combinations.

[database64128]

OK, I think I have finally fixed it. There are some minor behavioral differences in multi-line text boxes compared to the main branch:

• The hint text now always rests at the top. I think the new behavior is correct and looks better.
• IconVerticalAlignment is now actually respected. But if you set VerticalContentAlignment to something other than Stretch, you need to set IconVerticalAlignment as well.

I also un-obsoleted VerticalAlignmentConverter for use with the prefix and suffix text blocks.

@nicolaihenriksen Can you please take a look?

[database64128]

The test is not obsolete (running green on main branch), your changes do indeed cause the regression (particularly because of the use of fixed height).

I'm aware the test is still passing on the main branch. By obsolete, I meant that the test was no longer testing what it claims to be testing. The ContentGrid's VerticalAlignment clearly does not represent whether VerticalContentAlignment is respected.

[nicolaihenriksen]

I'm aware the test is still passing on the main branch. By obsolete, I meant that the test was no longer testing what it claims to be testing. The ContentGrid's VerticalAlignment clearly does not represent whether VerticalContentAlignment is respected.

With the change in this PR, I agree, the TextBox.VerticalContentAlignment is no longer respected at all. Everything is always centered for non-multi-line, and for multi-line it is now a strange mix.

As you can see the prefix/suffix, follow the text, the icons do not, nor does the clear button. I also think it is unfortunate with the current implementation that the scrollbar does not fill the width of the TextBox but only the width of the PART_ContentHost (not an issue you've introduced!).

I think the main issue we're facing here is the alignment of the icons, prefix/suffix, text and clear button should follow each other unless explicitly set otherwise. This is where issues arise with the PART_ContentHost as it would nice to have this always fill the available height (to display the optional scrollbar at the very bottom). However, doing that causes a lot of issues in having the text align with the other elements, and it does not address the width-issue mentioned above.

So my current approach (on a local branch), is to leave the code as-is with regards to alignment, but then always hide the built-in ScrollBar on the PART_ContentHost, and place another ScrollBar at the bottom of the TextBox control where you would expect it to be and syncronize its offset (in both directions) with the built-in one. Currently doing this with a Behavior<ScrollPresenter> hooking up some events to syncronize the offset. Last part I am missing is respecting an explicitly set visibility of TextBox.HorizontalScrollBarVisibility.

Here all the elements still align with each other (hardcoded to Center within the ContentGrid) while the TextBox.VerticalContentAlignment=Top is still respected AND the (custom) scroll bar now sits where you would expect it to.

[nicolaihenriksen]

@database64128 I have now pushed a branch called textBoxHorizScrollBarIssue which potentially solves the issue using my approach described above. Could you pull in this branch and see if it solves your particular issue? I will need approval from the primary maintainer (@Keboo) for a change like this. If it solves your issue, I'll create a PR and have him review it.

[database64128]

As you can see the prefix/suffix, follow the text, the icons do not, nor does the clear button.

This is because the icons now actually respect IconVerticalAlignment. I mentioned it in my earlier comment:

• IconVerticalAlignment is now actually respected. But if you set VerticalContentAlignment to something other than Stretch, you need to set IconVerticalAlignment as well.

I also think it is unfortunate with the current implementation that the scrollbar does not fill the width of the TextBox but only the width of the PART_ContentHost (not an issue you've introduced!).

It seems we disagree on this. 😄

The current behavior makes more sense to me, because the horizontal scrollbar won't scroll and hide the icons.

[nicolaihenriksen]

This is because the icons now actually respect IconVerticalAlignment...

The TextFieldAssist.IconVerticalAlignment has from its inception been intended to set alignment relative to the other elements hosted in the PART_ContentHost, not with respect to the TextBox control (that is what TextBox.VerticalContentAlignment is meant for). The reason it was necessary to introduce, was because of some use-cases where people use either large fonts, or large icons so there was a mismatch between the alignments; this property gave a bit more control over it.

The current behavior makes more sense to me, because the horizontal scrollbar won't scroll and hide the icons.

Oh it will not hide the icons, prefix/suffix or the clear button. My point is that the scrollbar is something "native" to the TextBox, so (to me) it feels weird to have it not fill the width just because we decided to decorate the control with a little extra elements. Even though it now fills the entire width, it still only scrolls the text.

[database64128]

I have now pushed a branch called textBoxHorizScrollBarIssue which potentially solves the issue using my approach described above. Could you pull in this branch and see if it solves your particular issue?

Played around for a bit:

[nicolaihenriksen]

@database64128 Hmm... Interesting. I think that is a timing issue, and I was maybe a bit bold simply placing a bang there. Apparently the SizeChanged can fire before Loaded it seems?! Try adding a guard for null and simply bail out. I think that should work. If not, we may need to invoke the same code when Loaded has fired.

UPDATE: Same problem for ScrollChanged I would assume.

[database64128]

My point is that the scrollbar is something "native" to the TextBox, so (to me) it feels weird to have it not fill the width just because we decided to decorate the control with a little extra elements. Even though it now fills the entire width, it still only scrolls the text.

Thanks for the explanation! Your point definitely also makes sense! I don't have strong feelings about this, as I don't have any use case where the horizontal scroll bar is enabled together with icons or prefix/suffix text.

[nicolaihenriksen]

@database64128 I have pushed two null-guard checks and an invocation of AssociatedObject_SizeChanged() in the Loaded event handler (which may not be needed).

As I cannot currently reproduce the crash, would you mind checking if the invocation is in fact needed or not? Thanks.

[database64128]

As I cannot currently reproduce the crash, would you mind checking if the invocation is in fact needed or not? Thanks.

The invocation does not appear to be necessary. Now that there's a null check and early bailout, don't forget to remove the null-forgiving operators below.

Could you pull in this branch and see if it solves your particular issue?

I can confirm that it solves my particular issue. However, there's now a new one:

The horizontal and vertical scrollbars no longer align when the TextBox is in focus. For reference, here's how it looked like before:

I'm also curious about the performance impact of this change. In my use case, the log viewer by default displays up to 1024 lines of log messages, and the user can choose to bump it higher.

[nicolaihenriksen]

@database64128 Thank you.

Yeah the dual-scrollbar issue I need to address. In fact once I simply get the correct (dynamic) margin I think the new approach will look better than the old where WPF has this weird thing with placing a white rectangle in the corner where the scrollbars meet. Anyway, I will look into fixing that part.

Regarding your performance concerns, I also have my concerns. However it is only a single extra control (ScrollBar) and some very simple logic connected to a limited set of events. I don't think it will have a major impact, but this is one of the reasons I would like @Keboo input on the matter; he is much deeper into WPFs inner workings than I am, so he probably has an opinion on the subject 😄.

[nicolaihenriksen]

@database64128 Hmm... Maybe I am changing my mind on the "full width horiz scrollbar" idea. Because with your use case where both are visible, things become really ugly when combined with other elements (icons, buttons, etc.)

I think I will pull it back in to only fill the width of the actual PART_ContentHost and ensure the corner alignment is correct...

[nicolaihenriksen]

@database64128 I have now pushed the changes to pull the custom horizontal scrollbar back into where the original horiz-scrollbar was placed. Hopefully that gives a better look for your use case?

[database64128]

@nicolaihenriksen With 6b40aa2:

The horizontal scrollbar is still off, and still moves a little when the TextBox transitions in or out of focus. Notice the few pixels of difference between the screenshots.

Also you forgot to remove the null-forgiving operator in UpdateTargetScrollBarVisibility(_builtInScrollBar!.Maximum > 0);. 😄

[nicolaihenriksen]

@database64128 Thanks again 😆

I have a hunch I know what is happening. I think it is taking icon- and prefixtext-margins into account even in the case where they are not visible. I'll fix this and push another update.

UPDATE: And good catch on the focus transition. That is because of the width of the border changing... That I also need to take into account 🤔

[nicolaihenriksen]

@database64128 Pushed 2 more commits. Cleanup of (hopefully the last remaining 😆) null-forgiving operators, and another commit which correctly deals with margins/visibility/hover-state/keyboard-focus; at least that is what I hope it does. It looks pretty good on my PC even though I am running a 150% DPI scaling so there may be some things that are slightly off. I did check with 100% as well, but I have been surprised before where there is a slight difference using other peoples setup (typically where DPI scaling is not at all in use) in contrast to mine.

I do have an office setup without DPI scaling, but I am currently not in the office...

[database64128]

@nicolaihenriksen Thanks! With the latest commit, everything now looks good for my use case. However, the horizontal scrollbar still moves around in the non-outlined variants:

I am running a 150% DPI scaling

Same here, let me guess, is it a 27-inch 4K monitor? 😄

[nicolaihenriksen]

@database64128 Haha, I knew there was something I had forgotten; compensation only needed for outlined style 😂

I'll fix that and create both an issue and a corresponding PR with my changes. Then we'll see what @Keboo thinks of it.

[nicolaihenriksen]

@database64128 Don't recall the actual size, but yes a 4K monitor 👍