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

Change terminal link hover widget position to be consistent #83175

Merged
merged 8 commits into from Oct 25, 2019

Conversation

@jmbockhorst
Copy link
Contributor

jmbockhorst commented Oct 23, 2019

The PR changes the terminal link tooltip position to be consistent with the rest of the editor. This is part of #78647, and is the first step for #77964.

Currently, the tooltip position is where the mouse position is, and it sometimes creates unexpected behaviors.

linkBefore

With the pull request, the tooltip position is based on where the link position is.

linkAfter

To do this, I had to change the terminalWidgetManager to render Widgets using bottom position, since the terminal is snapped to the bottom of the container and has no knowledge about the extra space at the top between the actual terminal and the container. Since link tooltips are the only thing I found that uses the terminalWidgetManager, I figured it would be okay. I also added a new parameter that controls whether or not the bottom position is to the top or bottom of the widget.

I tested it for both renderers and with increased letter spacing and line heights.

@Tyriar Tyriar added this to the October 2019 milestone Oct 23, 2019
@Tyriar Tyriar self-assigned this Oct 23, 2019
@jmbockhorst jmbockhorst changed the title Change link hover widget position to be consistent Change terminal link hover widget position to be consistent Oct 23, 2019
jmbockhorst and others added 2 commits Oct 23, 2019
@@ -43,13 +43,13 @@ export class TerminalWidgetManager implements IDisposable {
mutationObserver.observe(this._xtermViewport, { attributes: true, attributeFilter: ['style'] });
}

public showMessage(left: number, top: number, text: string): void {
public showMessage(left: number, bottom: number, text: string, positionIsTopOfWidget: boolean = false): void {

This comment has been minimized.

Copy link
@Tyriar

Tyriar Oct 23, 2019

Member

This could be a little nicer is bottom was renamed to y and we used something like this:

enum VerticalAlignment {
  Bottom,
  Top
}
let offsetRow = this._xterm.rows - location.start.row + 1;
let useTopPosition = false;

// Handle a link on the top row

This comment has been minimized.

Copy link
@Tyriar

Tyriar Oct 23, 2019

Member
Suggested change
// Handle a link on the top row
// Show the tooltip on the top of the next row to avoid obscuring the first row
if (this._configHelper.config.rendererType === 'dom') {
const target = (e.target as HTMLElement);
this._widgetManager.showMessage(target.offsetLeft, target.offsetTop, this._getLinkHoverString());
const font = this._configHelper.getFont();
const charWidth = font.charWidth;
const charHeight = font.charHeight;

const leftPosition = (location.start.col - 1) * (charWidth! + (font.letterSpacing / window.devicePixelRatio));
const bottomPosition = offsetRow * (charHeight! * font.lineHeight);

this._widgetManager.showMessage(leftPosition, bottomPosition, this._getLinkHoverString(), useTopPosition);
Comment on lines 107 to 115

This comment has been minimized.

Copy link
@Tyriar

Tyriar Oct 23, 2019

Member

Got it in this state with the dom renderer when zoomed in (ctrl/cmd++)

image

@jmbockhorst

This comment has been minimized.

Copy link
Contributor Author

jmbockhorst commented Oct 24, 2019

Thanks for the feedback. I made the requested changes and fixed the issue when zoomed.

jmbockhorst and others added 2 commits Oct 24, 2019
@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented Oct 24, 2019

The bottom row doesn't get positions correctly:

image

this._domNode.style.bottom = `${Math.max(_y, WIDGET_HEIGHT) - WIDGET_HEIGHT}px`;
} else {
// Y position is to the bottom of the widget
this._domNode.style.bottom = `${Math.min(Math.max(_y, WIDGET_HEIGHT), _container.offsetHeight - WIDGET_HEIGHT)}px`;

This comment has been minimized.

Copy link
@Tyriar

Tyriar Oct 24, 2019

Member

Looks like this is the problem, we need the row height to be passed through and used for the very bottom row.

This comment has been minimized.

Copy link
@jmbockhorst

jmbockhorst Oct 24, 2019

Author Contributor

We don't actually want to check the max of _y and WIDGET_HEIGHT when y is to the bottom of the widget, since even y = 0 will still be in the viewport.

@Tyriar
Tyriar approved these changes Oct 25, 2019
Copy link
Member

Tyriar left a comment

Works great, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.