Preserve ancestor scroll positions when focusing the native edit context#324763
Open
nstrayer wants to merge 1 commit into
Open
Preserve ancestor scroll positions when focusing the native edit context#324763nstrayer wants to merge 1 commit into
nstrayer wants to merge 1 commit into
Conversation
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 #324758
If you try and embed monaco editors in scrollable documents things can get a bit funky. We noticed this in Positron and fixed it there but if other VSCode forks or new vscode editors also end up embedding editors in native scrollable documents in the future the issues will show up so I figured an upstream PR was worth it to get ahead of those issues and save headaches for future people who get super confused about this scrolling behavior like I did!
Agent generated notes 👇
When the native EditContext input path focuses the hidden edit context node, the browser scrolls every scrollable ancestor to reveal it. The node is parked at the editor's last cursor position, so in hosts where Monaco's ancestors scroll natively (
overflow: auto), focusing the editor yanks the container's scroll position and can shift layout between the mouse-down hit test and click resolution, placing the cursor on the wrong line.The textarea input path has guarded against exactly this for years in
writeNativeTextAreaContent("browsers will try really hard to reveal the textarea... we try to undo the browser's desperate reveal"). This PR mirrors that guard inFocusTracker.focus()using the existingsaveParentsScrollTop/restoreParentsScrollTophelpers frombase/browser/dom.ts.VS Code's own surfaces don't show the bug because workbench scrolling is synthetic and
ListViewneutralizes browser-initiated scrolls; it only manifests for embedders. Repro harness, before/after measurements, and verification against amonaco-editor-corebuild from this branch are in #324758. Downstream, Positron carries this fix with a Playwright regression test (posit-dev/positron#14696).Note: the IME-toggle handler in
nativeEditContext.tscallsthis.domNode.focus()directly, bypassingFocusTracker.focus(). It is not in the click path and is left untouched here.