Conversation
Tyriar
left a comment
There was a problem hiding this comment.
Do you know how this problem is solved in the editor?
Would be good to consolidate so they both do the same thing
|
|
| } | ||
|
|
||
| private adjustHoverMaxHeight(target: TargetRect): void { | ||
| let maxHeight = 250; |
There was a problem hiding this comment.
There's probably a better starting value than this, especially for larger screens.
| } | ||
|
|
||
| private adjustHoverMaxHeight(target: TargetRect): void { | ||
| let maxHeight = 250; |
There was a problem hiding this comment.
This is solving a different tracked issue, right? I think it should be out of scope of this PR as it will need more discussion, in particular I always thought that we would need to integrate a DomScrollableElement.
There was a problem hiding this comment.
This hoverWidget contains a base hoverWidget which has such an element. It's just that this hoverWidget clears out the maxHeight at https://github.com/microsoft/vscode/blob/main/src/vs/workbench/services/hover/browser/hoverWidget.ts#L231
And here's where this hoverWidget imports the other one that has the scrollbar https://github.com/microsoft/vscode/blob/main/src/vs/workbench/services/hover/browser/hoverWidget.ts#L14
There was a problem hiding this comment.
That pixel high might cause problems in some scenarios though, and it would need to be tested to make sure the hover correctly flips when real estate isn't there
| if (e.key !== 'Tab') { | ||
| this.hideHover(); | ||
| } |
There was a problem hiding this comment.
Instead of this, I think we should still be hiding the hover but only when the document.activeElement lies outside the hover. Otherwise this happens which doesn't see right:
There was a problem hiding this comment.
Actually, being able to tab into the hover seems pretty good, because I'm not sure how else a user would think to use the keyboard to navigate into the hover
I also think that a tab loop might help, though that is a more complicated design. I can split the maxHeight changes into a separate PR.
|
Closing in favour of #166657 |
Ref #159088