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

Hover near top of document exceeds bounds of window #5227

Closed
1 task done
colin-grant-work opened this issue Sep 4, 2022 · 4 comments · Fixed by #9257
Closed
1 task done

Hover near top of document exceeds bounds of window #5227

colin-grant-work opened this issue Sep 4, 2022 · 4 comments · Fixed by #9257
Assignees
Labels
defect [core label] popovers Feedback for tooltips, syntax hints, info popups, etc

Comments

@colin-grant-work
Copy link

Check for existing issues

  • Completed

Describe the bug

A hover that is triggered near the top of the window extends beyond the top of the window, and there is no way to scroll the top of the hover into view.

To reproduce

  1. Trigger a hover near the top of the window
  2. Observe that you can't see the top of the hover.

Expected behavior

The hover should never extend beyond the window. It should either render above but be locked to the top of the window, or render below the item hovered on. Long hovers should be scrollable.

Environment

Zed 0.53.1 – /Applications/Zed.app
macOS 12.5.1
architecture x86_64

If applicable, add mockups / screenshots to help explain present your vision of the feature

image

If applicable, attach your ~/Library/Logs/Zed/Zed.log file to this issue

No response

@colin-grant-work colin-grant-work added defect [core label] triage Maintainer needs to classify the issue labels Sep 4, 2022
@Kethku
Copy link
Contributor

Kethku commented Sep 5, 2022

Note this is only the case because there is a diagnostic being hovered at the same location. The hover system has error diagnostics immediately, but needs to make an lsp request in order to acquire hover information. So we pick a side of the cursor to render at with just the error and then when the hover is available it gets rendered above.

In this case that causes an issue because the hover is too large.

One possible solution could be to just swap the side dynamically. We decided against this in order to avoid moving the popover after it has been rendered which looks janky. Another option might be to bias toward the bottom side more, but rendering the diagnostic below is not ideal in most cases.

The current behavior is a compromise where in most cases it gets it right but if you have a short diagnostic and a long hover and are pretty much at the top of the screen, it can get cut off. But in return you don't get popover jumping around the screen which I think might be a bigger win.

I'm interested in your thoughts though! We could maybe shrink the hover popover in this case which would at least make the content scrollable.

@colin-grant-work
Copy link
Author

Given the available options, it sounds like I'd probably opt for just guaranteeing that the hover doesn't exceed the window bounds by capping its size and making it scrollable. But if it's known ahead of time that there'll be a wait for an LSP response to get the hover, then it may make sense to wait to decide what to do with the hover (position and sizing) until the LSP response comes in. At that point, it sounds like its full render context and content is known, and an informed decision about which side to render it on and at what size can be made?

@Kethku
Copy link
Contributor

Kethku commented Sep 11, 2022

Could be. For now, the issue is rare enough that we will postpone fixing it in the short run. I'll think on it some more in the meantime.

@JosephTLyons JosephTLyons added editor Feedback for code editing, formatting, editor iterations, etc and removed triage Maintainer needs to classify the issue labels Oct 25, 2022
@JosephTLyons JosephTLyons transferred this issue from zed-industries/community Jan 24, 2024
@JosephTLyons
Copy link
Contributor

There's a screenshot of this in:

@JosephTLyons JosephTLyons added popovers Feedback for tooltips, syntax hints, info popups, etc and removed editor Feedback for code editing, formatting, editor iterations, etc labels Mar 9, 2024
ForLoveOfCats added a commit that referenced this issue Mar 13, 2024
)

Fixes #5227
Fixes #7304

Release Notes:

- Fixed an issue where not all editor hover popovers would be accounted
for when choosing to hover above or below the mouse cursor
([#5227](#5227)).
- Fixed an issue where editor hover could be dismissed by moving the
mouse between hover popovers
([#7304](#7304)).

Co-authored-by: Antonio Scandurra <antonio@zed.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect [core label] popovers Feedback for tooltips, syntax hints, info popups, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants