Skip to content
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.

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

Prevent conhost scrollbar overlapping content #14329

Merged
1 commit merged into from
Nov 10, 2022

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Nov 3, 2022

Prior to this PR, the conhost vertical scrollbar would be forced to be
visible whenever the "Disable Scroll-Forward" option was set. It was
assumed that it would be needed as soon as the current viewport was
filled, so it was better to start off visible and disabled.

When the viewport height and buffer height are the same, though, the
scrollbar is never needed, and conhost compensates for that by making
the window narrower. But since we were still forcing the scrollbar to be
visible, that would result in it overlapping content in the rightmost
columns.

This PR attempts to fix that issue by simply leaving the scrollbar to
decide the visibility itself. This is perhaps not as aesthetically
pleasing when it starts off hidden and then later becomes visible, but
that seems better than having it overlap the content.

I've manually confirmed this fixes the problem reported in issue #2449.

Closes #2449

@ghost ghost added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase labels Nov 3, 2022
@j4james j4james marked this pull request as ready for review November 3, 2022 15:28
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

sure, seems sensible to me!

@DHowett
Copy link
Member

DHowett commented Nov 3, 2022

Just to confirm, this will cause a console window to grow by one scrollbar's width when (1) in TerminalScrolling mode and (2) it gains one more line than fits in the original viewport?

Will this cause a resize event if the console is maximized while this happens?

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

(Blocking for question)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 3, 2022
@j4james
Copy link
Collaborator Author

j4james commented Nov 3, 2022

Just to confirm, this will cause a console window to grow by one scrollbar's width when (1) in TerminalScrolling mode and (2) it gains one more line than fits in the original viewport?

No. The only time the size changes is when you switch the configuration from a layout that could potentially need a scrollbar (i.e. the buffer height is larger than the window height), and one that would not (i.e. the buffer height and window height are equal). This is the way it has always worked.

If you are in the former configuration, there will already be space allocated for the scrollbar, but it now won't be visible - there'll be a gap along the right hand border where it's expected to appear. Once you gain more content than can fit in that window, the scrollbar becomes visible, but nothing needs to be resized because we've already made space for it.

If you are in the latter configuration, there is no space allocated for the scrollbar, but it should never be shown now, so that's not an issue either.

The only thing we're changing here is the visibility of the scrollbar, which you can think of as an overlay. Whether it's visible or not doesn't affect the size of the window in any way. Does that make sense?

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 3, 2022
@DHowett
Copy link
Member

DHowett commented Nov 8, 2022

(Sorry, I was on vacation!)

OH. Damn. That makes perfect sense. Thanks for explaining it. 😄

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 10, 2022
@ghost
Copy link

ghost commented Nov 10, 2022

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 1b09ae3 into microsoft:main Nov 10, 2022
@j4james j4james deleted the fix-scrollbar-overlap branch December 26, 2022 20:51
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling "Disable Scroll-Forward" causes incorrect display of vertical scrollbar
4 participants