Merge pull request microsoft#208804 from microsoft/alexd/spiritual-bug

Avoid doing a hit test if the line is empty or if the mouse is after the line width

Author: alexdima

src/vs/editor/browser/controller/mouseTarget.ts

@@ -21,6 +21,7 @@ import { AtomicTabMoveOperations, Direction } from 'vs/editor/common/cursor/curs
 import { PositionAffinity } from 'vs/editor/common/model';
 import { InjectedText } from 'vs/editor/common/modelLineProjectionData';
 import { Mutable } from 'vs/base/common/types';
+import { Lazy } from 'vs/base/common/lazy';
 
 const enum HitTestResultType {
 	Unknown,
@@ -36,6 +37,9 @@ class UnknownHitTestResult {
 
 class ContentHitTestResult {
 	readonly type = HitTestResultType.Content;
+
+	get hitTarget(): HTMLElement { return this.spanNode; }
+
 	constructor(
 		readonly position: Position,
 		readonly spanNode: HTMLElement,
@@ -404,26 +408,53 @@ abstract class BareHitTestRequest {
 
 class HitTestRequest extends BareHitTestRequest {
 	private readonly _ctx: HitTestContext;
-	public readonly target: HTMLElement | null;
-	public readonly targetPath: Uint8Array;
+	private readonly _eventTarget: HTMLElement | null;
+	public readonly hitTestResult = new Lazy(() => MouseTargetFactory.doHitTest(this._ctx, this));
+	private _useHitTestTarget: boolean;
+	private _targetPathCacheElement: HTMLElement | null = null;
+	private _targetPathCacheValue: Uint8Array = new Uint8Array(0);
+
+	public get target(): HTMLElement | null {
+		if (this._useHitTestTarget) {
+			return this.hitTestResult.value.hitTarget;
+		}
+		return this._eventTarget;
+	}
 
-	constructor(ctx: HitTestContext, editorPos: EditorPagePosition, pos: PageCoordinates, relativePos: CoordinatesRelativeToEditor, target: HTMLElement | null) {
+	public get targetPath(): Uint8Array {
+		if (this._targetPathCacheElement !== this.target) {
+			this._targetPathCacheElement = this.target;
+			this._targetPathCacheValue = PartFingerprints.collect(this.target, this._ctx.viewDomNode);
+		}
+		return this._targetPathCacheValue;
+	}
+
+	constructor(ctx: HitTestContext, editorPos: EditorPagePosition, pos: PageCoordinates, relativePos: CoordinatesRelativeToEditor, eventTarget: HTMLElement | null) {
 		super(ctx, editorPos, pos, relativePos);
 		this._ctx = ctx;
+		this._eventTarget = eventTarget;
 
-		if (target) {
-			this.target = target;
-			this.targetPath = PartFingerprints.collect(target, ctx.viewDomNode);
-		} else {
-			this.target = null;
-			this.targetPath = new Uint8Array(0);
-		}
+		// If no event target is passed in, we will use the hit test target
+		const hasEventTarget = Boolean(this._eventTarget);
+		this._useHitTestTarget = !hasEventTarget;
 	}
 
 	public override toString(): string {
 		return `pos(${this.pos.x},${this.pos.y}), editorPos(${this.editorPos.x},${this.editorPos.y}), relativePos(${this.relativePos.x},${this.relativePos.y}), mouseVerticalOffset: ${this.mouseVerticalOffset}, mouseContentHorizontalOffset: ${this.mouseContentHorizontalOffset}\n\ttarget: ${this.target ? (<HTMLElement>this.target).outerHTML : null}`;
 	}
 
+	public get wouldBenefitFromHitTestTargetSwitch(): boolean {
+		return (
+			!this._useHitTestTarget
+			&& this.hitTestResult.value.hitTarget !== null
+			&& this.target !== this.hitTestResult.value.hitTarget
+		);
+	}
+
+	public switchToHitTestTarget(): void {
+		this._useHitTestTarget = true;
+	}
+
 	private _getMouseColumn(position: Position | null = null): number {
 		if (position && position.column < this._ctx.viewModel.getLineMaxColumn(position.lineNumber)) {
 			// Most likely, the line contains foreign decorations...
@@ -459,10 +490,6 @@ class HitTestRequest extends BareHitTestRequest {
 	public fulfillOverlayWidget(detail: string): IMouseTargetOverlayWidget {
 		return MouseTarget.createOverlayWidget(this.target, this._getMouseColumn(), detail);
 	}
-
-	public withTarget(target: HTMLElement | null): HitTestRequest {
-		return new HitTestRequest(this._ctx, this.editorPos, this.pos, this.relativePos, target);
-	}
 }
 
 interface ResolvedHitTestRequest extends HitTestRequest {
@@ -509,7 +536,7 @@ export class MouseTargetFactory {
 		const ctx = new HitTestContext(this._context, this._viewHelper, lastRenderData);
 		const request = new HitTestRequest(ctx, editorPos, pos, relativePos, target);
 		try {
-			const r = MouseTargetFactory._createMouseTarget(ctx, request, false);
+			const r = MouseTargetFactory._createMouseTarget(ctx, request);
 
 			if (r.type === MouseTargetType.CONTENT_TEXT) {
 				// Snap to the nearest soft tab boundary if atomic soft tabs are enabled.
@@ -528,24 +555,13 @@ export class MouseTargetFactory {
 		}
 	}
 
-	private static _createMouseTarget(ctx: HitTestContext, request: HitTestRequest, domHitTestExecuted: boolean): IMouseTarget {
+	private static _createMouseTarget(ctx: HitTestContext, request: HitTestRequest): IMouseTarget {
 
 		// console.log(`${domHitTestExecuted ? '=>' : ''}CAME IN REQUEST: ${request}`);
 
-		// First ensure the request has a target
 		if (request.target === null) {
-			if (domHitTestExecuted) {
-				// Still no target... and we have already executed hit test...
-				return request.fulfillUnknown();
-			}
-
-			const hitTestResult = MouseTargetFactory._doHitTest(ctx, request);
-
-			if (hitTestResult.type === HitTestResultType.Content) {
-				return MouseTargetFactory.createMouseTargetFromHitTestPosition(ctx, request, hitTestResult.spanNode, hitTestResult.position, hitTestResult.injectedText);
-			}
-
-			return this._createMouseTarget(ctx, request.withTarget(hitTestResult.hitTarget), true);
+			// No target
+			return request.fulfillUnknown();
 		}
 
 		// we know for a fact that request.target is not null
@@ -566,7 +582,7 @@ export class MouseTargetFactory {
 		result = result || MouseTargetFactory._hitTestMargin(ctx, resolvedRequest);
 		result = result || MouseTargetFactory._hitTestViewCursor(ctx, resolvedRequest);
 		result = result || MouseTargetFactory._hitTestTextArea(ctx, resolvedRequest);
-		result = result || MouseTargetFactory._hitTestViewLines(ctx, resolvedRequest, domHitTestExecuted);
+		result = result || MouseTargetFactory._hitTestViewLines(ctx, resolvedRequest);
 		result = result || MouseTargetFactory._hitTestScrollbar(ctx, resolvedRequest);
 
 		return (result || request.fulfillUnknown());
@@ -704,7 +720,7 @@ export class MouseTargetFactory {
 		return null;
 	}
 
-	private static _hitTestViewLines(ctx: HitTestContext, request: ResolvedHitTestRequest, domHitTestExecuted: boolean): IMouseTarget | null {
+	private static _hitTestViewLines(ctx: HitTestContext, request: ResolvedHitTestRequest): IMouseTarget | null {
 		if (!ElementPath.isChildOfViewLines(request.targetPath)) {
 			return null;
 		}
@@ -721,36 +737,41 @@ export class MouseTargetFactory {
 			return request.fulfillContentEmpty(new Position(lineCount, maxLineColumn), EMPTY_CONTENT_AFTER_LINES);
 		}
 
-		if (domHitTestExecuted) {
-			// Check if we are hitting a view-line (can happen in the case of inline decorations on empty lines)
-			// See https://github.com/microsoft/vscode/issues/46942
-			if (ElementPath.isStrictChildOfViewLines(request.targetPath)) {
-				const lineNumber = ctx.getLineNumberAtVerticalOffset(request.mouseVerticalOffset);
-				if (ctx.viewModel.getLineLength(lineNumber) === 0) {
-					const lineWidth = ctx.getLineWidth(lineNumber);
-					const detail = createEmptyContentDataInLines(request.mouseContentHorizontalOffset - lineWidth);
-					return request.fulfillContentEmpty(new Position(lineNumber, 1), detail);
-				}
-
+		// Check if we are hitting a view-line (can happen in the case of inline decorations on empty lines)
+		// See https://github.com/microsoft/vscode/issues/46942
+		if (ElementPath.isStrictChildOfViewLines(request.targetPath)) {
+			const lineNumber = ctx.getLineNumberAtVerticalOffset(request.mouseVerticalOffset);
+			if (ctx.viewModel.getLineLength(lineNumber) === 0) {
 				const lineWidth = ctx.getLineWidth(lineNumber);
-				if (request.mouseContentHorizontalOffset >= lineWidth) {
-					const detail = createEmptyContentDataInLines(request.mouseContentHorizontalOffset - lineWidth);
-					const pos = new Position(lineNumber, ctx.viewModel.getLineMaxColumn(lineNumber));
-					return request.fulfillContentEmpty(pos, detail);
-				}
+				const detail = createEmptyContentDataInLines(request.mouseContentHorizontalOffset - lineWidth);
+				return request.fulfillContentEmpty(new Position(lineNumber, 1), detail);
 			}
 
-			// We have already executed hit test...
-			return request.fulfillUnknown();
+			const lineWidth = ctx.getLineWidth(lineNumber);
+			if (request.mouseContentHorizontalOffset >= lineWidth) {
+				// TODO: This is wrong for RTL
+				const detail = createEmptyContentDataInLines(request.mouseContentHorizontalOffset - lineWidth);
+				const pos = new Position(lineNumber, ctx.viewModel.getLineMaxColumn(lineNumber));
+				return request.fulfillContentEmpty(pos, detail);
+			}
 		}
 
-		const hitTestResult = MouseTargetFactory._doHitTest(ctx, request);
+		// Do the hit test (if not already done)
+		const hitTestResult = request.hitTestResult.value;
 
 		if (hitTestResult.type === HitTestResultType.Content) {
 			return MouseTargetFactory.createMouseTargetFromHitTestPosition(ctx, request, hitTestResult.spanNode, hitTestResult.position, hitTestResult.injectedText);
 		}
 
-		return this._createMouseTarget(ctx, request.withTarget(hitTestResult.hitTarget), true);
+		// We didn't hit content...
+		if (request.wouldBenefitFromHitTestTargetSwitch) {
+			// We actually hit something different... Give it one last change by trying again with this new target
+			request.switchToHitTestTarget();
+			return this._createMouseTarget(ctx, request);
+		}
+
+		// We have tried everything...
+		return request.fulfillUnknown();
 	}
 
 	private static _hitTestMinimap(ctx: HitTestContext, request: ResolvedHitTestRequest): IMouseTarget | null {
@@ -1019,7 +1040,7 @@ export class MouseTargetFactory {
 		return position;
 	}
 
-	private static _doHitTest(ctx: HitTestContext, request: BareHitTestRequest): HitTestResult {
+	public static doHitTest(ctx: HitTestContext, request: BareHitTestRequest): HitTestResult {
 
 		let result: HitTestResult = new UnknownHitTestResult();
 		if (typeof (<any>ctx.viewDomNode.ownerDocument).caretRangeFromPoint === 'function') {