From 8f97a5a78635560a01f44098bc9f1f036a77159a Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Thu, 19 May 2022 15:30:12 -0700 Subject: [PATCH] Deflake registerLinkProvider tests The root cause of this flakiness problem was related to the click event handler. For some reason it would not go off on some links but it would when clicking elsewhere in the terminal. I tried a bunch of things to keep the click handler to no avail, instead opting to move to using mousedown and mouseup events for activating links which comes with the nice benefit of ensuring that the link on mouseup is the same one on mousedown. Fixes #3821 --- src/browser/Linkifier2.ts | 17 +++++++++++------ test/api/Terminal.api.ts | 28 +++++++++++++++++++--------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/browser/Linkifier2.ts b/src/browser/Linkifier2.ts index acbf1c93..dae9acfa 100644 --- a/src/browser/Linkifier2.ts +++ b/src/browser/Linkifier2.ts @@ -18,6 +18,7 @@ export class Linkifier2 extends Disposable implements ILinkifier2 { private _linkProviders: ILinkProvider[] = []; public get currentLink(): ILinkWithState | undefined { return this._currentLink; } protected _currentLink: ILinkWithState | undefined; + private _mouseDownLink: ILinkWithState | undefined; private _lastMouseEvent: MouseEvent | undefined; private _linkCacheDisposables: IDisposable[] = []; private _lastBufferCell: IBufferCellPosition | undefined; @@ -61,7 +62,8 @@ export class Linkifier2 extends Disposable implements ILinkifier2 { this._clearCurrentLink(); })); this.register(addDisposableDomListener(this._element, 'mousemove', this._onMouseMove.bind(this))); - this.register(addDisposableDomListener(this._element, 'click', this._onClick.bind(this))); + this.register(addDisposableDomListener(this._element, 'mousedown', this._handleMouseDown.bind(this))); + this.register(addDisposableDomListener(this._element, 'mouseup', this._handleMouseUp.bind(this))); } private _onMouseMove(event: MouseEvent): void { @@ -129,7 +131,7 @@ export class Linkifier2 extends Disposable implements ILinkifier2 { let linkProvided = false; // There is no link cached, so ask for one - this._linkProviders.forEach((linkProvider, i) => { + for (const [i, linkProvider] of this._linkProviders.entries()) { if (useLineCache) { const existingReply = this._activeProviderReplies?.get(i); // If there isn't a reply, the provider hasn't responded yet. @@ -156,7 +158,7 @@ export class Linkifier2 extends Disposable implements ILinkifier2 { } }); } - }); + } } private _removeIntersectingLinks(y: number, replies: Map): void { @@ -222,18 +224,21 @@ export class Linkifier2 extends Disposable implements ILinkifier2 { return linkProvided; } - private _onClick(event: MouseEvent): void { + private _handleMouseDown(): void { + this._mouseDownLink = this._currentLink; + } + + private _handleMouseUp(event: MouseEvent): void { if (!this._element || !this._mouseService || !this._currentLink) { return; } const position = this._positionFromMouseEvent(event, this._element, this._mouseService); - if (!position) { return; } - if (this._linkAtPosition(this._currentLink.link, position)) { + if (this._mouseDownLink === this._currentLink && this._linkAtPosition(this._currentLink.link, position)) { this._currentLink.link.activate(event, this._currentLink.link.text); } } diff --git a/test/api/Terminal.api.ts b/test/api/Terminal.api.ts index b1582be8..3a023d4a 100644 --- a/test/api/Terminal.api.ts +++ b/test/api/Terminal.api.ts @@ -792,6 +792,9 @@ describe('API Integration Tests', function(): void { describe('registerLinkProvider', () => { it('should fire provideLinks when hovering cells', async () => { await openTerminal(page, { rendererType: 'dom' }); + // Focus the terminal as the cursor will show and trigger a rerender, which can clear the + // active link + await page.evaluate('window.term.focus()'); await page.evaluate(` window.calls = []; window.disposable = window.term.registerLinkProvider({ @@ -811,6 +814,9 @@ describe('API Integration Tests', function(): void { it('should fire hover and leave events on the link', async () => { await openTerminal(page, { rendererType: 'dom' }); + // Focus the terminal as the cursor will show and trigger a rerender, which can clear the + // active link + await page.evaluate('window.term.focus()'); await writeSync(page, 'foo bar baz'); // Wait for renderer to catch up as links are cleared on render await pollFor(page, `document.querySelector('.xterm-rows').textContent`, 'foo bar baz '); @@ -846,6 +852,9 @@ describe('API Integration Tests', function(): void { it('should work fine when hover and leave callbacks are not provided', async () => { await openTerminal(page, { rendererType: 'dom' }); + // Focus the terminal as the cursor will show and trigger a rerender, which can clear the + // active link + await page.evaluate('window.term.focus()'); await writeSync(page, 'foo bar baz'); // Wait for renderer to catch up as links are cleared on render await pollFor(page, `document.querySelector('.xterm-rows').textContent`, 'foo bar baz '); @@ -886,18 +895,12 @@ describe('API Integration Tests', function(): void { it('should fire activate events when clicking the link', async () => { await openTerminal(page, { rendererType: 'dom' }); + // Focus the terminal as the cursor will show and trigger a rerender, which can clear the + // active link + await page.evaluate('window.term.focus()'); await writeSync(page, 'a b c'); - // Wait for renderer to catch up as links are cleared on render await pollFor(page, `document.querySelector('.xterm-rows').textContent`, 'a b c '); - - // Focus terminal to avoid a render event clearing the active link - const dims = await getDimensions(); - await moveMouseCell(page, dims, 5, 5); - await page.mouse.down(); - await page.mouse.up(); - await timeout(200); // Not sure how to avoid this timeout, checking for xterm-focus doesn't help - await page.evaluate(` window.calls = []; window.disposable = window.term.registerLinkProvider({ @@ -913,6 +916,7 @@ describe('API Integration Tests', function(): void { } }); `); + const dims = await getDimensions(); await moveMouseCell(page, dims, 3, 1); await pollFor(page, `window.calls`, ['provide 1', 'hover 1']); await page.mouse.down(); @@ -933,6 +937,9 @@ describe('API Integration Tests', function(): void { it('should work when multiple links are provided on the same line', async () => { await openTerminal(page, { rendererType: 'dom' }); + // Focus the terminal as the cursor will show and trigger a rerender, which can clear the + // active link + await page.evaluate('window.term.focus()'); await writeSync(page, 'foo bar baz'); // Wait for renderer to catch up as links are cleared on render await pollFor(page, `document.querySelector('.xterm-rows').textContent`, 'foo bar baz '); @@ -979,6 +986,9 @@ describe('API Integration Tests', function(): void { it('should dispose links when hovering away', async () => { await openTerminal(page, { rendererType: 'dom' }); + // Focus the terminal as the cursor will show and trigger a rerender, which can clear the + // active link + await page.evaluate('window.term.focus()'); await writeSync(page, 'foo bar baz'); // Wait for renderer to catch up as links are cleared on render await pollFor(page, `document.querySelector('.xterm-rows').textContent`, 'foo bar baz ');