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

Significantly increase maxHeight of hover widget #242702

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

benibenj
Copy link
Contributor

@benibenj benibenj commented Mar 5, 2025

Copilot Generated Description: Increase the maximum height of the hover widget to 90% of the target window's inner height.

fixes #242377
cc @Tyriar

@benibenj benibenj self-assigned this Mar 5, 2025
@benibenj benibenj enabled auto-merge (squash) March 5, 2025 16:01
@vs-code-engineering vs-code-engineering bot added this to the March 2025 milestone Mar 5, 2025
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

This is a pretty scary change to do generally, I think we should instead have an option that allows the user to override. maxHeightRelativeToWindow=0.5 or something, there's probably a nicer name

@Tyriar
Copy link
Member

Tyriar commented Mar 5, 2025

An example of what could happen is the hover could be so high it obscures the target it's under. I have a similar problem with terminal tabs where there isn't enough horizontal space to show it fully on the size, so it obscures the terminal tab, making it impossible to click.

@benibenj
Copy link
Contributor Author

benibenj commented Mar 5, 2025

I see your point, to me it seemed more important to be able to read the hover content than to see the target which is why I suggested ths change. After thinking about this more. I think maxHeight should depend on the position of the target. If the target is at the bottom of the window, then maxHeight should be around 95% of the window, but when the target is in the center of the window, then it should only be around 45%. I think we should be solving this problem and not adding another option.

@Tyriar
Copy link
Member

Tyriar commented Mar 5, 2025

@benibenj I just don't want to deal with a long tail of "this hover is too big" issues that could come from this. Special casing hovers in the status bar by checking something like the target is within ~2px of bottom of screen and HoverPosition.ABOVE is used seems like a reasonable change provided there's a comment explaining it. At least it keeps the complexity inside the hover code.

@benibenj
Copy link
Contributor Author

benibenj commented Mar 5, 2025

What if we do not set a max height when the option forcePosition is used? We are already checking window boundaries in that case and the hover will not be rendered on top of the target. The statusbar could set it as it makes sense for it to have anyways

@Tyriar
Copy link
Member

Tyriar commented Mar 5, 2025

What if we do not set a max height when the option forcePosition is used?

forcePosition is about making sure it's anchored to that side though, whether the height is unbounded is a different concern. I see forcePosition is used for extensions and SCM history, there's no reason they should be allowed to be super large. Example of 50% height being a good thing:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hover with custom HTMLElement sets a fixed height, cutting off contents
2 participants