fix(preview): make task checkboxes work, and look like task checkboxes - #534
Merged
Conversation
Reopening #148 was right, and so was the reporter's "2.6.7 was the last working version" — it broke twice, for two unrelated reasons, and only the second one is ours. **2.6.8 — not ours.** comrak 0.54 changed the task-checkbox HTML, the preview's recognizer stopped matching it, and the control rendered disabled. Everyone saw a grey checkbox. #320 and #345 fixed that. **2.7.0 — ours.** #345 also replaced the write-back, and that is the regression: ```diff - let count = 0; - raw.replace(/^(\s*[-*+] )\[( |x|X)\]/gm, (match, prefix) => { - if (count++ === index) … // ordinal: EOL-independent + body.replace(/…/gm, (match, prefix, _state, offset) => { + const line = body.slice(0, offset).split('\n').length; // line arithmetic ``` `\s` matches `\r` as well as `\n`, and JavaScript's `/m` `^` also matches at the position between a `\r` and its `\n`. With `\s*` greedy, every match in a CRLF buffer starts one character early — on the previous line's `\n` — so the slice holds one `\n` too few and every task resolves to line N-1: CRLF, tasks on lines 2/3/4 → computed as 1/2/3 On the last task nothing matches `sourceLine`, the write is skipped, and `handleTaskCheckboxChange` restores the control. That is indistinguishable from a dead checkbox, and it is what #148 reports. Everywhere else the off-by-one lands on a *real* task line and silently rewrites the wrong one — which nobody filed, because nobody could see it happen. So a CRLF author had no working version after 2.6.7: grey until 2.7.0, then blue but writing to the wrong line. Every file written by a Windows editor was affected. Three fixes missed it because all three were verified on macOS, where the bug cannot occur. Horizontal whitespace only, so a match can never begin before the line start. The Rust `TASK_SOURCE_RE` needs no change: it runs over `markdown.lines()`, already split, so its `\s*` cannot span a line. Tests drive the real document session over both line endings. The existing behavioural toggle test passes `sourceLine: 1` — the one line number the bug cannot reach, since at offset 0 there is no preceding terminator to eat, which is why the suite stayed green through two releases. Reverting the pattern fails the six CRLF cases and leaves the five LF cases passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first version of these fixtures put the list immediately after the prose, which is the one shape that was always correct on LF — so the LF half of the suite passed while half the bug was still there. `\s*` is greedy and `/m` `^` matches at the start of a blank line, so the match ate the blank line's terminator and `offset` landed one line early. That is reachable on every platform, not only CRLF: measured against samples/stress-test.md (44 tasks, LF) the old pattern got six wrong — one per list block, each the first task after a blank line. CRLF widened it from the first task of each block to all of them. Fixtures now carry the blank line, with a no-blank-line control group and two LF-specific cases pinning the boundary.
The stress document predates several of the things Markpad renders, so a reader could pass it end to end while three features were broken. Diffed against `markdown-syntax.md`, which is the list of what the app claims to support, it had **none** of: wikilinks, block ids, video and audio embeds, YouTube embeds, or `++inserted++`. Added those, plus the two image forms it was missing (a title, and a reference-style destination). The media files are deliberately absent from the repository — what is being tested is that the *syntax* resolves to a player element rather than to a broken image. ## Task list shapes The larger addition, and the reason this is worth doing now. Toggling a checkbox in the preview has to find the line to rewrite in the source, and every marker shape is a separate case for that lookup. The document had two: a flat `-` list and one nested level. It now carries ordered `1.`, parenthesised `1)`, `*`, `+`, tasks inside a blockquote, four levels of mixed nesting, adjacent runs with nothing between them, and — the case that matters most — a list with **no blank line above it** next to one with a blank line. That last pair is not decoration. #148's bug lived in the interaction between a greedy `\s*` and `/m`, and a blank line above the list was half the condition: the pattern could eat the blank line's terminator and resolve the task one line early. Measured against the old and new documents with the pattern as it was before #534: old document, old pattern: 44 tasks, 6 wrong new document, old pattern: 76 tasks, 14 wrong Adjacent runs are there for the same reason. An off-by-one in a sparse list lands on nothing and looks like a dead checkbox; in a dense one it lands on a real task and silently ticks the neighbour, which is the failure nobody reports because nobody can see it. ## The shape from the report #148's reporter attached a file, and it is a shape nothing here had: a task whose item is a dozen indented lines — mixed 4- and 8-space indents, every line an inline code span, most ending in two trailing spaces so each is a hard break, the whole run one lazy-continuation paragraph inside the item. Its list also begins immediately after a heading rather than after prose. Reproduced faithfully, including the trailing double spaces, which are load-bearing here and must survive any reformatting. It exercises two things nothing else does: a checkbox whose item is many lines tall, so the control and its text can be laid out apart from one another; and continuation lines holding `$(…)`, `|` and `%`, which several of the preprocessing passes look for. The one dimension a single file cannot carry is line endings — this one is LF and the original was CRLF. `scripts/taskToggleLineEndings.test.ts` covers that half by driving both through the real write-back. Also added task-LIKE text that must not become a checkbox: inside prose, an indented code block, inline code, a fenced block, and an escaped bracket. Verified every new shape resolves to its own line under the current write-back pattern, that the two wikilink targets and both image paths exist, and that the old pattern still fails on 14 of them.
…rendering the document Two defects on the same gesture, both found in a reader's screenshot on #148 rather than by anyone reporting them. **Task lists lost their styling in the comrak upgrade.** `styles.css` has fifteen rules hanging off `li.task-list-item` and `ul.contains-task-list` — the bullet suppression, and the grid that puts the checkbox beside its text — and comrak emits neither class unless `render.tasklist_classes` is set. It defaults to off, so all fifteen were dead: every task list rendered with its bullet still showing AND the checkbox alone on a line above its text. It went missing in the 0.18 → 0.54 upgrade (#426), alongside the `disabled` change that #320/#345 fixed. Unlike that one it was never filed on its own, because it arrived looking like part of the same breakage. Turning it on moved the HTML under `TASK_ITEM_RE`, which anchored on `<li data-sourcepos=` immediately followed by `<input type="checkbox"`. comrak now writes a class into both, the pattern stopped matching, and `data-task-checkbox` stopped being injected — which is the attribute the preview uses to find a checkbox at all. Six tests caught it. Both class groups are optional in the pattern, so flipping the option back would fail a test rather than silently un-marking every checkbox. **Ticking a checkbox re-rendered the whole document.** `toggleTaskCheckbox` is careful not to: it toggles `task-done` on the item and leaves the DOM alone. But writing the buffer is enough on its own — MarkdownViewer's render effect fires whenever `rawContent` stops matching `_lastRenderedRawContent`, so in split view every tick rebuilt the article to arrive at the DOM that was already on screen, and took the reader's scroll position with it. In a long document the view landed somewhere else entirely. The write now records that the preview already matches. In the same synchronous block as the write, deliberately: the effect checks the field and arms a 16ms timer before anything is awaited, so marking after an `await` would be too late to stop the render it had scheduled. `mathDelimiterCorpus.json` is a live capture of this renderer's output; its one task-list row is re-captured, which is what its own test asks for when the renderer moves.
Turning on `tasklist_classes` fixed half the styling and left the other
half looking identical to the bug. comrak writes `task-list-item` only
on the branch where it opens the `<li>` itself, which a plain task item
does not take — so the `<ul>` gets `contains-task-list` and the items
get nothing:
<ul class="contains-task-list" …> ← list-style: none matched
<li data-sourcepos="1:1-1:15">… ← the grid rules did not
The bullet disappeared, because that one rule can match the list. Every
rule that positions the checkbox beside its text names the item, so they
all stayed dead and the checkbox kept its own line above the text.
Added in `processTaskItems` rather than by widening fifteen selectors to
`ul.contains-task-list > li`: that loop already owns which items are
really tasks — `data-task-checkbox` is the renderer's own verdict, and
it is checked immediately above — so the class lands on exactly that set
and nothing else, including in a list that holds both kinds of item.
PathGao
force-pushed
the
fix/task-toggle-crlf
branch
from
August 8, 2026 04:57
e7e3441 to
4a70c0c
Compare
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 #148.
@arcadepro was right to reopen it, right that 2.6.7 was the last version that worked, and right again when he said the newer fixes only changed the colour. Four separate defects were sitting on this one gesture. Three of them arrived together in the comrak 0.18 → 0.54 upgrade (#426, shipped as 2.6.8), and only one of the three was ever fixed.
disabled1. The toggle wrote to the wrong line
\smatches\ras well as\n, and JavaScript's/m^also matches at the position between a\rand its\n. With\s*greedy, a match can begin one character early — on the previous line's terminator — so the slice holds one\ntoo few and the task resolves to line N-1.This is not only a CRLF bug, which is what the first version of this PR claimed. A blank line above the list is the other half of the condition, and that is how most people write a list. Measured against
samples/stress-test.mdbefore and after:The old document had 44 tasks and got 6 wrong. Every one of the six was the first task after a blank line.
On the last task of a list nothing matches
sourceLine, the write is skipped, and the control snaps back — indistinguishable from a dead checkbox, which is what #148 reports. Everywhere else the off-by-one lands on a real task and silently rewrites the neighbour. Nobody filed that, because nobody can see it happen.Fix: horizontal whitespace only, so a match can never begin before the line start. The Rust
TASK_SOURCE_REneeds no change — it runs overmarkdown.lines(), already split.2. Task lists lost their styling
styles.csshas fifteen rules onli.task-list-itemandul.contains-task-list— the bullet suppression and the grid that puts the checkbox beside its text. comrak emits neither class unlessrender.tasklist_classesis set, and it defaults to off. All fifteen were dead: task lists rendered with the bullet still showing and the checkbox alone on a line above its text. This is what the reporter's screenshot shows.Turning the option on moved the HTML under
TASK_ITEM_RE, which anchored on<li data-sourcepos=immediately followed by<input type="checkbox". comrak now writes a class into both, so the pattern stopped matching anddata-task-checkboxstopped being injected — the attribute the preview uses to find a checkbox at all. Six tests caught it. Both class groups are now optional, so flipping the option back fails a test rather than silently un-marking every checkbox.That fixed half of it. comrak writes
task-list-itemonly on the branch where it opens the<li>itself, which a plain task item does not take — so the<ul>got its class and the items got nothing, the bullet disappeared, and every rule that positions the checkbox stayed dead.processTaskItemsnow adds it, which is where the app already decides which items are really tasks.3. Ticking re-rendered the whole document
toggleTaskCheckboxis careful not to re-render — it togglestask-doneon the item and leaves the DOM alone. But writing the buffer is enough on its own: MarkdownViewer's render effect fires wheneverrawContentstops matching_lastRenderedRawContent, so in split view every tick rebuilt the article to arrive at the DOM that was already on screen, and took the reader's scroll position with it. In a long document the view landed somewhere else entirely.The write now records that the preview already matches — in the same synchronous block, because the effect checks that field and arms a 16ms timer before anything is awaited.
4. The stress document could not have caught any of this
samples/stress-test.mdhad two task shapes: a flat-list and one nested level. It now carries ordered1., parenthesised1),*,+, tasks in a blockquote, four levels of mixed nesting, adjacent runs, and a list with no blank line above it beside one with a blank line.It also carries the shape from the reporter's own attachment: a task whose item is a dozen indented lines, every line an inline code span, most ending in two trailing spaces so each is a hard break. That is the shape where a checkbox and its text can be laid out apart from one another.
Diffed against
markdown-syntax.mdwhile in there, the document was also missing every wikilink, block id, video/audio embed, YouTube embed and++inserted++— so three features could have been broken with the stress test passing end to end. Added.Why it survived two releases
All three previous fixes were verified on macOS, where the CRLF half cannot occur. And the suite could not see the rest: the only behavioural call of
toggleTaskCheckboxlives intruncatedBufferGuard.test.tsand passessourceLine: 1— the one line number the bug cannot reach, because at offset 0 there is no preceding terminator for\s*to eat.scripts/taskToggleLineEndings.test.tsdrives the real document session over both line endings, with and without a blank line above the list, across every marker shape. Reverting the pattern fails six cases and leaves the LF-without-blank-line ones passing — so the fixtures reproduce the bug rather than merely asserting the fix.Validation
npm test863 ·npm run check0 errors ·cargo test141 ·npm audit0 vulnerabilitiesNot verified: no run on Windows or Linux, and the rendering fixes are confirmed by screenshot on macOS only.
🤖 Generated with Claude Code