fix(preview): never anchor the scroll restore to a boxless <br> - #464
Merged
Merged
Conversation
`convert_markdown` sets `render.hardbreaks`, so comrak ends every soft-wrapped line of prose with `<br data-sourcepos="…" />`. Those `br`s are `[data-sourcepos]` elements like any other, so `findAnchorElement` descends into one and returns it as the narrowest block containing the saved line — and a `br` reports `offsetTop === 0, offsetHeight === 0`. `getAnchorScrollTop(0, 0, …)` is `max(0, 0 + 0 - 60)`, so the restore sets `scrollTop = 0`: the reader is thrown to the top of the document rather than back to the line they left. Measured in Chrome over this pipeline's own output (real comrak + `processMarkdownHtml`, real CSS box): a heading-less document of soft-wrapped prose restored to 0 from 32 of 40 sampled scroll offsets (219, 328, 438, 547, 657, 766 px …); worst drift -4378 px. After the change, 0 of 40, worst drift 15 px. A document with headings, sub- headings and lists was hit at 20 of 40 — fewer of its anchor lines are `br` lines because headings and list items are single-line blocks — which is why issue sftwrdotdev#153's reporter sees one file restore and the text-only one "always go back to the top". The rate this file already measured stayed at 100% throughout: resolution succeeded, the element it resolved to just could not say where it was. The shim has no layout, so the regression test asserts the invariant structurally — the resolver may only return an element that generates a box. On the unfixed resolver it reports 300/450 anchor lines resolving to a boxless element.
This was referenced Aug 6, 2026
PathGao
added a commit
that referenced
this pull request
Aug 6, 2026
) Split view mapped a position by turning it into a share of the sending pane's scroll range and reading that share off the receiving pane's. A ratio cannot be right in general: forty source lines of table render as one tall block and forty of prose as a short one, so equal progress through the source is not equal progress through the rendered height. The error accumulates, which is why what #205's reporter noticed is the end of the document. A position now also carries the SOURCE LINE at the top of the sending pane's viewport, and each pane resolves that line through its own layout. The preview walks its `data-sourcepos` blocks to the narrowest one covering a scroll offset, and back, interpolating inside multi-line blocks; the editor uses Monaco's own `getTopForLineNumber` and a binary search to invert it, which stays correct with folds, wrapped lines and view zones. The two directions are the same descent over the same tree, so line -> pixel -> line is an identity rather than an approximation. The front-matter carve-out is untouched and is now also the fallback. Front matter renders as a panel with no source range at all, so a front-matter position carries no line and travels as section+ratio exactly as before. A body position falls back the same way when the preview holds no annotated block. A line no block owns — a blank line between blocks, a position past the last one — resolves to the nearest block instead of falling back, because doing otherwise would put a third of every document back on the proportional mapping and make the panes jump each time the reader crossed a gap. No feedback loop. The existing flags stop the echo; what is asserted here is that the mapping could not sustain one if an echo got through. A -> B -> A is idempotent: it snaps once, by at most the margin between two blocks, onto a position that is its own image, and both components already ignore a move under 5px. Getting there needed one fix a browser found and the DOM shim could not: blocks touch, so one block's bottom edge is the next one's top, and letting both claim that pixel made every echo walk the reader one block UP the document. A block now owns [top, bottom). Cost, measured in Chrome over this pipeline's own output rather than asserted. On a 13,356-line document (320,000px, 6,709 annotated blocks): offset -> line 0.89ms, line -> offset 0.28ms. On a 1,757-line document, 0.21ms and 0.12ms. Source ranges and derived container spans are memoised in WeakMaps keyed on the element; without them the walk is ~6ms. Nothing invalidates those memos explicitly — the preview is rebuilt wholesale by `bind:innerHTML`, so a changed document is a new set of nodes and the old entries are collected with them. The scroll handler got cheaper overall, not dearer. Capturing the tab's reading position used to be a separate `querySelectorAll` over every annotated element plus `offsetTop` on each, on the same event: 8.3ms on that 13,356-line document. It now shares the one descent, which also settles two disagreements it had with the restore — it took the outermost element covering an offset where `findAnchorElement` takes the narrowest, and it had no opinion about the boxless `<br>` of #464. scripts/scrollSyncBlockMapping.test.ts drives the mapping in both directions over real `processMarkdownHtml` output and measures the disagreement in SOURCE LINES, with the pre-fix mapping kept alongside as the control. On a document whose tail is tables and figures the ratio is 54.9 lines out near the end and 64.8 across the whole document; the block mapping is 0.9. The shim has no layout at all, so the layout is injected through the same `measure` callback the browser fills with `offsetTop`; what that cannot model — whether a browser really lays a table out that way — is stated in the file, and the touching-block case is covered by a second fixture because the first one's synthetic margins hid it. Refs #205. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 6, 2026
The review discipline here is real but undocumented: a mechanism section that explains why the old behaviour happened rather than what was done about it, a Scope section saying what was deliberately left alone, a falsification step on fixes, and a Verification section with the exact commands, their counts and an honest list of what was not checked. #468, #464, #462, #460 and #458 all have that shape. Nobody arriving from outside can know it. #463 came close by instinct, which is the argument for writing it down rather than hoping. Five headings, prompts only, no checkboxes. A checkbox that feels mandatory is a required field wearing a disguise, and friction is what makes a contributor abandon a template rather than fill it in; the header says outright that every section can be deleted. No licensing, conduct or "I read the guide" line -- there is no contributing guide to read. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
Closed
PathGao
added a commit
that referenced
this pull request
Aug 8, 2026
* fix(preview): interpolate between source lines when syncing scroll
Split view mapped a scroll position by descending the preview tree to the
single narrowest element covering it, then answering with that element's
first source line. An element is one answer, so the mapping quantised: every
offset inside a block resolved to the same line, and the panes only agreed
where a block happened to start.
Answer from two samples instead of one. `collectLineSamples` walks the
preview once into a flat, document-ordered table of (element, line) pairs —
the innermost element owning each line — and both directions binary-search
it for the pair bracketing the position and interpolate between them. This
is the model VS Code's Markdown preview uses, and it inverts the old
trade-off: narrower elements now make the answer *better*, because they make
the table denser, where before they made it coarser.
Two things fall out of that:
- The descent's `isCollapsedContainer` guard has to stay. Elements inside
a collapsed fold all measure to the same top, and a run of equal tops
would flatten a stretch of the table. `collectLineSamples` stops there,
matching the `isVisible` guard VS Code has for the same reason.
- Soft line breaks become worth annotating. `processSoftLineAnchors` puts
an empty inline-block beside each `<br>` inside a block spanning three
or more lines, so a long wrapped paragraph contributes several samples
rather than one. Under the old descent this made things worse (a
single-line range cannot interpolate); under a sample table it is just
more resolution. `<br>` itself has no box, so the anchor — not the
break — is what carries the line (#464).
Worst drift over the block-mapping fixtures: 0.9 source lines to 0.0.
Two assertions moved from integer lines to fractional ones. The editor side
has always answered with a fraction; an integer from the preview was itself
a source of the disagreement being fixed here.
* fix(preview): shift for front matter when scroll sync crosses panes
`data-sourcepos` counts from the first line of the BODY, because that is
what comrak is handed — front matter is stripped before the render. The
editor holds the whole file. On a document with front matter the two panes
therefore disagreed by exactly that many lines, a constant offset with
nothing to do with how precise the block mapping underneath was: on
`samples/stress-test.md` (ten lines of front matter) the preview sat
eleven source lines ahead of the editor, about half a screen.
`toBufferRange` already does this conversion for the jumps that #90 added.
Scroll sync crosses the same boundary, in both directions, and never did
it. `ScrollSyncPosition.line` is a buffer line — the editor is the only
pane that can produce one, and the buffer is all it knows — so the two
preview-side crossings convert, and a third one does too:
- `getPreviewScrollSyncPosition` shifts the line it reads off the preview
before handing it over;
- `scrollPreviewToSyncPosition` shifts back before seeking;
- `handleEditorScrollSync` shifts back before feeding `tocActiveLine`,
because the outline is built from `data-sourcepos` too. That is the
same bug wearing a different face: which heading the outline
highlighted depended on which pane you had scrolled.
`jumpToSelectedFragment.test.ts` already asserted that every renderer line
reaching the editor goes through the shift, but it enumerated only the two
jump consumers — the gap this shipped through. Scroll sync and the outline
are named there now, so forgetting a fourth consumer fails.
One assertion in `tocFollowsEditor.test.ts` had pinned the statement's
literal text. It now asserts what it meant: that the outline is fed before
the scroll-sync check, not inside it.
* fix(preview): keep the selection visible under the context menu
Right-clicking a selection made the highlight vanish. The menu focused
itself ten milliseconds after opening, which moves focus off the document
and stops the selection being painted — under the very menu whose Copy and
Edit items act on that selection.
The focus bought exactly one thing: the Escape handler bound to the menu
element. There is no arrow-key navigation to focus for, and the overlay
already handles click-to-dismiss. Escape now listens on the window, so it
still closes the menu and the selection keeps its highlight.
---------
Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local>
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.
Reported by @luanfernandes in an edit to #153: a text-only document always returns
to the top when he switches tabs and comes back, while another file restores fine.
He called it "partially fixed", which turns out to be exactly right — and the
variable is not the one either of us assumed.
Mechanism
convert_markdownsetsoptions.render.hardbreaks = true(lib.rs:1671), socomrak ends every soft-wrapped source line inside a paragraph with a
<br>— andstamps it with
data-sourceposlike any other node:search()inpreviewAnchor.tsdescends into anything carrying a source range, sofor a line that ends in a soft wrap it returns the
<br>as "the narrowest blockcontaining this line". A
<br>has no principal box:and the restore is
getAnchorScrollTop(0, 0, …, 60)=max(0, 0 + 0·ratio − 60)=0. The preview scrolls to the top. Measured round trip on v2.7.0 — capture at
offset S, switch tab, restore:
Capture and persistence are clean.
getPreviewScrollAnchornever picks a<br>—querySelectorAllis document order and the enclosing<p>hitsdistance === 0first.
tab.anchorLinesurvives the tab switch and is never0here, so the> 0gate is not involved.
offsetParentisARTICLE.markdown-bodyfor every matchedelement in both document shapes, so that constant cancels as the module's header
says it does.
The real variable is hard-wrapping, not headings
br[data-sourcepos]Headings dilute it — a heading, a list item and a code fence are single-line blocks
whose anchor lands on something with a box — but a structured document written in
wrapped prose still fails half the time. The reporter's "file with headings" is
almost certainly one long line per paragraph.
Why the suite could not catch it
Worth stating plainly, because the number looks good:
previewAnchorRestore.test.tsscoresfindAnchorElement 300/300 (100%)on bothfixtures, and always did. Resolution genuinely succeeds. The element it resolves
to simply cannot say where it is. There is no tolerated miss rate here — this is a
regression hiding behind a perfect metric.
Two gaps let it through.
scripts/renderProtocolDom.tsmodels no layout at all —grep -rn offsetTop scripts/returns nothing, sogetAnchorScrollTophad only everbeen exercised with hand-written numbers. And
DocumentBuildernever emits a<br data-sourcepos>, so the failing shape was not in the corpus, even though therecorded real output in
renderProtocolFixtures.tscontains it.The fix
BOXLESS_TAGS = new Set(['BR', 'WBR']), anisAnchorable()predicate, one clause inelementChildren, one optionaltagNameonAnchorNode. Skipping the<br>letssearchfall through to thecarriedmatch — the enclosing<p>— which is whatthe interpolation in
getAnchorScrollTopwas built for in the first place.The test adds a
wrappedParagraph()builder emitting the recorded hardbreak shape,and asserts the structural invariant per line rather than by sampling: the resolver
may only return an element that generates a box. Reverting the fix with the tests
kept gives
300/450 anchor lines resolved to a boxless element, 2 red, and all 8pre-existing tests still green.
Verification
Measured in headless Chrome, not in the app. The preview's box was reproduced
from
src/styles.cssplus the component-scoped rules, driving the realgetPreviewScrollAnchorandpreviewAnchor.tsover real comrak → realprocessMarkdownHtmloutput. Only Blink was measured; a<br>having no principalbox follows from CSSOM and I would expect the same in WKWebView and WebView2, but I
did not verify either.