fix: read caret geometry from the editor document (#8038)

[JohnMcLear]

Fixes #8038

The bug

With scrollWhenFocusLineIsOutOfViewport.scrollWhenCaretIsInTheLastLineOfViewport enabled (FOCUS_LINE_CARET_SCROLL=true in the Docker image), moving the caret onto the line at the bottom edge of the viewport throws in the browser and raises the pad's "An error occurred / press Ctrl+F5" box:

TypeError: can't access property "bottom", t is null
  C5@padbootstrap-….min.js
  _isCaretAtTheBottomOfViewport@…
  scrollWhenCaretIsInTheLastLineOfViewportWhenNecessary@…

Reproduces in Chromium as well as Firefox — it is not browser specific.

Root cause

caretPosition.getPosition() measured the caret from window.getSelection() of the pad's top-level window. Up to 2.x that was the right window: ace2_inner was loaded inside the ace_inner iframe, so the module's ambient window/document were the editor's. Since the 3.x bundling change ace2_inner.ts runs in the top-level window and reaches into the frames explicitly (document.getElementsByName("ace_outer")[0]…), but caretPosition.ts was never updated.

The caret lives in the ace_inner iframe, so the top window's selection is permanently empty (type: "None", rangeCount: 0). getPosition() therefore always returned null, and _isCaretAtTheBottomOfViewport() passed that null — through a ! assertion that was never true — straight into getBottomOfNextBrowserLine(), which dereferences .bottom.

Two consequences, both fixed here:

1. The crash in the reported code path.
2. Dead scrolling. Every caret-geometry consumer in scroll.ts has been inert since 3.0, because getPosition() never returned anything. On top of that, outerWin is the ace_outer iframe element, not a window, so outerWin.scrollTo()/scrollBy() hit Element.scrollTo()/scrollBy(), which silently do nothing on an iframe — the pixel amounts this feature computed were never applied.

The fix

• caretPosition.ts takes the editor document explicitly and reads the selection, the shadow-caret text node and the measuring ranges from it. That is also the coordinate space scroll.ts already compares against: the inner frame is as tall as its content and never scrolls, so its client rects line up with the outer document's scroll offsets (verified in-browser: rect.top === offsetTop for every line).
• getPosition() returning null is now a real case (focus outside the editor) and callers bail out instead of asserting it away.
• Scroll calls go through contentWindow, and the window.parent / outer[0].innerHeight hack in scrollNodeVerticallyIntoView — which from the top window scrolled the pad page by the editor's viewport height — scrolls the ace_outer window to the end of the pad, as originally intended (editor: fix enter key keep line in view #4639).
• $(range.endContainer).closest('body')[0].id also threw on a detached node; it is now null-safe.

Not in this PR, but found on the way and worth a follow-up: ace2_inner.ts has the same element-instead-of-window mistake for horizontal scrolling (outerWin.scrollBy(...) / outerWin.scrollX at lines 2824 and 3648-3653), so scrolling a long line into view sideways in no-wrap mode is equally inert. Left out to keep this diff to the reported crash.

Note for reviewers: restoring the caret-follow scrolling means scrollNodeVerticallyIntoView does something again on 3.x, so a caret that ends up outside the viewport is scrolled back into view instead of being left to the browser. That is the pre-3.0 behaviour.

Tests

New spec src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts (the setting is off by default, so the spec turns it on by patching clientVars before pad.ts reads it):

• crash regression — placing the caret on the bottom-edge line and arrowing past it produces no page errors. Runs in Chromium and Firefox.
• behaviour — with percentage.editionBelowViewport: 0.6, the same click scrolls the caret line clear of the bottom edge (measured jump ~506px, versus 0 before). Chromium only: Firefox applies its own scroll-into-view before the check runs, which leaves the trigger comparison decided by a pixel or two — too marginal to assert on. The behaviour itself was verified there by hand.

Verified failing without the source change (all three runnable tests fail, including the exact Cannot read properties of null (reading 'bottom')) and passing with it, 6 consecutive runs clean.

Backend suite: 1611 passing, 1 failing — the failure (Pad.ts › padDefaultContent hook › issue #7885) reproduces on unmodified develop and is unrelated.

[qodo-free-for-open-source-projects]

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix caret geometry and scrolling across editor iframes

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Measure caret geometry from the nested ace_inner editor document.
• Handle missing selections and route scrolling through ace_outer's content window.
• Add browser regression coverage for bottom-edge caret movement and scrolling.

Diagram

sequenceDiagram
  actor User
  participant Editor as Editor Events
  participant Scroll as Scroll Controller
  participant Inner as Inner Document
  participant Geometry as Caret Geometry
  participant Outer as Outer Window
  User->>Editor: Move caret
  Editor->>Scroll: Process selection
  Scroll->>Inner: Resolve document
  Scroll->>Geometry: Measure with document
  Geometry->>Inner: Read selection and ranges
  Inner-->>Geometry: Position or null
  Geometry-->>Scroll: Caret geometry
  alt Caret needs scrolling
    Scroll->>Outer: scrollTo or scrollBy
  else Selection unavailable
    Scroll-->>Editor: Skip scrolling
  end

Loading

High-Level Assessment

The explicit editor-document approach is appropriate because caret geometry must be measured in the ace_inner coordinate space, and resolving the document on demand handles iframe document replacement. Routing scroll operations through ace_outer.contentWindow directly fixes the corresponding browsing-context mismatch without introducing unnecessary abstraction.

Files changed (3) +295 / -71

Bug fix (2) +121 / -71

caretPosition.tsMeasure caret geometry using the editor document +68/-52

Measure caret geometry using the editor document

• Caret and line measurements now receive the ace_inner document explicitly and create selections, ranges, and shadow nodes within that context. The API returns null when no editor selection exists and verifies selection ownership without relying on the ambient top-level DOM.

src/static/js/caretPosition.ts

scroll.tsUse correct iframe contexts for measurement and scrolling +53/-19

Use correct iframe contexts for measurement and scrolling

• The scroll controller resolves the current ace_inner document before measuring the caret and safely exits when no selection is available. Scroll reads and writes now target ace_outer.contentWindow, including end-of-pad scrolling based on the inner document height.

src/static/js/scroll.ts

Tests (1) +174 / -0

scroll_caret_viewport.spec.tsCover bottom-edge caret scrolling regressions +174/-0

Cover bottom-edge caret scrolling regressions

• Adds Playwright scenarios that enable bottom-edge caret scrolling, populate an overflowing pad, and target the viewport boundary by coordinate. The tests verify that caret movement produces no browser errors and that Chromium applies the configured viewport-relative scroll.

src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[Copilot]

Pull request overview

This PR fixes a 3.x regression where caret geometry was being measured from the top-level document (which has an empty selection), causing getPosition() to return null and triggering a browser-side crash when caret-follow scrolling is enabled. It updates the caret/scroll logic to explicitly operate on the ace_inner document and to scroll via the ace_outer iframe’s contentWindow, and adds a Playwright regression spec to validate both “no crash” and restored scrolling behavior.

Changes:

• Measure caret position from the ace_inner document (instead of the top-level window/document) and make null a supported “no caret in editor” outcome.
• Route scrolling through ace_outer.contentWindow and resolve the inner editor document on demand to avoid stale iframe document references.
• Add a Playwright regression spec for the crash and for caret-follow scrolling behavior under the relevant setting.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/static/js/caretPosition.ts	Updates caret geometry helpers to take a Document and read selection/ranges from the editor iframe’s document.
src/static/js/scroll.ts	Uses the editor iframe document for caret measurements; scrolls via the ace_outer iframe’s contentWindow; adds null-safe bails.
src/tests/frontend-new/specs/scroll_caret_viewport.spec.ts	Adds E2E regression coverage for the bottom-of-viewport caret crash and restored scrolling behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[JohnMcLear]

Good catch — adopted in 427cda4. The 40-line setup now uses keyboard.insertText() per line (same reasoning as writeToPad()), which also cuts the spec's runtime by about a third. Re-verified after the change: 4 consecutive clean runs in Chromium + Firefox, and the spec still fails as expected when the source fix is reverted.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.