Fix markdown preview scrolling editor to wrong position on click#323250
Open
DakshUbhadia wants to merge 3 commits into
Open
Fix markdown preview scrolling editor to wrong position on click#323250DakshUbhadia wants to merge 3 commits into
DakshUbhadia wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an issue in the built-in Markdown extension where interacting with the Markdown preview (scrolling + then clicking back into the editor) could cause the editor to jump to an unrelated position due to stale preview→editor scroll-sync messages.
Changes:
- Adds an extension-host-side suppression window (
#isSyncingFromEditorSelection) to ignore previewrevealLinemessages shortly after editor selection changes. - Updates the preview webview’s
scrollDisabledCountguarding logic to better cover async scroll timing (images-loaded delay + programmatic scroll).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| extensions/markdown-language-features/src/preview/preview.ts | Adds a selection-change sync suppression flag and timer cleanup to avoid reacting to leaked scroll-sync messages. |
| extensions/markdown-language-features/preview-src/index.ts | Adjusts the webview-side scroll event suppression logic around programmatic scrolls and editor-selection updates. |
Comment on lines
+158
to
+164
| scrollDisabledCount++; | ||
| doAfterImagesLoaded(() => { | ||
| scrollToRevealSourceLine(line, documentVersion, settings); | ||
| if (scrollDisabledTimer) { clearTimeout(scrollDisabledTimer); } | ||
| scrollDisabledTimer = window.setTimeout(() => { | ||
| scrollDisabledCount = Math.max(0, scrollDisabledCount - 1); | ||
| }, 100); |
Comment on lines
+252
to
+256
| scrollDisabledCount++; | ||
| if (scrollDisabledTimer) { clearTimeout(scrollDisabledTimer); } | ||
| scrollDisabledTimer = window.setTimeout(() => { | ||
| scrollDisabledCount = Math.max(0, scrollDisabledCount - 1); | ||
| }, 300); |
Comment on lines
+372
to
+374
| if (this.#isSyncingFromEditorSelection) { | ||
| return; | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #323245
What this PR does
When a
.mdfile is open in split view with the preview, clicking backinto the editor after scrolling the preview causes the editor to jump to
a completely unrelated scroll position. Text between the previous cursor
position and the click target is also spuriously selected.
Root cause
The
scrollEditorWithPreviewfeature works by having the previewwebview post a
revealLinemessage on every scroll event. AscrollDisabledCountguard in the webview is supposed to suppressscroll events that result from programmatic scrolls (to prevent feedback
loops). However the guard had two problems:
Webview side (
preview-src/index.ts): WhenonDidChangeTextEditorSelectionfires, the preview callsscrollToRevealSourceLineinsidedoAfterImagesLoaded— an asyncPromise. The existing guard timers (50ms) expire before
window.scrollToactually fires because the async image-load delaypushes the scroll past the guard window. The resulting scroll DOM
event is not suppressed, so a stale
revealLinemessage is posted tothe extension host, which calls
editor.revealLineat the wrongposition.
Extension host side (
src/preview/preview.ts): There was noguard in
onDidScrollPreviewto detect that an incomingrevealLinemessage originated from an editor selection change rather than a
genuine user scroll of the preview.
Changes
preview-src/index.ts— In theonDidChangeTextEditorSelectionmessage handler, increment
scrollDisabledCountwith a 300ms guardwindow. This covers the full async path of
doAfterImagesLoaded+window.scrollTo+ one event loop tick. The counter is decrementedusing
Math.max(0, count - 1)to prevent it going negative if timersrace.
src/preview/preview.ts— Add a#isSyncingFromEditorSelectionflag that is set for 400ms whenever
onDidChangeTextEditorSelectionfires (called via the new
notifyEditorSelectionChanged()method beforethe message is posted). In
onDidScrollPreview, return early when thisflag is active. This is a defensive second layer in case a scroll
message leaks through the webview-side guard due to platform timing
differences.
clearTimeout(#editorSelectionSyncTimer)is added todispose()to prevent timer leaks.How to test
.mdfile with enough content to scroll (150+ lines).Ctrl+K Vto open the preview in split view.Before this fix: the editor jumps to the position the preview just
scrolled to, and text from the original cursor position to the click
target is selected.
After this fix: the cursor lands cleanly at the clicked position
with no spurious scroll or selection.