Skip to content

Fix horizontal ScrollBar placement for multi-line TextBox #3934

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.

Sign up for GitHub

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

Jump to bottom
Merged
Keboo merged 16 commits into from
Oct 4, 2025
Merged

Fix horizontal ScrollBar placement for multi-line TextBox #3934

Keboo merged 16 commits into from
Oct 4, 2025
+328 −89

Conversation

@nicolaihenriksen
Copy link
Contributor

@nicolaihenriksen nicolaihenriksen commented Sep 29, 2025 •
edited
Loading

Fixes #3933
Replaces PR #3931

This PR was done in collaboration with @database64128 who initially discovered the issue and created a PR (#3931) with an attempt to fix it.

The straight-forward solution to put the ScrollBar at the bottom, seems to be just to let the PART_ContentHost have VerticalAlignment=Stretch. While this "works", it screws up all sorts of other alignments with leading-/trailing-icons, prefix-/suffix-texts, clear button, etc.

So this PR is attempting to solve it differently. Namely by always hiding the built-in horizontal ScrollBar, and adding in a custom ScrollBar, which is fixated to the bottom of the control, and syncronized (in both directions) with the built-in ScrollBar (which is now always hidden).

The syncronization between the ScrollBars is done using a behavior, and the correct placement of the custom ScrollBar is done using a couple of converters taking all the needed parameters into account.

I have added the "visual breaking change" although in theory this only affects the multi-line scenario, and only fixes an issue.

EDIT: GitHub Copilots suggested changes were completely crap, but the per-file summary it provided is pretty good actually.

@nicolaihenriksen nicolaihenriksen added this to the 5.3.0 milestone Sep 29, 2025
@nicolaihenriksen nicolaihenriksen self-assigned this Sep 29, 2025
@nicolaihenriksen nicolaihenriksen added the visual breaking change Items here have affected the visual look of controls. label Sep 29, 2025
@nicolaihenriksen nicolaihenriksen marked this pull request as ready for review September 29, 2025 13:51
Copilot AI reviewed Sep 29, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a horizontal scrollbar positioning issue in TextBox controls where the horizontal scrollbar was not properly aligned at the bottom of multiline TextBoxes. The solution implements a custom horizontal scrollbar system that replaces the built-in scrollbar with better positioning control.

Key changes:

  • Replaces built-in horizontal scrollbar with custom synchronized scrollbar
  • Adds converters for proper scrollbar positioning and width calculation
  • Implements behavior for bidirectional synchronization between scrollbars

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
MaterialDesignTheme.TextBox.xaml Updates TextBox template to use custom horizontal scrollbar with proper Grid layout and synchronization behavior
TextBoxHorizontalScrollBarWidthConverter.cs New converter to calculate custom scrollbar width accounting for vertical scrollbar presence
TextBoxHorizontalScrollBarMarginConverter.cs New converter to calculate custom scrollbar margins considering icons, text, and border states
TextBoxHorizontalScrollBarBehavior.cs New behavior that synchronizes custom scrollbar with built-in hidden scrollbar
BoolToTextWrappingConverter.cs Demo converter for toggling text wrapping in demo application
Fields.xaml Updates demo with checkbox to test text wrapping and horizontal scrollbar functionality

@nicolaihenriksen nicolaihenriksen changed the title Text box horiz scroll bar issue Fix horizontal ScrollBar placement for multi-line TextBox Sep 29, 2025
@nicolaihenriksen nicolaihenriksen mentioned this pull request Sep 29, 2025
Closed
database64128
database64128 approved these changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@database64128 database64128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I can confirm that this PR in its current state fixes the bug for my use case, and does not cause any problems for the "Smart Hint" page in the demo app.

corvinsz
corvinsz approved these changes Oct 2, 2025
View reviewed changes
Copy link
Member

@corvinsz corvinsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I couldn't find anything wrong when testing manually.

@Keboo
Renames property for clarity.
239e61f 
Renames `isOutlinedStyle` to `hasOutlinedTextField` in `TextBoxHorizontalScrollBarMarginConverter` for better clarity and consistency.
@Keboo Keboo enabled auto-merge (squash) October 3, 2025 05:48
@Keboo
Improves UI test reliability
2a99dc4 
Adds retry logic to clear button click in date picker test; captures a screenshot of the split button popup during UI test.
@Keboo Keboo disabled auto-merge October 3, 2025 07:26
@Keboo
Copy link
Member

Keboo commented Oct 3, 2025

@nicolaihenriksen after reviewing this and the thread on the other PR with @database64128, I am fine letting this merge. Pushed a small naming cleanup and some UI test fixing.

@Keboo
Copy link
Member

Keboo commented Oct 4, 2025

@Keboo In your commit for UI test reliability, you added a this line, was that intentional? Doesn't that end up always producing a screenshot artifact (which is only intended for failing tests)?

SplitButtonTests.cs:97

await recorder.SaveScreenshot("PopupOpen");

Yes this was intentional and yes it will cause an image to always be created. That test is proving to be flaky on several PRs so I am fine with the image so that we can get it working properly.

@Keboo
Copy link
Member

Keboo commented Oct 4, 2025

@nicolaihenriksen after reviewing this and the thread on the other PR with @database64128, I am fine letting this merge. Pushed a small naming cleanup and some UI test fixing.

Thanks @Keboo! In your stream you also mentioned public vs. internal for the behavior and converters. I agree with you on the converters, they should probably be moved to the "internal" namespace but remain public. Since the behavior is also directly in use in our Style, should that be public as well then? And should we have an "internal" namespace inside the behaviors namespace as well (and move the behavior in there)?

UPDATE: I have pushed 2 commits moving the converters and behavior into "internal" namespaces.

Great, PR looks good to me. Will merge it.

@Keboo Keboo merged commit c26fcc5 into master Oct 4, 2025
2 checks passed
@Keboo Keboo deleted the textBoxHorizScrollBarIssue branch October 4, 2025 17:41
This was referenced Oct 6, 2025
Merged
Merged
Open
Merged
Merged
Merged
Open
Merged
Open
Open
Merged
Open
Merged
Open
Merged
Merged
This was referenced Oct 20, 2025
Closed
Closed
This was referenced Nov 1, 2025
Merged
Open
Open
Open
Open
@dependabot dependabot bot mentioned this pull request Nov 10, 2025
Closed
This was referenced Jan 3, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Keboo Keboo Keboo approved these changes

+2 more reviewers

@database64128 database64128 database64128 approved these changes

@corvinsz corvinsz corvinsz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nicolaihenriksen nicolaihenriksen

Labels

visual breaking change Items here have affected the visual look of controls.

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

Horizontal ScrollBar position in Multi-line TextBox is wrong

5 participants

@nicolaihenriksen @Keboo @database64128 @corvinsz