From 6d086a0ccadf119ceb0bb340e0b1b969df5ba0e5 Mon Sep 17 00:00:00 2001 From: Frederic Barthelemy Date: Tue, 14 Jul 2026 14:45:03 -0700 Subject: [PATCH 1/4] docs(specs): add GH13725 spec for markdown viewer / anchors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Warp's inline parser has no HTML-tag concept at all beyond the special case, so raw / tags render as literal text. Separately, and more importantly, a markdown-native [text](#fragment) link already parses today but resolves as a plain URL — there's no anchor/slug concept anywhere in the content model, so neither heading-based nor explicit fragment targets can be resolved. This adds the product + tech spec for tier-zero tag #1 of #13652 (split into #13725): parse as a Hyperlink (small, reuses the existing link styling/click path), give headings implicit GitHub-style slugs and let / register explicit anchors (medium — a genuinely new content- model concept, not an extension of an existing type), and wire fragment clicks to the scroll-to-character-offset autoscroll API that already exists (request_autoscroll_to_exact_vertical). Phases the work so headings + ship first, since that slice also fixes markdown-native fragment links and is the highest-value piece; arbitrary targets are phase 2. Nested under specs/GH13725/ (per the specs/APP-4319// chain precedent). --- specs/GH13725/product.md | 144 ++++++++++++++++++++ specs/GH13725/tech.md | 288 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 432 insertions(+) create mode 100644 specs/GH13725/product.md create mode 100644 specs/GH13725/tech.md diff --git a/specs/GH13725/product.md b/specs/GH13725/product.md new file mode 100644 index 00000000000..b2a80c08a42 --- /dev/null +++ b/specs/GH13725/product.md @@ -0,0 +1,144 @@ +# PRODUCT.md — Markdown viewer: ``/`` anchor links + +Issue: https://github.com/warpdotdev/warp/issues/13725 + +Split from: #13652 (bulk raw-HTML-subset request, closed in favor of per-feature issues). +Sibling specs in the same split: `` sizing (#13721), raw HTML tables (#13726, +`specs/GH13652/tables/`), `
`/`` (#10259), `
` (#13732), `` +(#13733), ``/`` (#13734), `align` (#13735), ``/`` (#13736). + +## Summary + +Hand-built READMEs and docs commonly pair `` (or `name="section"`) anchor +targets with `[Jump to Section](#section)` or `` links, either as a +manual table of contents or as inline cross-references. Warp's Markdown viewer supports +neither half of this pattern today: raw ``/`` tags render as literal text +(there is no HTML-tag token in the inline parser at all), and even the markdown-native +`[text](#fragment)` form — which **does** parse today as a link whose target is the literal +string `#fragment` — has nothing to resolve that fragment against, because no heading or +anchor in the document carries an id/slug of any kind. + +This request is therefore two related but separable capabilities: + +1. **Raw HTML anchor tags.** Recognize inline `…` as a hyperlink (reusing + the existing `Hyperlink` link styling) and ``/`` as a named anchor + target attached to the surrounding content. +2. **Fragment resolution + scroll-to.** Give every heading an implicit anchor (a slug + derived from its text, GitHub-style), let an explicit `` register an anchor too, + and make any link whose target is `#fragment` — whether it came from `` or from + markdown-native `[text](#fragment)` — scroll the viewer to the matching anchor instead + of falling through to plain-URL handling. + +Capability (2) is the harder half and is also what markdown-native links need — a +`[text](#fragment)` link already parses correctly and gets no benefit from HTML anchor +parsing at all. The issue explicitly frames working in-document anchor links as a +prerequisite for any future table-of-contents feature (#13083, #4720). + +Figma: none provided. + +## Goals / Non-goals + +In scope: + +- Parse an inline `link text` as a hyperlink with the given text, styled + and clickable exactly like a markdown `[link text](…)` — including external URLs + (`https://…`) and in-page fragments (`#target`). +- Parse `` and `` (empty or self-closing, the common + hand-authored form) as a named anchor target at that point in the document, with no + visible rendering of its own (matches GitHub/browser behavior — an anchor tag with no + text renders nothing). +- Give every heading (`#`…`######`) an **implicit** anchor slug derived from its rendered + text, so `[Jump to Target Section](#target-section)` works against ordinary headings with + zero authoring effort — the common case the issue's test document exercises. +- Resolve a `#fragment` hyperlink click (from either an `` tag or a markdown + `[text](#fragment)` link) against the set of anchors in the current document — explicit + ``/`` targets and implicit heading slugs — and scroll the viewer so the + target is visible, instead of the current behavior (treated as an opaque URL). +- A `#fragment` link with no matching anchor in the document degrades gracefully: it + remains a normally-styled, clickable-looking link, but clicking it is a no-op (no + navigation, no error, no crash) rather than attempting to open `#fragment` as a URL. +- `` attributes beyond `href` — `title`, `target`, `rel`, `class`, etc. — are + parsed-but-ignored: they don't break parsing, and they don't do anything (no `target` + window semantics, no HTML tooltip from `title`). + +Out of scope (explicit non-goals): + +- **Cross-document/cross-tab fragment links** (e.g. `[text](other-file.md#section)` + jumping into a different open document or tab). This spec covers same-document + resolution only; the tech spec should note whether the chosen anchor-index design leaves + room for that later. +- **Slug-collision policy beyond "first wins."** If two headings produce the same slug + (e.g. two headings both literally titled "Overview"), only the first is addressable by + that slug — matching common Markdown-renderer behavior (GitHub disambiguates duplicates + by appending `-1`, `-2`, …; replicating that exact disambiguation scheme is left to the + tech spec's judgment, not mandated here). +- **`` tags with both `href` and `id`/`name` on the same element.** The issue's test + case and the common real-world pattern always use them separately (`` as a bare + target, `` as the link). Supporting both roles on one tag is not required. +- **Any other raw-HTML tag** (``, ``, `
`, etc.) — each has its own + spec in this split. +- **Editing/authoring affordances** — e.g. no "copy anchor link" UI, no auto-slug preview + while typing a heading. This is a *rendering/navigation* feature only. +- **URL scheme validation or link-target security beyond what markdown links already do.** + `` reuses the exact same `Hyperlink::Url` styling and click path as markdown + links; it inherits that trust boundary as-is rather than introducing a new one. + +## Behavior + +1. `Visit Warp` renders as a clickable link reading "Visit + Warp", visually and behaviorally identical to the markdown link + `[Visit Warp](https://warp.dev)`. + +2. `Jump to Target Section` renders as a clickable link. + Clicking it scrolls the viewer so the heading (or explicit anchor) matching + `target-section` is visible. This is the HTML-tag half of the issue's test case. + +3. The markdown-native equivalent, `[Jump to Target Section](#target-section)`, gets the + **same** scroll-to-target click behavior as invariant 2 — it already parses as a link + today; only fragment *resolution* is new. This is the contrasting case named explicitly + in the issue ("resolves as a plain URL hyperlink" today). + +4. A heading `## Target Section` is addressable by `#target-section` (its GitHub-style + slug: lowercased, spaces to hyphens, punctuation stripped) with no authoring effort — + no `` required. This is what makes invariants 2 and 3 work against the issue's + test document out of the box. + +5. `` (or ``) placed anywhere in + the document — most commonly immediately before a heading, as a hand-authored anchor — + registers `target-section` as a jump target and renders no visible content itself. + +6. If both an explicit `` and a heading whose implicit slug is also `x` exist, + the explicit `` is the effective target for `#x` (explicit authoring intent wins + over the derived default) — the tech spec should confirm this is achievable without + extra complexity; if not, document the fallback order chosen instead. + +7. A `#fragment` link that matches nothing in the document remains a normal-looking, + clickable link. Clicking it does nothing observable (no scroll, no error dialog, no + attempt to open it as an external URL). This must not panic or freeze the viewer. + +8. Non-`href`/`id`/`name` attributes on `` (`title`, `target`, `rel`, `class`, inline + `style`, …) are accepted without breaking the parse; they have no behavioral effect. + +9. `` supports the same inline content markdown links do at minimum — plain text. + Bold/italic/code *inside* the anchor text is a nice-to-have the tech spec may size + separately; if infeasible in this slice, plain-text-only anchor content is an acceptable + MVP as long as it's called out. + +10. Malformed anchor tags (unterminated ``, `href`/`id` with no + value) degrade to literal text for that tag, without swallowing the rest of the + paragraph or document and without panicking. + +## Suggested phasing + +The two capabilities compound in value but are separately shippable: + +- **Phase 1:** `` inline links (parsed via the existing `Hyperlink` link-styling + machinery) **and** heading auto-anchors + fragment click resolution + scroll-to. This + alone delivers the issue's headline case — an inline HTML link or a markdown + `[text](#heading)` link jumping to a heading — and is the highest-value slice because it + fixes markdown-native fragment links too, not just the new HTML tag. +- **Phase 2:** Arbitrary ``/`` targets (anchors not attached to a heading). + Lower value on its own — most real documents anchor at headings — but completes the + issue's hand-built-table-of-contents use case for authors who anchor mid-paragraph. + +The tech spec should confirm or revise this split based on actual implementation cost. diff --git a/specs/GH13725/tech.md b/specs/GH13725/tech.md new file mode 100644 index 00000000000..b3bb7760e1f --- /dev/null +++ b/specs/GH13725/tech.md @@ -0,0 +1,288 @@ +# TECH.md — Markdown viewer: ``/`` anchor links + +Product spec: `specs/GH13725/product.md` +GitHub issue: https://github.com/warpdotdev/warp/issues/13725 +Sibling specs in the same split: `specs/GH13652/tables/` (raw HTML tables), and specs to +follow for `` sizing, `
`/``, `
`, ``, ``/``, +`align`, ``/``. + +## Context + +This is two mostly-independent pieces of work bolted together by the issue, and they should +be evaluated separately because their feasibility differs enormously: + +- **`` as a hyperlink tag: small.** Warp's inline parser has no HTML-tag concept at + all — `InlineToken` (`crates/markdown_parser/src/markdown_parser.rs:1674-1694`) covers + `Delimiter`, `Text`, `BackslashEscape`, `HtmlEntity`, `CodeSpan`, `AutoLink`, `LinkEnd`, + `UnderlineEnd`. The **only** literal HTML tag special-cased anywhere in the inline grammar + is ``/`` (`parse_inline_token_underline_start`/`_end`, :1628-1645), which is handled + exactly like a markdown delimiter pair (push a `Delimiter`/`UnderlineEnd` token, no + attribute parsing). `…` is a direct structural analog: a start delimiter + carrying one piece of data (the `href` value) and a fixed end tag. The existing markdown + link path, `parse_link` (:1116-1179) + `parse_link_target` (:1183-1271), already builds + exactly the styling this needs — `styles.hyperlink = Some(Hyperlink::Url(url))` + (:1149) — so an HTML `` reader can reuse `Hyperlink::Url` as its output type; it only + needs a new *front door* into that styling, not a new link model. +- **Fragment resolution + scroll-to: medium, and this is where the real work is.** A + markdown link `[text](#fragment)` **already parses today** — `parse_link_target` has no + opinion on what a link destination looks like beyond balanced parens/brackets, so + `#fragment` is accepted as a URL string exactly like `https://…` would be (confirmed at + `markdown_parser_tests.rs:1646-1653`, "Example 501": `[link](#fragment)` → + `Hyperlink::Url("#fragment")`). The gap is entirely downstream of parsing: nothing in the + content model or render/click path treats a `#`-prefixed `Hyperlink::Url` differently from + an external URL. + - `FormattedTextHeader` (`crates/markdown_parser/src/lib.rs:303-306`) is + `{ heading_size: usize, text: FormattedTextInline }` — no id/slug field. No heading + carries any addressable identity today. + - `grep -rn "slug\|anchor" crates/markdown_parser/ crates/editor/` (excluding this spec's + own additions) turns up nothing — there is no slug-generation code anywhere in the repo + to reuse. + - The hyperlink click path is `FormattedTextElement::register_default_click_handlers` + (`crates/warpui_core/src/elements/gui/formatted_text_element.rs:334-365`): for each + `Hyperlink::Url(url)` found via `line.hyperlinks(false)`, a registered callback receives + `HyperlinkUrl { url }` on click (:358-359). Today every consumer of this callback treats + `url` as an opener target (external browser / new tab) — there is no branch anywhere + that inspects the string for a leading `#` and no code path back into the editor's own + scroll state from here. + - **The scroll primitive this needs already exists**, which is the good news: + `EditorRenderState::request_autoscroll_to_exact_vertical(character_offset: CharOffset, + pixel_delta: Pixels)` (`crates/editor/src/render/model/mod.rs:3064-3074`) submits + `LayoutAction::Autoscroll { mode: AutoScrollMode::ScrollToExactVertical { .. } }`, handled + at :3694-3700+, and there's a simpler `scroll_to(ScrollPositionSnapshot)` / + `LayoutAction::ScrollTo` (:2951-2952, handled :3145-3148) for a raw scroll-top target. + Both already work in terms of a **character offset into the document**, which is exactly + what a resolved anchor needs to produce. The missing piece isn't "how do we scroll" — a + character-offset-based autoscroll API is already used elsewhere (selection-follow) — it's + "how do we go from `#target-section` to a character offset." + +This framing matters for scoping: the visible, "does it look done" work (parsing `` +text into a blue underlined link) is the cheap part. The part that makes the issue's test +case actually pass — `[Jump to Target Section](#target-section)` scrolling to the heading +below it — requires building an anchor index and wiring a new click branch, neither of which +exist in any form today. + +## Feasibility summary + +- **(i) `` inline parsing → `Hyperlink::Url`: SMALL.** New `InlineToken` variant(s) + (or extend delimiter handling, mirroring ``) plus an attribute-extraction step for + `href`. Reuses 100% of the existing link styling/click/render path once the token exists. +- **(ii) ``/`` as an invisible anchor token: SMALL–MEDIUM.** Needs a new + concept — a zero-width marker attached to a document position — since nothing in + `FormattedTextFragment`/`FormattedTextLine` represents "renders nothing, but is + addressable here." Smaller than (iii) because it's purely additive (a new fragment/marker + kind), not a change to an existing shared struct. + - **The `` half of this feature genuinely doesn't fit the current content model in + ANY existing type**, unlike table `
` (`specs/GH13652/tables/tech.md` item 1), which + at least had an existing multi-line cell rendering path to extend. This is closer to net + new plumbing: an anchor is not text, not a delimiter, not a link — it's an id-to-position + binding that must survive from parse time through to click-resolution time. +- **(iii) Heading auto-slugs + anchor index + fragment click resolution: MEDIUM–LARGE.** + Three sub-parts, each real but bounded: + - Slug generation from heading text (GitHub-style: lowercase, spaces→hyphens, strip + punctuation, dedupe via `-1`/`-2` suffixes). Pure function, no existing code to build on + (product non-goal notes dedupe policy is flexible), straightforward to unit test in + isolation. + - An **anchor index**: id → position, built once per document (or incrementally per edit) + from (a) every heading's slug and (b) every ``/`` marker found during + parse. This needs a place to live — likely alongside or inside the existing document/ + buffer model that already tracks headings for other purposes (e.g. whatever powers + "jump to heading" style navigation, if Warp has one — **needs verification**; if no such + index exists yet, this is genuinely new state, not a variant of something already + tracked). + - Click-time resolution: extend (or add a sibling to) + `register_default_click_handlers`'s callback so a `Hyperlink::Url` starting with `#` is + looked up in the anchor index instead of being handed to the URL-open callback, then + calls `request_autoscroll_to_exact_vertical`/`scroll_to` with the resolved offset. A + miss (product invariant 7) simply does nothing — no fallback to "open `#fragment` as a + URL" (that would be actively wrong; today's behavior of doing that is precisely the bug + being fixed). + +This spec recommends implementing **(i) + (iii)** as phase 1 (matches the product spec's +phasing — this is the slice that fixes the issue's headline test case and also repairs +markdown-native `[text](#heading)` links, which get zero benefit from (ii) alone), and +**(ii)** as phase 2. + +## Proposed changes + +### 1. `text` inline token (phase 1) + +Add HTML-anchor delimiter tokens to `InlineToken` +(`crates/markdown_parser/src/markdown_parser.rs:1674-1694`), following the `` precedent +exactly in shape but carrying data: + +- A start token that captures the `href` attribute value — unlike ``'s zero-data + `Delimiter { kind: UnderlineStart, count: 1 }`, this needs the URL string threaded through + to close-time, so it likely needs its own variant rather than reusing `Delimiter` verbatim + (e.g. `InlineToken::HtmlAnchorStart(String)` for the href, paired with a `HtmlAnchorEnd` + token on ``), or the URL could live in a small ad hoc attribute parser called at + `` applies `styles.hyperlink = Some(Hyperlink::Url(href))` to the fragments + between start and end — the exact same `backtrack_styles` call `parse_link` makes at + :1148-1150, just triggered by `` instead of `]`+`(url)`. +- A minimal attribute parser for the opening tag: extract `href="…"` (single/double-quoted), + tolerate and discard any other attributes (`title`, `target`, `rel`, `class`, …) per + product invariant 8 — this can be a small nom combinator scanning `key="value"` pairs + without needing a general HTML tokenizer (the existing paste-path parser in + `html_parser.rs` already depends on `html5ever`, but pulling that into the inline-token + grammar for a single-tag case is likely overkill; a purpose-built attribute scanner is + more consistent with how `` is handled today — call this choice out for maintainer + review, since `html5ever` is an available and more robust alternative if the ad hoc parser + proves fragile against real-world `` markup). +- Malformed input (unterminated ``) falls back to + literal text for the tag, matching how `parse_link` falls back to a literal `]` on failure + (:1170-1177) — product invariant 10. + +### 2. Heading slugs (phase 1) + +Add a `slug: String` (or `Option` if collision/empty-text edge cases need to opt +out) field to `FormattedTextHeader` (`crates/markdown_parser/src/lib.rs:303-306`), computed +at parse time from the heading's rendered text. This is a **shared-struct change** like the +table `
` cell-type change in the sibling spec — audit callers of +`FormattedTextHeader { .. }` construction and pattern matches before landing (`grep -rn +"FormattedTextHeader" crates/`) to size the ripple; expect this to be small since the field +is purely additive. + +Slug algorithm (GitHub-compatible, since that's the ecosystem convention the product spec +points to): lowercase, strip characters outside `[a-z0-9 -]`, collapse/trim spaces, replace +spaces with `-`. Deduplicate across the document by appending `-1`, `-2`, … to repeats, +processed in document order — this requires slug generation to happen as a document-wide +pass (or with access to prior headings' slugs), not purely per-heading, so it likely belongs +as a post-process over the parsed `FormattedText.lines` rather than inline in the heading +parser itself. + +### 3. Anchor index (phase 1 for headings, phase 2 extends it for ``) + +Build a document-scoped `HashMap` (id → character offset of the target) +by walking `FormattedText.lines` after parsing: + +- Every `FormattedTextLine::Header(h)` contributes `h.slug → `. +- (Phase 2) every ``/`` marker contributes `id → `. +- Product invariant 6 (explicit `` wins over a same-named implicit heading slug): + since phase 2 markers are indexed after or alongside headings, insert order (or an + explicit "explicit beats implicit" rule in the insert step) resolves the collision — flag + for implementation whether `HashMap::insert` overwrite order alone is sufficient or needs + an explicit priority check. + +Where this index lives and how it's invalidated on edit is the main open question — needs +verification against however Warp already tracks per-document derived state (if anything +comparable exists for, say, syntax-highlighting spans or the table offset maps referenced in +the tables spec, follow that pattern; if nothing comparable exists, this is new +per-document cached state that must be recomputed on edit, which the tech implementer should +scope against the render/relayout lifecycle in `render/model/mod.rs`). + +### 4. Fragment-aware click resolution (phase 1) + +`register_default_click_handlers` (`formatted_text_element.rs:334-365`) currently maps every +`Hyperlink::Url(url)` to the same `HyperlinkUrl { url }` callback unconditionally (:358-359). +Add a branch: if `url` starts with `#`, resolve `url[1..]` against the anchor index instead +of invoking the URL-open callback. + +- **Hit:** call `request_autoscroll_to_exact_vertical(character_offset, Pixels::zero())` (or + `scroll_to` with a `ScrollPositionSnapshot` built from the offset, whichever the + surrounding `EventContext`/`AppContext` at the click site can most directly construct — + needs verification of which of the two APIs is reachable from + `formatted_text_element.rs`'s click callback without new plumbing). +- **Miss:** do nothing (product invariant 7) — explicitly *not* falling through to the + URL-open callback with `#fragment` as a literal URL, which is today's bug. +- This requires `register_default_click_handlers` (or its caller in the Markdown-viewer + wiring, not yet located — **needs verification** of exactly where the viewer instantiates + `FormattedTextElement` and supplies the click callback) to have access to the anchor index + from (3) at click time. + +### 5. ``/`` anchor markers (phase 2) + +Represent a bare anchor as a new zero-width construct — the cleanest option is a +`FormattedTextFragment` with empty `text` and a new style/marker field (e.g. +`styles.anchor_id: Option`), since fragments are already the unit `hyperlinks()` +and similar traversal helpers walk over; a marker-as-empty-fragment reuses that traversal +for free. An alternative is a dedicated `FormattedTextLine` variant, but that's a heavier +change (every line-level consumer would need a new match arm) for something that's +conceptually inline, not block-level — recommend the fragment-marker approach unless review +surfaces a reason `FormattedTextLine` fits better. + +`visible text` (both an id and content in one tag) is out of scope per product +non-goal — the phase-2 reader only needs to handle the empty/self-closing form +(`` or ``), simplifying the parser considerably (no need to also +apply link/text styling in this path). + +### 6. Feature gating + +Recommend a **new** feature flag (e.g. `FeatureFlag::MarkdownAnchorLinks`) rather than +riding an existing one — unlike the tables spec, there's no existing "structural HTML" +flag this naturally extends, and gating separately lets phase 1 (headings + ``) ship +independently of phase 2 (``) if their cost estimates diverge during implementation. + +### 7. Security + +`` reuses `Hyperlink::Url` verbatim — no new trust boundary, no script/event-handler +attributes are read (only `href`; all others parsed-but-discarded per product invariant 8). +Fragment resolution never leaves the document (no network, no file access) — a `#fragment` +click either scrolls within the current buffer or is a no-op. ``/`` values are +used only as HashMap keys for in-document lookup, never interpolated into a URL, path, or +shell context. + +## Testing and validation + +### Parser unit tests (`crates/markdown_parser/src/markdown_parser_tests.rs`) + +- `Visit Warp` → `Hyperlink::Url` fragment identical in + shape to the equivalent markdown link (invariant 1). +- `Jump` → `Hyperlink::Url("#target")` fragment (invariant 2) — + parsing only; resolution is tested separately below. +- Attributes beyond `href` (`title`, `target="_blank"`, `class="x"`) parsed-but-ignored, no + effect on output (invariant 8). +- Unterminated `` / missing closing `` → literal text fallback, rest of + paragraph intact (invariant 10). +- Heading slug generation: plain text, text with punctuation/mixed case, duplicate headings + → `-1`/`-2` suffixes (invariant 4, product non-goal on exact dedupe scheme — assert the + chosen behavior, not GitHub's exact algorithm unless matched intentionally). +- (Phase 2) `` / `` → zero-width anchor marker, no visible + text emitted (invariant 5). +- (Phase 2) `text` (both id and content on one tag) — confirm documented + behavior (out of scope; assert it doesn't panic, even if unspecified which role wins). + +### Anchor index / resolution tests (`crates/editor/` — exact module TBD per item 3) + +- Document with a heading and a `[text](#slug)` link → click resolves to the heading's + character offset (invariants 2, 3, 4). +- `` and markdown `[text](#slug)` targeting the same heading → identical + resolved offset (invariant 3 — the two syntaxes must be equivalent post-resolution). +- `#fragment` with no matching anchor → click is a no-op, no panic, link still renders + normally (invariant 7). +- (Phase 2) Explicit `` colocated with a heading whose implicit slug is also `x` + → explicit anchor wins per invariant 6, or documented fallback if not achievable. + +### Integration / manual + +Per CONTRIBUTING, before/after screenshots plus a short recording reproducing the issue's +motivating test document verbatim: the raw-HTML `` jump, the +markdown-native `[Jump to Target Section](#target-section)` jump (contrast case — today +resolves as a plain URL), the external `` link, and the `` marker preceding the heading. Confirm scroll lands the heading at +or near the top of the viewport (exact positioning behavior — top-align vs. some offset — is +an implementation choice the manual pass should sanity-check against `request_autoscroll_to_ +exact_vertical`'s existing `pixel_delta` semantics). + +## Risks and follow-ups + +- **The anchor index's storage/invalidation lifecycle is the single biggest unknown.** + Everything else in this spec (slug generation, click resolution, the `` marker + itself) is bounded, ordinary parser/render work. Where a per-document id→offset map lives, + how it's kept in sync with edits, and whether an existing analogous cache already exists to + extend — none of this is verified yet and should be the first thing implementation + confirms, since it could shift (iii) from MEDIUM to LARGE if no such lifecycle hook exists. +- **``'s attribute parser (item 1) needs a maintainer call between a purpose-built + scanner and reusing `html5ever`.** The purpose-built path matches the `` precedent's + spirit (minimal, inline-grammar-native) but is more exposed to malformed real-world HTML + than a real parser; `html5ever` is already a dependency of the crate (via + `html_parser.rs`) so there's no new-dependency cost either way. +- **Interaction with the HTML-table spec:** an ``/`` inside a table cell should + work automatically once cell inline content is parsed via the same `parse_phrasing_content` + path the tables spec already plans to reuse — verify once both land, no explicit design + change anticipated here. +- **Cross-document fragment links (explicit non-goal) are the natural next step** once + same-document resolution ships, and the anchor-index design should be sanity-checked + against not painting that in a corner (e.g. keying the index by document id in addition to + fragment id would future-proof it cheaply) — not required for this slice, but worth a + design glance before implementation locks in the index's shape. From d2122a06fb93ac2d29e2eda59a4d44fe7fdd4ff7 Mon Sep 17 00:00:00 2001 From: Frederic Barthelemy Date: Fri, 17 Jul 2026 13:28:14 -0700 Subject: [PATCH 2/4] Retarget anchor spec at the existing fragment-scroll path (GH13725) The prior draft claimed no #-fragment click branch existed and that the fix site was FormattedTextElement::register_default_click_handlers. On master the Markdown viewer already routes #-fragment clicks through maybe_open_url -> scroll_to_matching_header -> find_matching_header, which walks outline_blocks() live per click and scrolls via request_autoscroll_to. The real gap is that find_matching_header compares against exact lowercased heading text, not a GitHub slug. Rescope phase 1 to slug-normalize inside find_matching_header (no anchor index, no cache, no invalidation lifecycle), refresh drifted line refs, add render-surface (GUI vs TUI) and user-content-prefix notes, and flag the untracked html_tag_support_tests.rs probes that must be inverted. --- specs/GH13725/product.md | 4 + specs/GH13725/tech.md | 356 ++++++++++++++++++++++++--------------- 2 files changed, 223 insertions(+), 137 deletions(-) diff --git a/specs/GH13725/product.md b/specs/GH13725/product.md index b2a80c08a42..4eb2ea7bedf 100644 --- a/specs/GH13725/product.md +++ b/specs/GH13725/product.md @@ -82,6 +82,10 @@ Out of scope (explicit non-goals): - **URL scheme validation or link-target security beyond what markdown links already do.** `` reuses the exact same `Hyperlink::Url` styling and click path as markdown links; it inherits that trust boundary as-is rather than introducing a new one. +- **Anchor scroll-to in the terminal (TUI) Markdown renderer.** `` links will + *render* in the TUI viewer for free (it shares the parser), but clicking a `#fragment` + there does not scroll — the TUI has no scroll model or click-to-navigate path. Fragment + resolution is a GUI-viewer feature in this slice. ## Behavior diff --git a/specs/GH13725/tech.md b/specs/GH13725/tech.md index b3bb7760e1f..85626cce90b 100644 --- a/specs/GH13725/tech.md +++ b/specs/GH13725/tech.md @@ -12,54 +12,75 @@ This is two mostly-independent pieces of work bolted together by the issue, and be evaluated separately because their feasibility differs enormously: - **`` as a hyperlink tag: small.** Warp's inline parser has no HTML-tag concept at - all — `InlineToken` (`crates/markdown_parser/src/markdown_parser.rs:1674-1694`) covers + all — `InlineToken` (`crates/markdown_parser/src/markdown_parser.rs:1684`) covers `Delimiter`, `Text`, `BackslashEscape`, `HtmlEntity`, `CodeSpan`, `AutoLink`, `LinkEnd`, `UnderlineEnd`. The **only** literal HTML tag special-cased anywhere in the inline grammar - is ``/`` (`parse_inline_token_underline_start`/`_end`, :1628-1645), which is handled + is ``/`` (`parse_inline_token_underline_start`/`_end`, :1635/:1648), which is handled exactly like a markdown delimiter pair (push a `Delimiter`/`UnderlineEnd` token, no attribute parsing). `…` is a direct structural analog: a start delimiter carrying one piece of data (the `href` value) and a fixed end tag. The existing markdown - link path, `parse_link` (:1116-1179) + `parse_link_target` (:1183-1271), already builds + link path, `parse_link` (:1125) + `parse_link_target` (:1192), already builds exactly the styling this needs — `styles.hyperlink = Some(Hyperlink::Url(url))` - (:1149) — so an HTML `` reader can reuse `Hyperlink::Url` as its output type; it only + (:1158) — so an HTML `` reader can reuse `Hyperlink::Url` as its output type; it only needs a new *front door* into that styling, not a new link model. -- **Fragment resolution + scroll-to: medium, and this is where the real work is.** A +- **Fragment resolution + scroll-to: small, because the scaffolding already exists.** A markdown link `[text](#fragment)` **already parses today** — `parse_link_target` has no opinion on what a link destination looks like beyond balanced parens/brackets, so `#fragment` is accepted as a URL string exactly like `https://…` would be (confirmed at - `markdown_parser_tests.rs:1646-1653`, "Example 501": `[link](#fragment)` → - `Hyperlink::Url("#fragment")`). The gap is entirely downstream of parsing: nothing in the - content model or render/click path treats a `#`-prefixed `Hyperlink::Url` differently from - an external URL. - - `FormattedTextHeader` (`crates/markdown_parser/src/lib.rs:303-306`) is - `{ heading_size: usize, text: FormattedTextInline }` — no id/slug field. No heading - carries any addressable identity today. - - `grep -rn "slug\|anchor" crates/markdown_parser/ crates/editor/` (excluding this spec's - own additions) turns up nothing — there is no slug-generation code anywhere in the repo - to reuse. - - The hyperlink click path is `FormattedTextElement::register_default_click_handlers` - (`crates/warpui_core/src/elements/gui/formatted_text_element.rs:334-365`): for each - `Hyperlink::Url(url)` found via `line.hyperlinks(false)`, a registered callback receives - `HyperlinkUrl { url }` on click (:358-359). Today every consumer of this callback treats - `url` as an opener target (external browser / new tab) — there is no branch anywhere - that inspects the string for a leading `#` and no code path back into the editor's own - scroll state from here. - - **The scroll primitive this needs already exists**, which is the good news: - `EditorRenderState::request_autoscroll_to_exact_vertical(character_offset: CharOffset, - pixel_delta: Pixels)` (`crates/editor/src/render/model/mod.rs:3064-3074`) submits - `LayoutAction::Autoscroll { mode: AutoScrollMode::ScrollToExactVertical { .. } }`, handled - at :3694-3700+, and there's a simpler `scroll_to(ScrollPositionSnapshot)` / - `LayoutAction::ScrollTo` (:2951-2952, handled :3145-3148) for a raw scroll-top target. - Both already work in terms of a **character offset into the document**, which is exactly - what a resolved anchor needs to produce. The missing piece isn't "how do we scroll" — a - character-offset-based autoscroll API is already used elsewhere (selection-follow) — it's - "how do we go from `#target-section` to a character offset." + `markdown_parser_tests.rs:1644-1647`, "Example 501": `[link](#fragment)` → + `Hyperlink::Url("#fragment")`). And the click path **already** treats a `#`-prefixed + `Hyperlink::Url` differently from an external URL: `maybe_open_url` + (`app/src/notebooks/editor/view.rs:1971`) routes it to `scroll_to_matching_header` rather + than the URL opener. The gap is narrow and specific: the matcher compares the fragment + against the heading's exact lowercased text instead of a GitHub-style slug, so a hyphenated + fragment misses a spaced heading. Fixing that one comparison is the core of the resolution + work — no new content-model field and no new click branch are required for headings. + - `FormattedTextHeader` (`crates/markdown_parser/src/lib.rs:303`) is + `{ heading_size: usize, text: FormattedTextInline }` — no id/slug field. But this no + longer matters for phase 1: heading matching happens in the editor at click time via + `find_matching_header`, which re-reads each heading's text out of the live buffer + (`content.text_in_range(...)`) on every click — it never consulted a parse-time slug + field, so none needs to be added to `FormattedTextHeader`. + - `grep -rn "slug" crates/markdown_parser/ crates/editor/` turns up no slug-*generation* + helper — the normalization function has to be written — but the *matching loop* it plugs + into already exists (`find_matching_header`, `app/src/notebooks/editor/model.rs:1351`), + so this is a small localized change, not new plumbing. + - **A `#`-fragment click path already exists in the Markdown viewer** — this is the key + fact that reshapes the scoping below. The viewer's click entry point is + `NotebookEditorView::maybe_open_url` + (`app/src/notebooks/editor/view.rs:1955`), and it **already branches on a leading `#`**: + at :1971-1983, `if url.starts_with('#')` it calls + `model.scroll_to_matching_header(&url, ctx)` and returns early on a hit, only falling + through to ordinary URL-open handling on a miss. So the fragment-scroll wiring the earlier + draft of this spec treated as net-new does not need to be built — it is live on master. + (The general `FormattedTextElement::register_default_click_handlers` helper at + `crates/warpui_core/src/elements/gui/formatted_text_element.rs:336` is used by many other + surfaces — settings pages, banners, modals, the changelog, AI views — but **not** by the + notebook Markdown viewer, and none of those callers has a `#`-fragment branch. It is not + the fix site; do not target it.) + - **The gap is the matching rule, not the scroll or the click branch.** + `scroll_to_matching_header` (`app/src/notebooks/editor/model.rs:1335`) delegates to + `find_matching_header` (:1351), which walks `content.outline_blocks()`, filters to + `BlockType::Text(BufferBlockStyle::Header { .. })`, and compares the fragment against + `heading.trim().to_lowercase()` (:1374) — i.e. the **exact lowercased heading text**, not + a GitHub-style slug. So `#target-section` (hyphenated) misses a heading titled + "Target Section" (spaced), which is precisely the issue's failing case. It does already + strip the `#` prefix and `urlencoding::decode` the fragment (:1352-1356), so URL-escaped + fragments are handled; only the text-vs-slug comparison is wrong. + - **The scroll primitive is already wired end-to-end.** On a match, + `scroll_to_matching_header` calls + `render_state.request_autoscroll_to(AutoScrollMode::PositionOffsetInViewportCenter(range.start))` + (`app/src/notebooks/editor/model.rs:1346`), backed by + `EditorRenderState::request_autoscroll_to` (`crates/editor/src/render/model/mod.rs:3054`, + with `PositionOffsetInViewportCenter` handled at :3661/:3712). Nothing new is needed on the + scroll side — a resolved heading range already scrolls today. This framing matters for scoping: the visible, "does it look done" work (parsing `` -text into a blue underlined link) is the cheap part. The part that makes the issue's test -case actually pass — `[Jump to Target Section](#target-section)` scrolling to the heading -below it — requires building an anchor index and wiring a new click branch, neither of which -exist in any form today. +text into a blue underlined link) is one piece. The part that makes the issue's headline test +case pass — `[Jump to Target Section](#target-section)` scrolling to the heading below it — is +**far smaller than the earlier draft claimed**: the click branch, the scroll call, and the +per-click heading iteration all already exist in `find_matching_header`. What's missing is slug +normalization inside that one function so a hyphenated fragment matches a spaced heading. ## Feasibility summary @@ -76,26 +97,29 @@ exist in any form today. at least had an existing multi-line cell rendering path to extend. This is closer to net new plumbing: an anchor is not text, not a delimiter, not a link — it's an id-to-position binding that must survive from parse time through to click-resolution time. -- **(iii) Heading auto-slugs + anchor index + fragment click resolution: MEDIUM–LARGE.** - Three sub-parts, each real but bounded: - - Slug generation from heading text (GitHub-style: lowercase, spaces→hyphens, strip - punctuation, dedupe via `-1`/`-2` suffixes). Pure function, no existing code to build on - (product non-goal notes dedupe policy is flexible), straightforward to unit test in - isolation. - - An **anchor index**: id → position, built once per document (or incrementally per edit) - from (a) every heading's slug and (b) every ``/`` marker found during - parse. This needs a place to live — likely alongside or inside the existing document/ - buffer model that already tracks headings for other purposes (e.g. whatever powers - "jump to heading" style navigation, if Warp has one — **needs verification**; if no such - index exists yet, this is genuinely new state, not a variant of something already - tracked). - - Click-time resolution: extend (or add a sibling to) - `register_default_click_handlers`'s callback so a `Hyperlink::Url` starting with `#` is - looked up in the anchor index instead of being handed to the URL-open callback, then - calls `request_autoscroll_to_exact_vertical`/`scroll_to` with the resolved offset. A - miss (product invariant 7) simply does nothing — no fallback to "open `#fragment` as a - URL" (that would be actively wrong; today's behavior of doing that is precisely the bug - being fixed). +- **(iii) Heading slug matching + fragment click resolution: SMALL.** The click branch, the + scroll call, and the per-click heading walk all already exist in `find_matching_header` + (`app/src/notebooks/editor/model.rs:1351`). The only work is: + - Slug normalization (GitHub-style: lowercase, spaces→hyphens, strip punctuation). This is + a pure function with no existing code to build on, straightforward to unit test in + isolation. It gets applied inside `find_matching_header`: normalize the incoming fragment + and normalize each heading's text with the **same** function, then compare — replacing the + current `heading.trim().to_lowercase() == target` check (:1374). Both sides run through + one normalizer, so a hyphenated fragment matches a spaced heading. + - **No new anchor index.** `find_matching_header` already iterates `content.outline_blocks()` + and reads each heading's text live from the buffer on every click, so there is no + id→offset map to build, no place for it to live, and no cache-invalidation problem — the + function recomputes against the current buffer each time it runs. (Dedupe of collision + slugs, if wanted, is likewise "first match wins" as a natural consequence of the loop + returning on the first hit — no separate `-1`/`-2` bookkeeping is required for phase 1; + the product spec's non-goal already accepts first-wins.) + - Miss behavior (product invariant 7) is already correct: `find_matching_header` returns + `None` → `scroll_to_matching_header` returns `false` → `maybe_open_url` falls through. + The one nuance to preserve: on a miss today, `maybe_open_url` still hands `#fragment` to + the URL opener (view.rs:1984+), which for an in-document fragment is a no-op in practice + but should be confirmed not to surface a broken-link tooltip; if it does, the miss path + should early-return instead of falling through. Flag this for the implementer to verify + against invariant 7's "no error dialog" clause. This spec recommends implementing **(i) + (iii)** as phase 1 (matches the product spec's phasing — this is the slice that fixes the issue's headline test case and also repairs @@ -107,7 +131,7 @@ markdown-native `[text](#heading)` links, which get zero benefit from (ii) alone ### 1. `text` inline token (phase 1) Add HTML-anchor delimiter tokens to `InlineToken` -(`crates/markdown_parser/src/markdown_parser.rs:1674-1694`), following the `` precedent +(`crates/markdown_parser/src/markdown_parser.rs:1684`), following the `` precedent exactly in shape but carrying data: - A start token that captures the `href` attribute value — unlike ``'s zero-data @@ -116,10 +140,10 @@ exactly in shape but carrying data: (e.g. `InlineToken::HtmlAnchorStart(String)` for the href, paired with a `HtmlAnchorEnd` token on ``), or the URL could live in a small ad hoc attribute parser called at `` applies `styles.hyperlink = Some(Hyperlink::Url(href))` to the fragments between start and end — the exact same `backtrack_styles` call `parse_link` makes at - :1148-1150, just triggered by `` instead of `]`+`(url)`. + :1157-1158, just triggered by `` instead of `]`+`(url)`. - A minimal attribute parser for the opening tag: extract `href="…"` (single/double-quoted), tolerate and discard any other attributes (`title`, `target`, `rel`, `class`, …) per product invariant 8 — this can be a small nom combinator scanning `key="value"` pairs @@ -133,62 +157,86 @@ exactly in shape but carrying data: literal text for the tag, matching how `parse_link` falls back to a literal `]` on failure (:1170-1177) — product invariant 10. -### 2. Heading slugs (phase 1) +### 2. Heading slug normalization (phase 1) -Add a `slug: String` (or `Option` if collision/empty-text edge cases need to opt -out) field to `FormattedTextHeader` (`crates/markdown_parser/src/lib.rs:303-306`), computed -at parse time from the heading's rendered text. This is a **shared-struct change** like the -table `
` cell-type change in the sibling spec — audit callers of -`FormattedTextHeader { .. }` construction and pattern matches before landing (`grep -rn -"FormattedTextHeader" crates/`) to size the ripple; expect this to be small since the field -is purely additive. +No parse-time change and **no field on `FormattedTextHeader`**. Heading matching happens in +the editor at click time via `find_matching_header` +(`app/src/notebooks/editor/model.rs:1351`), which already reads each heading's text out of +the live buffer (`content.text_in_range(...)`, :1371-1373) on every click. The fix is a single +slug normalizer applied on both sides of the comparison inside that function. Slug algorithm (GitHub-compatible, since that's the ecosystem convention the product spec points to): lowercase, strip characters outside `[a-z0-9 -]`, collapse/trim spaces, replace -spaces with `-`. Deduplicate across the document by appending `-1`, `-2`, … to repeats, -processed in document order — this requires slug generation to happen as a document-wide -pass (or with access to prior headings' slugs), not purely per-heading, so it likely belongs -as a post-process over the parsed `FormattedText.lines` rather than inline in the heading -parser itself. - -### 3. Anchor index (phase 1 for headings, phase 2 extends it for ``) - -Build a document-scoped `HashMap` (id → character offset of the target) -by walking `FormattedText.lines` after parsing: - -- Every `FormattedTextLine::Header(h)` contributes `h.slug → `. -- (Phase 2) every ``/`` marker contributes `id → `. -- Product invariant 6 (explicit `` wins over a same-named implicit heading slug): - since phase 2 markers are indexed after or alongside headings, insert order (or an - explicit "explicit beats implicit" rule in the insert step) resolves the collision — flag - for implementation whether `HashMap::insert` overwrite order alone is sufficient or needs - an explicit priority check. - -Where this index lives and how it's invalidated on edit is the main open question — needs -verification against however Warp already tracks per-document derived state (if anything -comparable exists for, say, syntax-highlighting spans or the table offset maps referenced in -the tables spec, follow that pattern; if nothing comparable exists, this is new -per-document cached state that must be recomputed on edit, which the tech implementer should -scope against the render/relayout lifecycle in `render/model/mod.rs`). - -### 4. Fragment-aware click resolution (phase 1) - -`register_default_click_handlers` (`formatted_text_element.rs:334-365`) currently maps every -`Hyperlink::Url(url)` to the same `HyperlinkUrl { url }` callback unconditionally (:358-359). -Add a branch: if `url` starts with `#`, resolve `url[1..]` against the anchor index instead -of invoking the URL-open callback. - -- **Hit:** call `request_autoscroll_to_exact_vertical(character_offset, Pixels::zero())` (or - `scroll_to` with a `ScrollPositionSnapshot` built from the offset, whichever the - surrounding `EventContext`/`AppContext` at the click site can most directly construct — - needs verification of which of the two APIs is reachable from - `formatted_text_element.rs`'s click callback without new plumbing). -- **Miss:** do nothing (product invariant 7) — explicitly *not* falling through to the - URL-open callback with `#fragment` as a literal URL, which is today's bug. -- This requires `register_default_click_handlers` (or its caller in the Markdown-viewer - wiring, not yet located — **needs verification** of exactly where the viewer instantiates - `FormattedTextElement` and supplies the click callback) to have access to the anchor index - from (3) at click time. +spaces with `-`. Add it as a small pure function (natural home: alongside `find_matching_header` +in `model.rs`, or a shared helper if a second caller appears). It should be unit-testable in +isolation on `&str → String`. + +`find_matching_header` already normalizes the incoming fragment as `target = fragment +.strip_prefix('#')` → `urlencoding::decode` → `trim().to_lowercase()` (:1352-1357). Replace +that trailing `to_lowercase()` with the slug normalizer, and change the per-heading comparison +at :1374 from `heading.trim().to_lowercase() == target` to `slug(&heading) == target`. Both +sides then run through the same normalizer, so `#target-section` matches a heading "Target +Section". No document-wide pass and no prior-heading state are needed — first-match-wins for +collisions falls out of the loop returning on the first hit, which satisfies the product +non-goal's "first wins" dedupe stance. + +### 3. No anchor index (phase 1) + +There is deliberately no anchor index in phase 1. The earlier draft of this spec proposed a +document-scoped `HashMap` rebuilt per edit, and treated its storage and +invalidation lifecycle as the biggest unknown — but that entire structure is unnecessary +because `find_matching_header` already performs the id→offset lookup live: it iterates +`content.outline_blocks()`, filters to headers, and returns the matching header's +`start..end` range directly from the current buffer. Iterating a handful of outline blocks on +a click is cheap, and re-reading the buffer each time means there is nothing to keep in sync +with edits. **No new per-document cached state, no invalidation hook, no lifecycle question.** + +Phase 2 (``/`` markers) needs the parsed anchor positions to be reachable at +click time, and that is where any indexing question actually lives — deferred to that phase's +design, not phase 1's. + +### 4. Fragment-aware click resolution — already exists (phase 1) + +The click branch does **not** need to be built: `maybe_open_url` +(`app/src/notebooks/editor/view.rs:1955`) already does, on a leading `#` (:1971-1983), call +`scroll_to_matching_header`, which on a hit calls +`request_autoscroll_to(AutoScrollMode::PositionOffsetInViewportCenter(range.start))` +(`app/src/notebooks/editor/model.rs:1346`). The `#`-branch, the resolution call, and the +scroll are all live on master. Do **not** target +`FormattedTextElement::register_default_click_handlers` — that helper is used by unrelated +surfaces and is not on the Markdown viewer's click path. + +The only phase-1 behavior change here is downstream of the slug fix in item 2: + +- **Hit:** already correct — `find_matching_header` returns a range, + `scroll_to_matching_header` requests the autoscroll, `maybe_open_url` early-returns + (view.rs:1978-1982). Once item 2's slug normalization lands, headings that previously missed + now resolve. +- **Miss:** `find_matching_header` returns `None` → `scroll_to_matching_header` returns + `false` → `maybe_open_url` falls through to the ordinary URL path. Per product invariant 7 + this must be observably inert (no scroll, no error, no crash). Confirm the fall-through does + not raise a broken-link tooltip for an in-document `#fragment`; if it does, add an + early-return on the `#`-prefixed miss instead of falling through. This is the one spot the + implementer must check by hand against invariant 7. + +### Render surfaces (GUI vs. TUI) + +Master now has a second Markdown render surface — the TUI renderer at +`crates/warp_tui/src/tui_markdown.rs` — and it shares the same `markdown_parser` crate and the +same `FormattedText`/`Hyperlink` model as the GUI. This splits the feature cleanly: + +- **`` *parsing* (item 1) benefits both surfaces for free.** Because the new + `Hyperlink::Url` fragments are produced in the shared parser, the TUI renderer picks them up + with no TUI-specific code: `inline_spans` already reads + `fragment.styles.hyperlink → Some(Hyperlink::Url(url))` and renders the link text styled but + inert (`crates/warp_tui/src/tui_markdown.rs:194-196`). So an `` link will *display* + correctly in the TUI the moment the parser change lands. +- **Fragment *scroll* resolution (items 2–4) is GUI-only.** The whole resolution path lives in + `app/src/notebooks/editor/` (`maybe_open_url`, `scroll_to_matching_header`, + `find_matching_header`) — the TUI has no `maybe_open_url` equivalent and no scroll model to + drive, so there is nothing to hook. `#fragment` clicks resolving to a scroll are explicitly + **out of scope for the TUI surface** in this slice; the TUI simply renders the link as inert + styled text. Call this out so a reviewer doesn't read "shared parser" as "shared behavior." ### 5. ``/`` anchor markers (phase 2) @@ -218,12 +266,33 @@ independently of phase 2 (``) if their cost estimates diverge during imple `` reuses `Hyperlink::Url` verbatim — no new trust boundary, no script/event-handler attributes are read (only `href`; all others parsed-but-discarded per product invariant 8). Fragment resolution never leaves the document (no network, no file access) — a `#fragment` -click either scrolls within the current buffer or is a no-op. ``/`` values are -used only as HashMap keys for in-document lookup, never interpolated into a URL, path, or +click either scrolls within the current buffer or is a no-op. ``/`` values +(phase 2) are used only as in-document lookup keys, never interpolated into a URL, path, or shell context. +**On GitHub's `user-content-` prefix (intentionally not replicated).** GitHub's renderer +prefixes the *rendered DOM ids* it emits for headings and `` anchors with +`user-content-` (to avoid collisions with GitHub's own page-chrome ids), while leaving the +`href="#…"` fragments authors write unprefixed; a small piece of client JS bridges the two at +click time. Warp has no such collision surface and no DOM: resolution happens **in-process**, +comparing the fragment against slugs computed live from heading text (item 2), not against any +emitted id attribute. So Warp deliberately does **not** add a `user-content-` prefix on either +side — there is nothing for it to disambiguate against, and adding it would only break parity +with the plain `#slug` fragments authors actually write. Worth stating explicitly so a +reviewer familiar with GitHub's scheme doesn't flag its absence as a bug. + ## Testing and validation +**Invert the existing negative-behavior probes first.** The master checkout carries an +untracked probe file, `crates/markdown_parser/src/html_tag_support_tests.rs` (wired via +`#[path] mod html_tag_support_tests;` at `markdown_parser.rs:1998-1999`), whose two anchor +tests — `test_raw_html_anchor_href_not_parsed_as_hyperlink` and +`test_raw_html_anchor_id_not_registered_as_target` — currently assert the **no-op** status +quo (raw `` produces *no* hyperlink style; raw `` registers *no* target). Once +item 1 lands, both assertions become false and CI goes red. The implementation must **invert +them into positive assertions** (`` *does* produce a `Hyperlink::Url`; `` *does* +register an anchor) as part of the same change, not leave them asserting the old behavior. + ### Parser unit tests (`crates/markdown_parser/src/markdown_parser_tests.rs`) - `Visit Warp` → `Hyperlink::Url` fragment identical in @@ -234,22 +303,28 @@ shell context. effect on output (invariant 8). - Unterminated `` / missing closing `` → literal text fallback, rest of paragraph intact (invariant 10). -- Heading slug generation: plain text, text with punctuation/mixed case, duplicate headings - → `-1`/`-2` suffixes (invariant 4, product non-goal on exact dedupe scheme — assert the - chosen behavior, not GitHub's exact algorithm unless matched intentionally). +- Slug normalizer (item 2): plain text, text with punctuation/mixed case, multi-space runs + → normalized slug (invariant 4). Test the pure `&str → String` function directly; first-wins + collision behavior is covered by the resolution tests below, not the normalizer. - (Phase 2) `` / `` → zero-width anchor marker, no visible text emitted (invariant 5). - (Phase 2) `text` (both id and content on one tag) — confirm documented behavior (out of scope; assert it doesn't panic, even if unspecified which role wins). -### Anchor index / resolution tests (`crates/editor/` — exact module TBD per item 3) +### Resolution tests (`app/src/notebooks/editor/` — `find_matching_header` / `model.rs`) + +These target the matcher directly, since that is where the phase-1 change lives. -- Document with a heading and a `[text](#slug)` link → click resolves to the heading's - character offset (invariants 2, 3, 4). -- `` and markdown `[text](#slug)` targeting the same heading → identical - resolved offset (invariant 3 — the two syntaxes must be equivalent post-resolution). -- `#fragment` with no matching anchor → click is a no-op, no panic, link still renders - normally (invariant 7). +- Heading `## Target Section` + fragment `#target-section` → `find_matching_header` returns + the heading's range; today (exact-text match) it returns `None`. This is the regression the + slug normalizer fixes (invariants 2, 3, 4). +- `` and markdown `[text](#slug)` targeting the same heading resolve to the + same range (invariant 3 — the two syntaxes are equivalent because both reach + `find_matching_header` via the same `#`-branch in `maybe_open_url`). +- Fragment with no matching heading → `find_matching_header` returns `None`, + `scroll_to_matching_header` returns `false`, no panic (invariant 7). +- First-wins collision: two headings normalizing to the same slug → the fragment resolves to + the first, exercised by asserting the returned range is the earlier heading's. - (Phase 2) Explicit `` colocated with a heading whose implicit slug is also `x` → explicit anchor wins per invariant 6, or documented fallback if not achievable. @@ -259,19 +334,25 @@ Per CONTRIBUTING, before/after screenshots plus a short recording reproducing th motivating test document verbatim: the raw-HTML `` jump, the markdown-native `[Jump to Target Section](#target-section)` jump (contrast case — today resolves as a plain URL), the external `` link, and the `` marker preceding the heading. Confirm scroll lands the heading at -or near the top of the viewport (exact positioning behavior — top-align vs. some offset — is -an implementation choice the manual pass should sanity-check against `request_autoscroll_to_ -exact_vertical`'s existing `pixel_delta` semantics). +id="target-section">` marker preceding the heading. Confirm scroll lands the heading in +view (the existing call uses `AutoScrollMode::PositionOffsetInViewportCenter`, i.e. the target +is centered rather than top-aligned — sanity-check that this reads well for the anchor-jump +case, since it is the behavior already shipped for other `#`-fragment jumps and changing it +would affect them too). ## Risks and follow-ups -- **The anchor index's storage/invalidation lifecycle is the single biggest unknown.** - Everything else in this spec (slug generation, click resolution, the `` marker - itself) is bounded, ordinary parser/render work. Where a per-document id→offset map lives, - how it's kept in sync with edits, and whether an existing analogous cache already exists to - extend — none of this is verified yet and should be the first thing implementation - confirms, since it could shift (iii) from MEDIUM to LARGE if no such lifecycle hook exists. +- **No anchor-index lifecycle risk in phase 1.** An earlier draft of this spec called a + per-document id→offset map's storage and invalidation the single biggest unknown. That risk + is now moot: phase 1 builds no index (item 3). `find_matching_header` resolves live against + the current buffer per click, so there is nothing to store, sync, or invalidate. The only + residual judgment call is cosmetic — whether the existing center-in-viewport autoscroll + reads well for anchor jumps (noted in the manual-testing section). +- **The one behavioral check the implementer must not skip** is the miss path (item 4): today + a `#`-fragment that resolves to nothing falls through to the URL opener. Confirm that + fall-through is observably inert (no broken-link tooltip) per invariant 7, and add an + early-return if it is not. This is the sole place phase-1 correctness is not already + guaranteed by existing master behavior. - **``'s attribute parser (item 1) needs a maintainer call between a purpose-built scanner and reusing `html5ever`.** The purpose-built path matches the `` precedent's spirit (minimal, inline-grammar-native) but is more exposed to malformed real-world HTML @@ -282,7 +363,8 @@ exact_vertical`'s existing `pixel_delta` semantics). path the tables spec already plans to reuse — verify once both land, no explicit design change anticipated here. - **Cross-document fragment links (explicit non-goal) are the natural next step** once - same-document resolution ships, and the anchor-index design should be sanity-checked - against not painting that in a corner (e.g. keying the index by document id in addition to - fragment id would future-proof it cheaply) — not required for this slice, but worth a - design glance before implementation locks in the index's shape. + same-document resolution ships. Phase 1's live-resolution approach doesn't paint this into a + corner — a cross-document jump would resolve against a *different* document's buffer, which + is an orthogonal addition to `find_matching_header`'s target selection rather than a change + to any cached index shape (there is none). Worth a design glance if phase 2 introduces an + `` index, but nothing in phase 1 constrains it. From 645e363b989cec1de3d7ecbbe5a8f730bab11bb0 Mon Sep 17 00:00:00 2001 From: Frederic Barthelemy Date: Fri, 17 Jul 2026 13:46:21 -0700 Subject: [PATCH 3/4] Add cross-document fragment navigation as phase 3 Restore the cut cross-document non-goal as an in-scope phase, sized MEDIUM after confirming a fragment-less relative link already opens and focuses the target Markdown-viewer tab today. The remaining work is carrying the #section through the existing file-open flow and draining it as a deferred scroll once the destination document parses, reusing the same find_matching_header slug resolver as the same-document jump. --- specs/GH13725/product.md | 35 +++++++++-- specs/GH13725/tech.md | 125 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 147 insertions(+), 13 deletions(-) diff --git a/specs/GH13725/product.md b/specs/GH13725/product.md index 4eb2ea7bedf..eb6a311c21f 100644 --- a/specs/GH13725/product.md +++ b/specs/GH13725/product.md @@ -54,6 +54,13 @@ In scope: `[text](#fragment)` link) against the set of anchors in the current document — explicit ``/`` targets and implicit heading slugs — and scroll the viewer so the target is visible, instead of the current behavior (treated as an opaque URL). +- **Cross-document fragment links** — a link whose target combines a relative file path and + a fragment, e.g. `[text](other-file.md#section)`. Clicking it opens (or, if already open, + focuses) that file's Markdown-viewer tab and scrolls it to the matching anchor once the + document has loaded. A relative file link *without* a fragment already opens the target + today; this in-scope item is specifically the fragment half — carrying the `#section` + through the file-open flow and scrolling after load. Deferred to phase 3 (see phasing), + because it builds on the same-document slug resolver rather than replacing it. - A `#fragment` link with no matching anchor in the document degrades gracefully: it remains a normally-styled, clickable-looking link, but clicking it is a no-op (no navigation, no error, no crash) rather than attempting to open `#fragment` as a URL. @@ -63,10 +70,15 @@ In scope: Out of scope (explicit non-goals): -- **Cross-document/cross-tab fragment links** (e.g. `[text](other-file.md#section)` - jumping into a different open document or tab). This spec covers same-document - resolution only; the tech spec should note whether the chosen anchor-index design leaves - room for that later. +- **Following a cross-document fragment link into a non-Markdown target, or into an + external editor.** The cross-document scroll-to-anchor (in-scope above, phase 3) applies + only when the target file opens in Warp's Markdown viewer. If the user's Markdown Viewer + preference is off — so the file opens in the code editor or an external app — the + fragment is dropped and only the file opens, matching how a plain relative file link + behaves today. Bridging a `#slug` anchor to a code-editor line is a separate concern. +- **Cross-document anchor scroll in the terminal (TUI) Markdown renderer.** Same rationale + as the same-document TUI non-goal below — the TUI has no scroll model or click-to-open + path. `other-file.md#section` in the TUI renders as inert styled text. - **Slug-collision policy beyond "first wins."** If two headings produce the same slug (e.g. two headings both literally titled "Overview"), only the first is addressable by that slug — matching common Markdown-renderer behavior (GitHub disambiguates duplicates @@ -132,6 +144,15 @@ Out of scope (explicit non-goals): value) degrade to literal text for that tag, without swallowing the rest of the paragraph or document and without panicking. +11. (Phase 3) Clicking `[text](other-file.md#section)` opens `other-file.md` in the + Markdown viewer (or focuses its tab if already open — the same open/focus behavior a + plain `other-file.md` link has today) and, once that document has loaded, scrolls it to + the `section` anchor using the same slug resolution as a same-document jump. If + `other-file.md` opens but has no matching anchor, the outcome degrades to the + same-document miss (invariant 7): the file is shown, unscrolled, no error. If the file + itself cannot be resolved (does not exist relative to the document), clicking is a + no-op, matching a broken plain relative link today. + ## Suggested phasing The two capabilities compound in value but are separately shippable: @@ -144,5 +165,11 @@ The two capabilities compound in value but are separately shippable: - **Phase 2:** Arbitrary ``/`` targets (anchors not attached to a heading). Lower value on its own — most real documents anchor at headings — but completes the issue's hand-built-table-of-contents use case for authors who anchor mid-paragraph. +- **Phase 3:** Cross-document fragment links (`other-file.md#section`). Independent of + phases 1–2 in spirit but built on phase 1's slug resolver: the file-open, tab-focus, and + dedup machinery already exists (a fragment-less relative link opens today), so this phase + adds only fragment carry-through plus a deferred scroll after the target document loads. + Sequenced last because it reuses — rather than reshapes — the same-document resolver, so + it gains from phase 1 landing first. The tech spec should confirm or revise this split based on actual implementation cost. diff --git a/specs/GH13725/tech.md b/specs/GH13725/tech.md index 85626cce90b..60038ee3683 100644 --- a/specs/GH13725/tech.md +++ b/specs/GH13725/tech.md @@ -121,10 +121,21 @@ normalization inside that one function so a hyphenated fragment matches a spaced should early-return instead of falling through. Flag this for the implementer to verify against invariant 7's "no error dialog" clause. +- **(iv) Cross-document fragment navigation (`other-file.md#section`): MEDIUM.** The + file-open, tab-focus, and dedup are free — a fragment-less relative link opens the target + in the Markdown viewer today (`resolve_and_open` → `OpenFileNotebook` → + `open_file_notebook`, `app/src/workspace/view.rs:8470`). The remaining work is bounded: + split the `#section` off before file resolution, thread it to the destination pane + (mirroring the code editor's existing `line_and_column` plumbing), and drain it as a + **deferred scroll** once the new document parses — the one genuinely new element, since + there is no on-load hook to reuse. Sized above SMALL because of that new deferred-scroll + state and the multi-hop field plumbing; below LARGE because the targeting, dedup, and + scroll primitives all already exist. Full mechanism in **item 6a**. + This spec recommends implementing **(i) + (iii)** as phase 1 (matches the product spec's phasing — this is the slice that fixes the issue's headline test case and also repairs -markdown-native `[text](#heading)` links, which get zero benefit from (ii) alone), and -**(ii)** as phase 2. +markdown-native `[text](#heading)` links, which get zero benefit from (ii) alone), +**(ii)** as phase 2, and **(iv)** as phase 3. ## Proposed changes @@ -261,6 +272,88 @@ riding an existing one — unlike the tables spec, there's no existing "structur flag this naturally extends, and gating separately lets phase 1 (headings + ``) ship independently of phase 2 (``) if their cost estimates diverge during implementation. +### 6a. Cross-document fragment navigation (phase 3) + +**Size: MEDIUM.** The file-open half is free — a fragment-less relative link +(`other-file.md`) already opens the target in the Markdown viewer today, including +tab-focus and dedup. What's missing is carrying the `#section` through that flow and +scrolling after the *new* document parses. This is bounded, well-scoped plumbing, not new +targeting machinery — but it is more than a single parameter through one existing call, and +it needs a new piece of deferred state on the destination editor because the scroll target +can't be applied until the document is loaded and there is no on-load hook today. + +**What already works (verified on master).** Clicking a relative link with no fragment +routes through `maybe_open_url` (`app/src/notebooks/editor/view.rs:1955`); because it does +not start with `#`, it goes to `NotebookLinks::resolve_and_open` +(`app/src/notebooks/link.rs:323`). `resolve` (:128) resolves the relative path against the +session's base directory (:211-222), `resolve_file` (:235) confirms it exists on disk, and +`open` (:258) emits `LinkEvent::OpenFileNotebook` for a Markdown target (:282/:290). That +event is consumed at `app/src/pane_group/pane/notebook_pane.rs:175`, re-emitted as +`Event::OpenFileInWarp`, and handled by `Workspace::open_file_notebook` +(`app/src/workspace/view.rs:8470`) — which **already de-dupes an open pane and focuses it** +(:8489-8494) or opens a new tab/split (:8503-8520). So open, focus, and dedup for the target +document are live; only the fragment is lost. + +**Why the fragment is lost today.** The path is cleaned by +`CleanPathResult::with_line_and_column_number` (`crates/warp_util/src/path.rs:158`), whose +`LINE_AND_COLUMN_REGEX` (:47) strips `:line[:col]`, `[l, c]`, and `#L100`-style suffixes +(:48-56) but **not** a bare `#section` fragment. So `other-file.md#section` survives cleaning +intact, is resolved as a literal on-disk path, misses (`ResolveError::FileNotFound`), and +`resolve_and_open`'s closure silently drops the error (:328-332) — the current no-op. + +**The precedent for "open a file AND position the viewport" already exists — for the code +editor.** `LinkTarget::LocalFile` carries `line_and_column` (link.rs:35) end-to-end, and +`add_tab_for_code_file` (`app/src/workspace/view.rs:12828`) threads it through to position the +code editor. The Markdown-notebook path is the gap: `add_tab_for_file_notebook` (:12767) and +`open_file_notebook` (:8470) carry only a `path`, no viewport target, and `OpenFileNotebook` +(link.rs:441) has no fragment field. + +**Concrete mechanism (three localized changes):** + +1. **Split the fragment before file resolution.** In `NotebookLinks::resolve` + (link.rs:128), for a link that is neither a parseable URL nor a bare `#fragment`, peel a + trailing `#…` off the string before it reaches `CleanPathResult`, keeping it as an + `Option anchor` alongside the cleaned path. (A bare `#fragment` is still handled + earlier by `maybe_open_url`'s `starts_with('#')` branch — this split only affects strings + that have a path *and* a fragment.) Once the fragment is removed, `resolve_file` finds the + real file and the existing open/focus flow runs unchanged. + +2. **Thread the fragment to the destination pane**, mirroring how `line_and_column` already + rides the code path. Add `anchor: Option` to `LinkTarget::LocalFile` (link.rs:33), + to `LinkEvent::OpenFileNotebook` (link.rs:441), to `pane_group::Event::OpenFileInWarp` + (`app/src/pane_group/mod.rs:549`), and through `open_file_notebook` (view.rs:8470) into the + `FilePane`/notebook it constructs. This is additive field-plumbing along an existing event + chain — the analog of the code editor's `line_and_column`, which already proves the shape + works end-to-end. + +3. **Apply the scroll after the destination document loads — the one genuinely new piece.** + The same-document jump can call `scroll_to_matching_header` immediately because the buffer + is already parsed. A freshly opened notebook is not: the offset the slug resolves to does + not exist until parse completes, and there is **no on-load callback in the notebook-open + flow to hang the scroll on** (`open_file_notebook` constructs the pane and returns; nothing + fires when its content finishes parsing). The destination editor model therefore needs a + small piece of **deferred-scroll state** — a `pending_anchor: Option` set at + construction — that is consumed once, on the first successful parse/layout, by calling the + existing `scroll_to_matching_header` (`app/src/notebooks/editor/model.rs:1335`) with the + pending fragment and then clearing it. This reuses phase 1's resolver verbatim; the only + new logic is *when* to call it. The implementer must locate the model's + content-ready/relayout point (the notebook already rebuilds layout on content load — e.g. + the `rebuild_layout` path around `model.rs:1315`) and drain `pending_anchor` there. If no + anchor matches after load, draining is a no-op — identical to the same-document miss + (product invariant 7). + +**Resolution reuse.** The slug comparison is entirely unchanged: the destination scroll runs +through the same `find_matching_header` (`app/src/notebooks/editor/model.rs:1351`) the +same-document jump uses, so phase 1's slug normalizer (item 2) is the cross-document resolver +too — no second matcher, no divergence. This is the concrete sense in which phase 1 "leaves +room" for cross-document navigation: the target document's resolver is the *same function*, +invoked after open instead of in place. + +**Non-Markdown / external-editor targets.** If the Markdown Viewer preference is off, `open` +routes the file to `open_file` → the code editor or system handler (link.rs:284), which has +no slug concept. Per the product non-goal, the fragment is simply dropped in that case: the +file opens, unscrolled. Only the `OpenFileNotebook` branch carries the anchor. + ### 7. Security `` reuses `Hyperlink::Url` verbatim — no new trust boundary, no script/event-handler @@ -327,6 +420,13 @@ These target the matcher directly, since that is where the phase-1 change lives. the first, exercised by asserting the returned range is the earlier heading's. - (Phase 2) Explicit `` colocated with a heading whose implicit slug is also `x` → explicit anchor wins per invariant 6, or documented fallback if not achievable. +- (Phase 3) Fragment-split in `resolve`: `other-file.md#section` yields cleaned path + `other-file.md` + anchor `section`; `other-file.md` (no fragment) yields no anchor; + `other-file.md#L10` still routes to the existing line-number path, not the anchor path + (guard against regressing the `#L`-suffix handling in `CleanPathResult`). +- (Phase 3) A resolved `LinkTarget::LocalFile` for a `#section` link carries the anchor + through to `OpenFileNotebook` (assert on the emitted event, mirroring the existing + `link_tests.rs:432` `OpenFileNotebook` assertion). ### Integration / manual @@ -334,7 +434,11 @@ Per CONTRIBUTING, before/after screenshots plus a short recording reproducing th motivating test document verbatim: the raw-HTML `` jump, the markdown-native `[Jump to Target Section](#target-section)` jump (contrast case — today resolves as a plain URL), the external `` link, and the `` marker preceding the heading. Confirm scroll lands the heading in +id="target-section">` marker preceding the heading. (Phase 3) additionally: clicking +`[text](other-file.md#section)` opens/focuses `other-file.md` and lands on its `section` +heading after load; re-clicking when the tab is already open focuses and re-scrolls rather +than opening a duplicate; a `#section` with no matching heading in the opened file shows the +file unscrolled with no error. Confirm scroll lands the heading in view (the existing call uses `AutoScrollMode::PositionOffsetInViewportCenter`, i.e. the target is centered rather than top-aligned — sanity-check that this reads well for the anchor-jump case, since it is the behavior already shipped for other `#`-fragment jumps and changing it @@ -362,9 +466,12 @@ would affect them too). work automatically once cell inline content is parsed via the same `parse_phrasing_content` path the tables spec already plans to reuse — verify once both land, no explicit design change anticipated here. -- **Cross-document fragment links (explicit non-goal) are the natural next step** once - same-document resolution ships. Phase 1's live-resolution approach doesn't paint this into a - corner — a cross-document jump would resolve against a *different* document's buffer, which - is an orthogonal addition to `find_matching_header`'s target selection rather than a change - to any cached index shape (there is none). Worth a design glance if phase 2 introduces an - `` index, but nothing in phase 1 constrains it. +- **Cross-document fragment links are now specced as phase 3 (item 6a), not just gestured + at.** Investigation confirmed a fragment-less relative link already opens/focuses the target + Markdown-viewer tab today, so phase 3 is MEDIUM (carry the fragment through + deferred + scroll after load), not LARGE. Phase 1's live-resolution approach doesn't paint this into a + corner: the cross-document jump resolves against the *destination* document's buffer using + the **same** `find_matching_header` slug resolver, invoked after open instead of in place — + so nothing in phase 1 forecloses it, and phase 1 landing first is what phase 3 builds on. + The one new element phase 3 introduces is deferred-scroll state on the destination editor + (there is no on-load hook to reuse); see item 6a and product invariant 11. From cc40c91f81133fda573a3c8e9b45d12cf64af7c9 Mon Sep 17 00:00:00 2001 From: Frederic Barthelemy Date: Sat, 18 Jul 2026 15:16:01 -0700 Subject: [PATCH 4/4] Address review findings on anchor spec (GH13725) - Specify a genuinely GitHub-compatible slug normalizer that preserves Unicode word characters (accented Latin, CJK) instead of stripping to ASCII, so non-English README anchors keep working. - Specify phase 2's anchor lookup: extend the same live click-time walk find_matching_header already uses (no cache), reading fragment anchor_id styles alongside the header-outline pass, with a defined explicit-anchor-wins precedence rule over colliding heading slugs. - Replace the untracked-probe-file testing plan with committed test cases in markdown_parser_tests.rs and the editor resolution tests, including Unicode slug cases and the phase 2 precedence cases; frame the existing probe file as historical context only. --- specs/GH13725/tech.md | 127 +++++++++++++++++++++++++++++++++++------- 1 file changed, 106 insertions(+), 21 deletions(-) diff --git a/specs/GH13725/tech.md b/specs/GH13725/tech.md index 60038ee3683..9f313c9239a 100644 --- a/specs/GH13725/tech.md +++ b/specs/GH13725/tech.md @@ -176,11 +176,33 @@ the editor at click time via `find_matching_header` the live buffer (`content.text_in_range(...)`, :1371-1373) on every click. The fix is a single slug normalizer applied on both sides of the comparison inside that function. -Slug algorithm (GitHub-compatible, since that's the ecosystem convention the product spec -points to): lowercase, strip characters outside `[a-z0-9 -]`, collapse/trim spaces, replace -spaces with `-`. Add it as a small pure function (natural home: alongside `find_matching_header` -in `model.rs`, or a shared helper if a second caller appears). It should be unit-testable in -isolation on `&str → String`. +**Slug algorithm — genuinely GitHub-compatible, not ASCII-only.** GitHub's heading slugger +(`gfm-auto-identifiers`, the same rule set GitHub Pages/`github/cmark-gfm` implement) does +**not** strip non-ASCII text: it lowercases (Unicode-aware), removes characters that are +punctuation/symbols per the Unicode categories it excludes, converts spaces to `-`, and +otherwise **preserves any Unicode alphanumeric/word character**, including accented Latin, +CJK, Cyrillic, etc. A heading `"Café Société"` slugs to `café-société`, not `caf-socit`; +`"日本語"` slugs to `日本語`. An ASCII-only `[a-z0-9 -]` filter silently breaks every non-English +README anchor, which is a real regression against the product goal, not a cosmetic gap. + +Concretely, the normalizer should: + +1. Lowercase the input (Unicode-aware lowercasing, e.g. Rust's `str::to_lowercase`, not an + ASCII-only lowercase — this already matters for e.g. Turkish/German/accented casing). +2. Strip characters that are ASCII punctuation/symbols outside `-` and whitespace (mirroring + GFM's exclusion set: roughly `!"#$%&'()*+,./:;<=>?@[\]^`{|}~`), while leaving all other + Unicode letters/digits/marks (CJK, accented Latin, etc.) untouched. This is the one place + the two algorithms diverge from a naive "strip non-ASCII" reading — the exclusion set is + defined by *punctuation class*, not by *ASCII range*. +3. Replace runs of whitespace with a single `-`, trim leading/trailing `-`. +4. Do **not** apply `user-content-` or any other prefix (see the Security section's note on + why Warp doesn't need GitHub's DOM-collision prefix). + +Add it as a small pure function (natural home: alongside `find_matching_header` in +`model.rs`, or a shared helper if a second caller appears). It should be unit-testable in +isolation on `&str → String`, with cases spanning plain ASCII, accented Latin, and CJK +headings (see the Testing section's updated case list) — the Unicode-preserving behavior is +exactly the kind of thing that regresses silently if only ASCII cases are tested. `find_matching_header` already normalizes the incoming fragment as `target = fragment .strip_prefix('#')` → `urlencoding::decode` → `trim().to_lowercase()` (:1352-1357). Replace @@ -203,8 +225,9 @@ a click is cheap, and re-reading the buffer each time means there is nothing to with edits. **No new per-document cached state, no invalidation hook, no lifecycle question.** Phase 2 (``/`` markers) needs the parsed anchor positions to be reachable at -click time, and that is where any indexing question actually lives — deferred to that phase's -design, not phase 1's. +click time. It resolves this the same way phase 1 does — a live walk at click time, no cached +index — detailed concretely in item 5 below, including the precedence rule for an explicit +anchor and a heading slug that collide. ### 4. Fragment-aware click resolution — already exists (phase 1) @@ -265,6 +288,42 @@ non-goal — the phase-2 reader only needs to handle the empty/self-closing form (`` or ``), simplifying the parser considerably (no need to also apply link/text styling in this path). +**Lookup strategy at click time: extend the same live walk, no cache.** `find_matching_header` +already re-walks `content.outline_blocks()` on every click rather than consulting a cached +index (item 3) — phase 2 extends that same function rather than introducing a second, +differently-shaped lookup: + +- `outline_blocks()` returns block-level markers (`BlockType::Text(BufferBlockStyle::Header { + .. })`, `CodeBlock`, `PlainText`, list/table markers, etc.) — it does not itself walk inline + fragments within a block, so it cannot see an `anchor_id` marker sitting mid-paragraph. + `find_matching_header` therefore needs a second pass alongside the header scan: for each + block returned by `outline_blocks()`, additionally read that block's fragments (via the same + `content` accessor the header path already uses to pull heading text, + `content.text_in_range`/the fragment-level equivalent) and check each fragment's + `styles.anchor_id` for a match, before or interleaved with the existing header-text + comparison. Concretely: rename/extend `find_matching_header` (or add a sibling + `find_matching_anchor` called first) so the combined resolver checks explicit anchors and + heading slugs in one pass over the document, still with no persisted state — the walk itself + is the "index," recomputed per click exactly as phase 1 established. +- This keeps phase 2 architecturally consistent with phase 1's core decision (item 3): no + `HashMap`, no invalidation hook, no lifecycle question. The only new + per-click cost is reading fragment styles in addition to block outlines, which is the same + order of cost as the existing header-text read. + +**Precedence when an explicit `` and a heading slug collide on the same fragment.** +Define one deterministic rule: **explicit anchors win.** Concretely, the combined resolver +checks explicit `anchor_id` fragments for an exact match against the (undecoded, as-authored) +id first; only if no explicit anchor matches does it fall back to the slug-normalized heading +comparison from item 2. Rationale: an `` is an author's literal, intentional target +string — it should never be shadowed by an incidental heading-slug collision the author didn't +control (e.g. two different headings elsewhere in the doc normalizing to the same slug, see +item 2's "first-wins" dedupe among headings). This rule composes cleanly with item 2's +first-occurrence-wins rule, which continues to apply only *among* headings when no explicit +anchor is present — i.e. the full ordering is: (1) exact match against an explicit ``/`` value, first occurrence wins if multiple share an id; (2) else, slug-normalized +match against a heading, first occurrence wins among headings. Both levels reuse the same +single-pass walk, so there is no separate index or second traversal for the fallback tier. + ### 6. Feature gating Recommend a **new** feature flag (e.g. `FeatureFlag::MarkdownAnchorLinks`) rather than @@ -376,15 +435,25 @@ reviewer familiar with GitHub's scheme doesn't flag its absence as a bug. ## Testing and validation -**Invert the existing negative-behavior probes first.** The master checkout carries an -untracked probe file, `crates/markdown_parser/src/html_tag_support_tests.rs` (wired via -`#[path] mod html_tag_support_tests;` at `markdown_parser.rs:1998-1999`), whose two anchor -tests — `test_raw_html_anchor_href_not_parsed_as_hyperlink` and -`test_raw_html_anchor_id_not_registered_as_target` — currently assert the **no-op** status -quo (raw `` produces *no* hyperlink style; raw `` registers *no* target). Once -item 1 lands, both assertions become false and CI goes red. The implementation must **invert -them into positive assertions** (`` *does* produce a `Hyperlink::Url`; `` *does* -register an anchor) as part of the same change, not leave them asserting the old behavior. +**Validation is committed, tracked test cases — not the probe file.** All cases below live in +tracked test modules (`markdown_parser_tests.rs`, editor-model tests) that CI runs today and +will run against the implementation PR; nothing in this spec's validation depends on a file +that isn't checked in. + +**Historical context on the probe file.** The master checkout currently carries an *untracked* +scratch file, `crates/markdown_parser/src/html_tag_support_tests.rs` (wired via `#[path] mod +html_tag_support_tests;` at `markdown_parser.rs:1998-1999`, itself marked "DO NOT COMMIT" in +its header comment), whose two anchor probes — +`test_raw_html_anchor_href_not_parsed_as_hyperlink` and +`test_raw_html_anchor_id_not_registered_as_target` — assert today's **no-op** status quo (raw +`` produces no hyperlink style; raw `` registers no target). These were exploratory +probes used to confirm current behavior while scoping this spec, not a test suite the +implementation is meant to build on. The implementation PR must **not** leave these assertions +in place or rely on the untracked file continuing to exist — it must add the committed positive +assertions listed below to `markdown_parser_tests.rs` directly (`` *does* produce a +`Hyperlink::Url`; `` *does* register an anchor), which subsumes and inverts what the probe +file was checking. If the probe file is still present at implementation time, delete it or fold +any remaining useful cases into the committed suite — it should not ship as project state. ### Parser unit tests (`crates/markdown_parser/src/markdown_parser_tests.rs`) @@ -396,9 +465,16 @@ register an anchor) as part of the same change, not leave them asserting the old effect on output (invariant 8). - Unterminated `` / missing closing `` → literal text fallback, rest of paragraph intact (invariant 10). -- Slug normalizer (item 2): plain text, text with punctuation/mixed case, multi-space runs - → normalized slug (invariant 4). Test the pure `&str → String` function directly; first-wins - collision behavior is covered by the resolution tests below, not the normalizer. +- Slug normalizer (item 2), tested as a pure `&str → String` function, table-driven over: + - Plain ASCII text, text with punctuation/mixed case, multi-space runs → normalized slug + (invariant 4). + - **Unicode headings, to lock in genuine GitHub-compatibility (not ASCII-only):** + accented Latin (`"Café Société"` → `café-société`), CJK (`"日本語"` → `日本語`, unchanged — + no word characters to strip and no ASCII case to fold), and a mixed-script heading + (e.g. `"Section 日本語 Café"` → `section-日本語-café`). These guard against silently + reintroducing an ASCII-only filter. + - First-wins collision behavior is covered by the resolution tests below, not the + normalizer itself. - (Phase 2) `` / `` → zero-width anchor marker, no visible text emitted (invariant 5). - (Phase 2) `text` (both id and content on one tag) — confirm documented @@ -411,6 +487,9 @@ These target the matcher directly, since that is where the phase-1 change lives. - Heading `## Target Section` + fragment `#target-section` → `find_matching_header` returns the heading's range; today (exact-text match) it returns `None`. This is the regression the slug normalizer fixes (invariants 2, 3, 4). +- Heading with a non-English title (e.g. `## Café Société`) + fragment `#café-société` → + resolves, exercising the Unicode-preserving normalizer end-to-end through the matcher, not + just the pure function in isolation. - `` and markdown `[text](#slug)` targeting the same heading resolve to the same range (invariant 3 — the two syntaxes are equivalent because both reach `find_matching_header` via the same `#`-branch in `maybe_open_url`). @@ -418,8 +497,14 @@ These target the matcher directly, since that is where the phase-1 change lives. `scroll_to_matching_header` returns `false`, no panic (invariant 7). - First-wins collision: two headings normalizing to the same slug → the fragment resolves to the first, exercised by asserting the returned range is the earlier heading's. -- (Phase 2) Explicit `` colocated with a heading whose implicit slug is also `x` - → explicit anchor wins per invariant 6, or documented fallback if not achievable. +- (Phase 2) Explicit `` sharing a fragment/document with a heading whose implicit + slug is also `x` → the combined resolver's explicit-anchor-first pass (item 5) returns the + anchor's position, not the heading's, per the defined precedence rule (invariant 6). +- (Phase 2) Explicit anchor lookup when no heading collides: `` alone resolves + via the fragment-styles pass added to the combined resolver (item 5), independent of the + header-outline pass. +- (Phase 2) Two `` markers with the same id → first occurrence wins, mirroring the + heading first-wins rule. - (Phase 3) Fragment-split in `resolve`: `other-file.md#section` yields cleaned path `other-file.md` + anchor `section`; `other-file.md` (no fragment) yields no anchor; `other-file.md#L10` still routes to the existing line-number path, not the anchor path