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

Terminal link tooltip location is wrong under the DOM renderer #50128

Closed
Tyriar opened this issue May 18, 2018 · 11 comments · Fixed by #59060
Closed

Terminal link tooltip location is wrong under the DOM renderer #50128

Tyriar opened this issue May 18, 2018 · 11 comments · Fixed by #59060
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented May 18, 2018

The underline also isn't appearing

screen shot 2018-05-18 at 12 12 49 pm

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues labels May 18, 2018
@Tyriar Tyriar added this to the May 2018 milestone May 18, 2018
@Tyriar Tyriar self-assigned this May 18, 2018
@Tyriar Tyriar changed the title Terminal link tooltip location is wrong for split terminals Terminal link tooltip location is wrong under the DOM renderer May 18, 2018
@Tyriar Tyriar modified the milestones: May 2018, Backlog May 18, 2018
@z3-io
Copy link

z3-io commented Jun 28, 2018

I also encountered this problem on Ubuntu 16.04, underline is not displayed on std input , and std output for Terminal. Hope can be fixed.

Version 1.24.1
Commit 24f6262
Date 2018-06-13T17:47:35.732Z
Shell 1.7.12
Renderer 58.0.3029.110
Node 7.9.0
Architecture x64

@Tyriar Tyriar added help wanted Issues identified as good community contribution opportunities good first issue Issues identified as good for first-time contributors labels Sep 12, 2018
@alexr00 alexr00 modified the milestones: Backlog, September 2018 Sep 19, 2018
@agurod42
Copy link
Contributor

agurod42 commented Sep 20, 2018

Hi!

After some testing I found that the problem (regarding tooltip location) seems to be that, when Linkifier emits the LinkHoverEventTypes.TOOLTIP event, the offsetX and offsetY properties of the fired MouseEvent instance are zero:

screenshot

I believe this is because the DOM element which fires the event is the span rendering the character instead of some element with the size of the entire terminal (like the .xterm-cursor-layer in the default renderer).

In order to solve it, I added pointer-events: none; to .xterm-rows and .xterm-rows span styles so the mousemove event don't target those objects and the .xterm-screen becomes the offsetX, offsetY provider.

@Tyriar, do you think it is a good solution? I can make a PR, if you want.

Edit: Those changes should be made on the vscode-xterm package (And maybe this issue should be there also?)

@agurod42
Copy link
Contributor

Regarding the underline don't appearing I'm trying this solution https://github.com/agurodriguez/xterm.js/commit/be4142d79582765600229103cb7a1e1bc2ac9058, and it seems to work (I'm going to sleep right now, but tomorrow I'll make some testing).

Do you think it's a feasible solution?

@alexr00
Copy link
Member

alexr00 commented Sep 20, 2018

@agurodriguez , we probably don't want to handle the tooltip location in xterm.js since xterm.js should just be providing all the info it has in the mouse event without making specific assumptions about how that info will be used. I'll let @Tyriar comment on the underline 😊

@Tyriar
Copy link
Member Author

Tyriar commented Sep 20, 2018

@agurodriguez I made some comments on the underline in that commit https://github.com/agurodriguez/xterm.js/commit/be4142d79582765600229103cb7a1e1bc2ac9058, looks pretty close to what we're after. I created xtermjs/xterm.js#1704 to track this, a PR would be fantastic! You would also want to remove the line in xterm.d.ts that says link underlines are not supported.

@agurod42
Copy link
Contributor

Underline don't appearing was fixed in xtermjs/xterm.js#1705

We should wait for xterm's 3.8.0 release in order to update vscode and close this issue, right?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 21, 2018

@agurodriguez I've already pulled in your changes, we'll close this off when #59060 is merged.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 26, 2018

Hmm, something is still a bit off, mousing over and pausing on the bottom few pixels will show the tooltip in the correct position but the pixels above will show the tooltip where it should be for the line above 🤔

image
image

@Tyriar Tyriar reopened this Sep 26, 2018
@Tyriar Tyriar modified the milestones: September 2018, October 2018 Sep 26, 2018
@Tyriar Tyriar removed good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities labels Sep 26, 2018
@agurod42
Copy link
Contributor

agurod42 commented Sep 30, 2018

I still have no idea why this is happening but this is what I got so far:

Target of the mouseover event in the expected behavior scenario:

The span right below the span where I had the pointer

screen shot 2018-09-30 at 12 21 27 am

Target of the mouseover event in the unexpected behavior scenario:

The actual span where I had the pointer

screen shot 2018-09-30 at 12 21 07 am

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Sep 30, 2018
Before the y from getCoords could be -1, a ceil call was also causing
a single pixel offset in each dimension

Related microsoft/vscode#50128
@Tyriar
Copy link
Member Author

Tyriar commented Sep 30, 2018

PR out upstream which should fix this: xtermjs/xterm.js#1716

@alexr00 alexr00 removed their assignment Oct 1, 2018
@Tyriar Tyriar modified the milestones: October 2018, Backlog Oct 25, 2018
@Tyriar Tyriar modified the milestones: Backlog, October 2019 Oct 8, 2019
@Tyriar Tyriar added upstream Issue identified as 'upstream' component related (exists outside of VS Code) intergrated-terminal-xtermjs labels Oct 8, 2019
@Tyriar
Copy link
Member Author

Tyriar commented Oct 10, 2019

I can no longer reproduce.

@Tyriar Tyriar closed this as completed Oct 10, 2019
@alexr00 alexr00 added the verified Verification succeeded label Oct 30, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug terminal Integrated terminal issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants