spec: <details>/<summary> in markdown rendering (#10259)#10462
spec: <details>/<summary> in markdown rendering (#10259)#10462lonexreb wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec proposes support for HTML5 <details> / <summary> in Warp markdown rendering with sanitizer, accessibility, interaction, and test requirements. The core behavior is scoped, but several requirements need tightening before implementation.
Concerns
- The behavior contract does not define missing or misplaced
<summary>handling, even though untrusted markdown commonly contains malformed HTML. - The implementation pointers reference markdown files that do not match this checkout's parser/rendering layout, which makes the spec difficult to implement as written.
Security
- The arbitrary-depth nesting requirement should explicitly preserve existing parser/resource limits or require an iterative/guarded implementation so malicious deeply nested input cannot cause stack overflow or rendering denial of service.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| - `<details>` — block-level container. May carry `open` attribute. No other | ||
| attributes preserved. | ||
| - `<summary>` — exactly one is the visible always-rendered header. Additional |
There was a problem hiding this comment.
<details> has no <summary> or the first <summary> is not the first child; otherwise malformed README/agent HTML can render inconsistently across parser and native/custom widget paths.
|
|
||
| ### B5. Nested `<details>` | ||
|
|
||
| - Supported to arbitrary depth. |
There was a problem hiding this comment.
arbitrary depth leaves untrusted markdown nesting unbounded; specify that implementation must respect existing parser/resource limits or use guarded iterative rendering to avoid stack overflow or rendering DoS.
|
|
||
| ## Implementation Pointers | ||
|
|
||
| - Parser config: `app/src/markdown/parser.rs` — extend the HTML allowlist |
There was a problem hiding this comment.
app/src/markdown/{parser,renderer,sanitize}.rs paths do not match this checkout's markdown pipeline (crates/markdown_parser/src/html_parser.rs, parser tests, and app render surfaces); update the pointers to the actual files implementers must change.
|
Round-1 fixes for oz-for-oss review: (1) added Malformed |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Reviewed the spec-only PR for <details> / <summary> markdown rendering support. The spec covers the behavior, sanitizer scope, accessibility, malformed input, and test matrix, but two requirements conflict or weaken the intended resource protections.
Concerns
- B5 promises arbitrary-depth nesting while B8.1 and T12 cap rendered disclosure widgets at 32 levels, creating conflicting implementation and test expectations.
- The high-cardinality DoS guard is labeled a soft cap even though the following lines require deterministic fallback after the 1000th
<details>. - The custom ARIA path should state that
aria-controlsIDs are renderer-generated after sanitization, not derived from input attributes.
Security
- The resource-limit language should be deterministic because this feature renders untrusted recursive markup.
Verdict
Found: 1 critical, 1 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| ### B5. Nested `<details>` | ||
|
|
||
| - Supported to arbitrary depth. |
There was a problem hiding this comment.
🚨 [CRITICAL] This conflicts with B8.1/T12, which cap rendered <details> nesting at 32 and render deeper levels as plain text. Replace the arbitrary-depth promise with the capped behavior so implementation and tests agree.
| - Supported to arbitrary depth. | |
| - Supported up to the B8.1 resource limit (32 rendered levels); deeper `<details>` fall back to plain text. |
| literal `<details>` and `<summary>` opening / closing tags become visible | ||
| text in the output, and any inner markdown body still renders normally | ||
| through the existing markdown pipeline (no content is silently dropped). | ||
| - **Maximum `<details>` count per rendered surface: 1000** (soft cap). |
There was a problem hiding this comment.
| - **Maximum `<details>` count per rendered surface: 1000** (soft cap). | |
| - **Maximum `<details>` count per rendered surface: 1000** (hard cap). |
| widget: | ||
| - `role="group"` on the disclosure container. | ||
| - `aria-expanded` on the summary toggle. | ||
| - `aria-controls` linking the summary to its body region. |
There was a problem hiding this comment.
💡 [SUGGESTION] [SECURITY] Specify that aria-controls targets use renderer-generated IDs after sanitization, never input-supplied id attributes; B8 drops input IDs, but the custom ARIA path still needs a safe linkage source.
|
Round-2 fixes: (1) CRITICAL resolved — removed 'arbitrary depth' wording; nesting cap is a hard 32-level limit with plain-text fallback for over-cap levels; B5/B8.1/T12 now match; (2) 1000-block cap is now hard cap (deterministic), not soft — A_hard_cap_1000 tests the 1001st renders as plain text exactly at the threshold; (3) |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec adds markdown <details> / <summary> support with explicit sanitizer, accessibility, malformed-input, and deterministic resource-limit requirements.
Concerns
- The spec includes conversation list previews in scope but does not define behavior for non-interactive or row-click preview surfaces.
- The custom-widget accessibility contract does not require an activatable control role/focus target for the summary toggle.
- The test matrix misses the multiple-
<summary>sibling normalization promised by the behavior contract.
Verdict
Found: 0 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| Warp's markdown renderer should support standard HTML5 `<details>` and | ||
| `<summary>` tags so collapsible disclosure widgets work in any rendered | ||
| markdown surface (agent output, README previews, conversation list previews). |
There was a problem hiding this comment.
| layer supports them, OR an equivalent ARIA pattern when using a custom | ||
| widget: | ||
| - `role="group"` on the disclosure container. | ||
| - `aria-expanded` on the summary toggle. |
There was a problem hiding this comment.
role=button; aria-expanded alone may not be announced or operated as a disclosure control.
| expansion would re-introduce nested `<details>` after depth-32 is | ||
| bounded by the same depth counter; the run terminates and the over-cap | ||
| level renders as plain text. | ||
| - T15. Malformed input matrix (B7.1): one assertion per case — |
There was a problem hiding this comment.
💡 [SUGGESTION] B1/B7.1 require additional <summary> siblings to be concatenated into the first summary, but this malformed-input matrix omits that case; add a test so implementations do not render extra summaries inconsistently.
|
Addressed all 3 round-N concerns: 1. Conversation list preview / non-interactive surface behavior. Added B9.1 'Surface-mode rendering (interactive vs non-interactive)' as the authoritative contract. Two modes: 2. Custom-widget activatable role / focus target. Reworked B10 with explicit MUST requirements for the custom-widget path: 3. Tests for multi- Commit: 0662da3 |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines <details> / <summary> support for Warp markdown rendering, including sanitizer allowlists, accessibility behavior, malformed-input handling, non-interactive preview mode, and deterministic resource limits.
Concerns
- The top-level goals still promise collapsible widgets across every markdown surface, which conflicts with the later non-interactive preview-mode contract.
- The implementation pointers say no per-call renderer changes are expected, but B9.1 requires explicit mode selection and should identify the preview call sites that must opt into
NonInteractivePreview.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| - Render `<details>` / `<summary>` as collapsible disclosure widgets across | ||
| every markdown surface the renderer is used in. |
There was a problem hiding this comment.
| `warp-details-2`, …). When the native `<details>` / `<summary>` semantics | ||
| path is taken, the renderer SKIPS identifier generation: | ||
| `crates/warpui_core/src/elements/formatted_text_element.rs`. | ||
| - Renderer call sites (no per-call changes expected; listed so reviewers can |
There was a problem hiding this comment.
NonInteractivePreview. List those call sites and require the new option wiring before implementation.
1 participant
Spec-only PR. References #10259.
Summary
Adds support for standard HTML5
<details>and<summary>tags in Warp's markdown renderer, so collapsible disclosure widgets work in agent output, READMEs, and other rendered markdown surfaces. Today these tags are stripped or rendered as plain text.Why
<details>/<summary>is part of GFM's de facto surface and standard HTML5. It is heavily used in long agent outputs and open-source READMEs to support progressive disclosure. Aligning Warp's renderer with this convention removes friction from a common rendering case.Spec
See
specs/GH10259/SPEC.mdfor the full behavior contract: collapsed-by-default unlessopen, click + keyboard toggle, nested-to-arbitrary-depth, inline-only<summary>content, full markdown in the body, sanitizer attribute allowlist (details:[open],summary:[]), accessibility (native or ARIArole=group+aria-expanded+aria-controls), and tests covering each.Notes
Labels
ready-to-spec