Skip to content

Preserve ancestor scroll positions when focusing the native edit context#14696

Open
nstrayer wants to merge 4 commits into
mainfrom
fix/14085-cell-click-cursor
Open

Preserve ancestor scroll positions when focusing the native edit context#14696
nstrayer wants to merge 4 commits into
mainfrom
fix/14085-cell-click-cursor

Conversation

@nstrayer

@nstrayer nstrayer commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #14085
Fixes #14656

When a Positron notebook cell is taller than the viewport, clicking a line could place the cursor on the wrong line (#14085) and/or yank the scroll position to the top of the cell (#14656). Both symptoms share one root cause: when Monaco focuses its hidden edit-context node during click handling, the browser tries to reveal that node by scrolling every scrollable ancestor. The node is parked at the editor's last cursor position, so in a long cell that reveal shifts the notebook's scroll container mid-click, desyncing Monaco's hit test from what is on screen.

The fix mirrors the guard the textarea input path has carried for years (writeNativeTextAreaContent in textAreaEditContextInput.ts): save ancestor scroll positions, focus, restore. Three lines in FocusTracker.focus(), fenced per upstream-compatibility conventions.

Why this is safe outside notebooks

  • The guard only writes back a scrollTop that actually changed during focus(). In stock VS Code surfaces (editors, VS Code notebooks, lists) scrolling is synthetic -- listView.ts even actively resets browser-initiated scrolls -- so the guard is a no-op there.
  • The two input modes are switchable via editor.editContext, and the textarea mode has applied this exact guard globally for ~a decade. Any surface depending on the browser reveal would already be broken in textarea mode.
  • Upstream's native EditContext implementation (Sept 2024) simply never got the guard; the bug only manifests in hosts with natively scrolling ancestors, which VS Code itself doesn't have. Positron notebooks are the first such host here. A follow-up PR offering the same fix to microsoft/vscode is planned, which would let this fork patch dissolve at a future upstream merge (related upstream report: [Bug] Re-focusing editor on blur scrolls to the editor on Safari microsoft/monaco-editor#4042).

Release Notes

New Features

  • N/A

Bug Fixes

Validation Steps

@:positron-notebooks

A new e2e regression test covers the long-cell scenario (notebook-click-cursor-position.test.ts): it verifies the precondition that the hidden edit-context node is far outside the scroll viewport, clicks a line near the top of a 60-line cell, and asserts the cursor landed on that exact line. It fails without the fix and passes with it.

Manual check:

  1. Open a notebook and create a code cell with 60+ lines (taller than the viewport).
  2. Press Esc to exit edit mode, scroll back to the top of the cell.
  3. Click directly on the text of a visible line.
  4. The cursor lands on the clicked line and the scroll position does not jump.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:positron-notebooks

readme  valid tags

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

PETE's assessment 🧪

Verdict: 🟢 Adequate -- The bug-fix adds a targeted e2e regression test that reproduces the exact click-on-wrong-line scenario, and the behavior (browser scroll-reveal during focus) genuinely requires the full app rendered to verify.

What changed

  • Adds a 3-line guard in FocusTracker.focus() (nativeEditContextUtils.ts) that saves ancestor scrollTop, focuses the hidden edit-context node, then restores scroll -- preventing the browser from yanking a natively-scrolling notebook cell mid-click. Fenced per upstream-compatibility conventions and mirroring the long-standing textarea-path guard.
  • Adds a second e2e test to the existing notebook-click-cursor-position.test.ts for the long-cell (taller-than-viewport) variant.

Tests in this PR

  • Unit (Vitest/Mocha) ✅ (not applicable -- behavior depends on real browser layout/scroll geometry, which jsdom cannot reproduce)
  • Extension host ✅ (not applicable)
  • E2E (Playwright) ✅ (added test to notebook-click-cursor-position.test.ts)

Existing coverage

The new e2e test is the primary and correct coverage. I confirmed it exercises the fixed path: it establishes the precondition that the hidden input sits well below the scroll viewport (inputBox.y > containerBox bottom + 100), clicks on the text of a visible line, and asserts the typed marker lands on that line -- the exact regression the guard prevents. Per the PR, it fails without the fix and passes with it. A Vitest test cannot reach this: the bug only manifests when real scrollable ancestors and Monaco's client-coordinate hit test interact under actual layout.

Suggested additions

None.

Suggested tags (optional)

None needed. The describe block is already tagged @:positron-notebooks (in the PR body), @:win, and @:web, so the regression runs at PR time on both surfaces where native scrolling ancestors can trigger the browser reveal.

Deployment note (optional)

None needed. This is precisely the kind of surface-sensitive fix where the @:win and @:web tags matter, and both are already present on the enclosing describe, so Windows and web get PR-time coverage rather than falling through to nightly-only.


PETE (Positron Extreme Test Experiment) - LLM-based test-coverage advisor, in pilot. Triggers on PR open and on /recheck-tests comments. Wrong verdict? Comment /recheck-tests (or /rePETE) on this PR to re-run. Please share feedback on how PETE performed here.

@nstrayer nstrayer requested review from jmcphers and seeM July 6, 2026 20:54
@nstrayer

nstrayer commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@seeM Tagged you here because you did the work on the notebook scrolling on load so you may be more familiar with the scrolling mechanisms than normal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant