Skip ul and ol when highlighting selected markdown element (microsoft#161139)

Fixes microsoft#155552

For lists, the outer ul/ol always has the same source line as the first element in the list. We should prefer using the first element in the list when highlighting the active line

This also fixes a bug where scroll sync would stop working if you added lines to the doc. This was caused by `lineCount` getting out of sync as the document is being updated. I've removed this state and made the reveal logic more robust instead

Author: mjbvz

extensions/markdown-language-features/media/markdown.css

@@ -55,8 +55,8 @@ body.showEditorSelection .code-line {
 	position: relative;
 }
 
-body.showEditorSelection :not(tr).code-active-line:before,
-body.showEditorSelection :not(tr).code-line:hover:before {
+body.showEditorSelection :not(tr,ul,ol).code-active-line:before,
+body.showEditorSelection :not(tr,ul,ol).code-line:hover:before {
 	content: "";
 	display: block;
 	position: absolute;
@@ -65,6 +65,10 @@ body.showEditorSelection :not(tr).code-line:hover:before {
 	height: 100%;
 }
 
+.vscode-high-contrast.showEditorSelection  :not(tr,ul,ol).code-line .code-line:hover:before {
+	border-left: none;
+}
+
 body.showEditorSelection li.code-active-line:before,
 body.showEditorSelection li.code-line:hover:before {
 	left: -30px;
@@ -78,10 +82,6 @@ body.showEditorSelection li.code-line:hover:before {
 	border-left: 3px solid rgba(0, 0, 0, 0.40);
 }
 
-.vscode-light.showEditorSelection .code-line .code-line:hover:before {
-	border-left: none;
-}
-
 .vscode-dark.showEditorSelection .code-active-line:before {
 	border-left: 3px solid rgba(255, 255, 255, 0.4);
 }
@@ -90,10 +90,6 @@ body.showEditorSelection li.code-line:hover:before {
 	border-left: 3px solid rgba(255, 255, 255, 0.60);
 }
 
-.vscode-dark.showEditorSelection .code-line .code-line:hover:before {
-	border-left: none;
-}
-
 .vscode-high-contrast.showEditorSelection .code-active-line:before {
 	border-left: 3px solid rgba(255, 160, 0, 0.7);
 }
@@ -102,9 +98,6 @@ body.showEditorSelection li.code-line:hover:before {
 	border-left: 3px solid rgba(255, 160, 0, 1);
 }
 
-.vscode-high-contrast.showEditorSelection .code-line .code-line:hover:before {
-	border-left: none;
-}
 
 ul ul,
 ul ol,

extensions/markdown-language-features/preview-src/index.ts

@@ -87,6 +87,10 @@ onceDocumentLoaded(() => {
 			}
 		});
 	}
+
+	if (typeof settings.settings.selectedLine === 'number') {
+		marker.onDidChangeTextEditorSelection(settings.settings.selectedLine, documentVersion);
+	}
 });
 
 const onUpdateView = (() => {
@@ -110,7 +114,6 @@ window.addEventListener('resize', () => {
 }, true);
 
 window.addEventListener('message', async event => {
-
 	switch (event.data.type) {
 		case 'onDidChangeTextEditorSelection':
 			if (event.data.source === documentResource) {
@@ -235,7 +238,7 @@ document.addEventListener('dblclick', event => {
 	}
 
 	const offset = event.pageY;
-	const line = getEditorLineNumberForPageOffset(offset, documentVersion, settings);
+	const line = getEditorLineNumberForPageOffset(offset, documentVersion);
 	if (typeof line === 'number' && !isNaN(line)) {
 		messaging.postMessage('didClick', { line: Math.floor(line) });
 	}
@@ -284,7 +287,7 @@ window.addEventListener('scroll', throttle(() => {
 	if (scrollDisabledCount > 0) {
 		scrollDisabledCount -= 1;
 	} else {
-		const line = getEditorLineNumberForPageOffset(window.scrollY, documentVersion, settings);
+		const line = getEditorLineNumberForPageOffset(window.scrollY, documentVersion);
 		if (typeof line === 'number' && !isNaN(line)) {
 			messaging.postMessage('revealLine', { line });
 		}

extensions/markdown-language-features/preview-src/scroll-sync.ts

@@ -7,14 +7,6 @@ import { SettingsManager } from './settings';
 
 const codeLineClass = 'code-line';
 
-function clamp(min: number, max: number, value: number) {
-	return Math.min(max, Math.max(min, value));
-}
-
-function clampLine(line: number, lineCount: number) {
-	return clamp(0, lineCount - 1, line);
-}
-
 
 export interface CodeLineElement {
 	element: HTMLElement;
@@ -38,10 +30,13 @@ const getCodeLineElements = (() => {
 					// Fenched code blocks are a special case since the `code-line` can only be marked on
 					// the `<code>` element and not the parent `<pre>` element.
 					cachedElements.push({ element: element.parentElement as HTMLElement, line });
+				} else if (element.tagName === 'UL' || element.tagName === 'OL') {
+					// Skip adding list elements since the first child has the same code line (and should be preferred)
 				} else {
 					cachedElements.push({ element: element as HTMLElement, line });
 				}
 			}
+			console.log(cachedElements);
 		}
 		return cachedElements;
 	};
@@ -149,20 +144,17 @@ export function scrollToRevealSourceLine(line: number, documentVersion: number,
 	window.scroll(window.scrollX, Math.max(1, window.scrollY + scrollTo));
 }
 
-export function getEditorLineNumberForPageOffset(offset: number, documentVersion: number, settingsManager: SettingsManager) {
-	const lineCount = settingsManager.settings?.lineCount ?? 0;
+export function getEditorLineNumberForPageOffset(offset: number, documentVersion: number) {
 	const { previous, next } = getLineElementsAtPageOffset(offset, documentVersion);
 	if (previous) {
 		const previousBounds = getElementBounds(previous);
 		const offsetFromPrevious = (offset - window.scrollY - previousBounds.top);
 		if (next) {
 			const progressBetweenElements = offsetFromPrevious / (getElementBounds(next).top - previousBounds.top);
-			const line = previous.line + progressBetweenElements * (next.line - previous.line);
-			return clampLine(line, lineCount);
+			return previous.line + progressBetweenElements * (next.line - previous.line);
 		} else {
 			const progressWithinElement = offsetFromPrevious / (previousBounds.height);
-			const line = previous.line + progressWithinElement;
-			return clampLine(line, lineCount);
+			return previous.line + progressWithinElement;
 		}
 	}
 	return null;

extensions/markdown-language-features/preview-src/settings.ts

@@ -7,7 +7,8 @@ export interface PreviewSettings {
 	readonly source: string;
 	readonly line?: number;
 	readonly fragment?: string;
-	readonly lineCount: number;
+	readonly selectedLine?: number;
+
 	readonly scrollPreviewWithEditor?: boolean;
 	readonly scrollEditorWithPreview: boolean;
 	readonly disableSecurityWarnings: boolean;

extensions/markdown-language-features/src/preview/documentRenderer.ts

@@ -62,7 +62,8 @@ export class MdDocumentRenderer {
 		markdownDocument: vscode.TextDocument,
 		resourceProvider: WebviewResourceProvider,
 		previewConfigurations: MarkdownPreviewConfigurationManager,
-		initialLine: number | undefined = undefined,
+		initialLine: number | undefined,
+		selectedLine: number | undefined,
 		state: any | undefined,
 		token: vscode.CancellationToken
 	): Promise<MarkdownContentProviderOutput> {
@@ -72,7 +73,7 @@ export class MdDocumentRenderer {
 			source: sourceUri.toString(),
 			fragment: state?.fragment || markdownDocument.uri.fragment || undefined,
 			line: initialLine,
-			lineCount: markdownDocument.lineCount,
+			selectedLine,
 			scrollPreviewWithEditor: config.scrollPreviewWithEditor,
 			scrollEditorWithPreview: config.scrollEditorWithPreview,
 			doubleClickToSwitchToEditor: config.doubleClickToSwitchToEditor,

extensions/markdown-language-features/src/preview/preview.ts

@@ -310,8 +310,16 @@ class MarkdownPreview extends Disposable implements WebviewResourceProvider {
 		const shouldReloadPage = forceUpdate || !this.currentVersion || this.currentVersion.resource.toString() !== pendingVersion.resource.toString() || !this._webviewPanel.visible;
 		this.currentVersion = pendingVersion;
 
+		let selectedLine: number | undefined = undefined;
+		for (const editor of vscode.window.visibleTextEditors) {
+			if (this.isPreviewOf(editor.document.uri)) {
+				selectedLine = editor.selection.active.line;
+				break;
+			}
+		}
+
 		const content = await (shouldReloadPage
-			? this._contentProvider.renderDocument(document, this, this._previewConfigurations, this.line, this.state, this._disposeCts.token)
+			? this._contentProvider.renderDocument(document, this, this._previewConfigurations, this.line, selectedLine, this.state, this._disposeCts.token)
 			: this._contentProvider.renderBody(document, this));
 
 		// Another call to `doUpdate` may have happened.

extensions/markdown-language-features/src/preview/scrolling.ts

@@ -11,13 +11,20 @@ export function scrollEditorToLine(
 	line: number,
 	editor: vscode.TextEditor
 ) {
+	const revealRange = toRevealRange(line, editor);
+	editor.revealRange(revealRange, vscode.TextEditorRevealType.AtTop);
+}
+
+function toRevealRange(line: number, editor: vscode.TextEditor): vscode.Range {
 	const sourceLine = Math.floor(line);
+	if (sourceLine >= editor.document.lineCount) {
+		return new vscode.Range(editor.document.lineCount - 1, 0, editor.document.lineCount - 1, 0);
+	}
+
 	const fraction = line - sourceLine;
 	const text = editor.document.lineAt(sourceLine).text;
 	const start = Math.floor(fraction * text.length);
-	editor.revealRange(
-		new vscode.Range(sourceLine, start, sourceLine + 1, 0),
-		vscode.TextEditorRevealType.AtTop);
+	return new vscode.Range(sourceLine, start, sourceLine + 1, 0);
 }
 
 export class StartingScrollFragment {