Skip to content

fix(scroll-sync): measure a block from the scroll container, not from its offset parent - #515

Merged
PathGao merged 4 commits into
masterfrom
fix/scroll-sync-offset-parent
Aug 7, 2026
Merged

fix(scroll-sync): measure a block from the scroll container, not from its offset parent#515
PathGao merged 4 commits into
masterfrom
fix/scroll-sync-offset-parent

Conversation

@PathGao

@PathGao PathGao commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Follows #474, on @Guardiancelte's retest of it: the per-block mapping is right, the ruler it reads is not.

offsetTop is not the distance to the top of the document

The mapping resolves a position by descending to the annotated element that owns it and reading offsetTop. That is measured from the element's offset parent, which CSSOM defines as the nearest positioned ancestor — or the nearest table, td or th, positioned or not. #474 assumed one offset parent for everything in the preview and said so in a comment. The preview has plenty.

Measured in Chrome, over this app's own styles.css:

element                        offsetTop   offsetParent          really at
p                                     84   article.markdown-body        84
table                                994   article.markdown-body       994
tr   (a row of that table)             1   TABLE                       995
td                                    32   TABLE                      1026
tr   (the last row)                   94   TABLE                      1088
pre  (a code block)                    0   div.code-block-shell       1135

comrak stamps a source range on every table row and cell, so the descent goes inside a table and reads that middle column as if it were the right-hand one. renderRichContent wraps every code block in a position: relative shell for the copy button, so a code block reports 0.

Both of @Guardiancelte's cases are that, from the two directions:

  • line → offset. A line inside a table resolves to ~200px, so the pane scrolls to the top of the document. "one scroll down, it sends me to the top of the document, and it never comes back until I am passed both tables".
  • offset → line. Every row's box is far above an offset in document space, so the nearest row is always the last one: the other pane freezes on the end of the table until the reader is past it, then jumps. "the left side is frozen until the top of the table is visible on the right side… and when that unfreeze, one scroll down shot the left side all the way back down".

The direction that misbehaves depends on which pane is sending, which is why it looked like the mouse side mattered.

The fix

measureAnchorBox sums the offset-parent chain and subtracts the container's own sum. Subtracting rather than stopping at the container is what keeps it exact whether or not the container is itself an offset parent — with .markdown-body's transform: translate3d(0,0,0) Chrome makes it one, but nothing should depend on that.

The tab-restore path and the front-matter carve-out read the same measurement now; both used raw offsetTop and had the same defect (an anchor saved inside a table reopened the tab at the top).

Mermaid — the other half of the report

preEl.replaceWith(container) gave the diagram none of the code block's data-sourcepos, and Mermaid's SVG keeps nothing of the <pre> either, so a diagram was the one block in the preview that mapped to no source line at all — not even through a descendant. Several hundred pixels of preview got attributed to whatever block was nearest, which is why the two panes' top headings disagreed by about a diagram (# 11. Mermaid Diagrams against Sequence Diagram). The container now inherits the range it replaced.

Cost

Two to three extra offsetTop reads per candidate. Measuring every annotated element of a 9,900-element preview in Chrome: 2.20 ms read raw, 6.30 ms walked. A descent measures a few dozen, so this is tens of microseconds on a scroll event, against the 0.89 ms the descent itself costs on a 13,356-line document.

Tests

scripts/scrollSyncBlockMapping.test.ts had the blind spot in the shape of its own layout: layOut handed the mapping one flat stack of absolute tops, which models a preview containing neither a table nor a positioned wrapper. Same class of miss as #464 — the shim has no layout, so what it does not model cannot fail.

It now models the offset parent instead. Every box is converted to what a browser would report before the production measureAnchorBox converts it back, the fixture's tables carry the row and cell ranges comrak really emits, and wrapCodeBlocks puts the .code-block-shell around the <pre>s the way renderRichContent does. rawMeasureoffsetTop read raw — is kept alongside as the control, the way RATIO_ONLY is.

Falsification, mapping reverted to raw offsetTop, tests kept:

✖ the tail of the document stays on the same source line
  AssertionError: the preview drifted 622.8 source lines from the editor at scrollTop 11834 (of 12158)
✖ every part of the document stays on the same source line, not only the tail
  AssertionError: the preview drifted 623.9 source lines at scrollTop 11854
✖ scrolling one pane down always moves the other pane down
  AssertionError: editor 6930 -> 6991 moved the preview backwards: 9576.0 -> 34.0
✖ a line inside a table maps into that table, not to the top of the document
  AssertionError: a line in a table at 20184px resolved to 204px
✖ an offset inside a table resolves to the row at it, not to the row after the last
  AssertionError: halfway down the table resolved to line 670 of 655-670
✖ a row reports an offset measured from its table, and a code block from its shell
✖ an echo through the other pane settles in one step
✖ touching blocks do not walk the panes up the document
ℹ pass 14   ℹ fail 8

That third one is the reporter's screenshots exactly: one scroll click down, and the other pane at 34px.

And with only the Mermaid line reverted:

✖ a rendered diagram answers for the source lines it replaced
  AssertionError: flowchart: the diagram container must carry the source range of the code block it replaced

Verification

npm run check (0 errors), npm test (782 passing), cargo test (125 passing). npm audit reports one moderate advisory in mermaid 11.16.0, unrelated to this change and present on master.

What is still modelled rather than measured is the layout — whether a browser lays a table out the way the fixture's heights say. What is now measured is the thing that broke: the numbers in the table at the top of this description are from Chrome, not from the shim.


@Guardiancelte — thank you, both of those cases were real and neither was the mapping I described to you; they were the measurement under it. Your large-table file is the one I would most like retested once this lands. The remaining difference you may still notice is the one named in #474: what the panes align is the line at the top of each viewport, so at the very end of a document the preview is not necessarily at its own last pixel.

PathGao and others added 3 commits August 7, 2026 00:47
…g imports

Four functions in MarkdownViewer.svelte have no caller. Three of them are
stale copies of render steps that moved elsewhere and kept evolving there:
processTaskItems and processBlockIds moved into markdown.ts in 09fa88f, and
the ==highlight== rewrite moved into the Rust renderer, where it later grew
code-span protection (#228, #371). Each migration re-pointed the call site
and left the old body behind, so the copies have been sitting there since
b46a283 looking like reusable helpers. getSplitTransition is plain dead.

Also removed: 24 exports nothing outside their own file imports, the
escapeHtmlText alias that only forwards to escapeHtml, an addFrontMatterList-
Item wrapper with no production caller, the toPlainRecord guard that
isFrontMatterMapping already makes unreachable, and FrontMatterField's
editableValue, which computes the same string as displayValue in every
branch with nothing to keep the two from drifting apart.

No behaviour change. The front matter tag test now calls the plural helper.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same sweep as the previous commit, finished in scripts/. Twenty-two names in
keymapHarness.ts, renderProtocolDom.ts, renderProtocolFixtures.ts,
windowTagEditor.ts and scrollSyncBlockMapping.test.ts are only used inside
their own file, so the export widened the contract for nothing.
RenderFixtureName had no use anywhere, including its own file, and is gone.

tsconfig.json puts scripts/ inside `npm run check`, so a name another file
still imports would fail the type check rather than pass silently.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… not from its offset parent

The per-block mapping resolves a position by descending to the annotated
element that owns it and reading `offsetTop`. `offsetTop` is measured from
the element's OFFSET PARENT, which CSSOM defines as the nearest positioned
ancestor -- or the nearest `table`, `td` or `th`, positioned or not. The
preview contains both kinds, and comrak stamps a source range on every table
row and cell, so the descent goes inside a table and reads table-relative
offsets as document offsets.

Measured in Chrome over this app's own stylesheet: a row 945px down a
document reports `offsetTop = 1`, and a `<pre>` 1,085px down reports 0
because `renderRichContent` wraps every code block in a `position: relative`
shell for the copy button.

Both halves of #205's surviving report follow. A line inside a table maps to
a couple of hundred pixels, so the pane jumps to the top of the document; and
every offset in the table is far below every row's box, so the reverse
direction sticks on the last row until the reader is past the whole table.

`measureAnchorBox` sums the offset-parent chain and subtracts the
container's own. Subtracting rather than stopping at the container keeps it
exact whether or not the container is itself an offset parent.

A rendered Mermaid diagram now also inherits the source range of the `<pre>`
it replaces. Mermaid's SVG keeps nothing of the code block, so a diagram was
the one block in the preview that mapped to no source line at all -- not even
through a descendant -- and several hundred pixels of preview were attributed
to whatever block was nearest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…set-parent

# Conflicts:
#	src/lib/utils/previewAnchor.ts
@PathGao
PathGao merged commit 729b7ec into master Aug 7, 2026
4 checks passed
@PathGao
PathGao deleted the fix/scroll-sync-offset-parent branch August 7, 2026 08:49
PathGao added a commit that referenced this pull request Aug 7, 2026
3,164 lines: 124 headings, 543 table rows, three Mermaid diagrams, code
blocks in eight languages, footnotes, math, raw HTML and text in seven
scripts. Written to compare Markdown readers, and the document that found
both defects fixed in #515 — a table's rows and a rendered diagram report
positions the scroll mapping could not read.

Authored to @Guardiancelte, who wrote it, reported #205 and retested every
round of scroll-sync fixes against it.

Co-authored-by: Guardiancelte <174342084+Guardiancelte@users.noreply.github.com>
PathGao pushed a commit that referenced this pull request Aug 7, 2026
3,164 lines: 124 headings, 543 table rows, three Mermaid diagrams, code
blocks in eight languages, footnotes, math, raw HTML and text in seven
scripts. Written to compare Markdown readers, and the document that found
both defects fixed in #515 — a table's rows and a rendered diagram report
positions the scroll mapping could not read.

Authored to @Guardiancelte, who wrote it, reported #205 and retested every
round of scroll-sync fixes against it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant