Support raw HTML <br> line breaks in the Markdown renderer#13854
Support raw HTML <br> line breaks in the Markdown renderer#13854fbartho wants to merge 9 commits into
Conversation
…v#13732) Warp's Markdown viewer rendered a raw <br> as literal text in both paragraphs and GFM pipe-table cells. Recognize <br>, <br/>, and <br /> (case-insensitive) as an inline token that produces a forced line break. The break is stored as a newline fragment: both the paragraph and table-cell render paths already turn an embedded newline into a real visual break, so no rendering changes are needed. The newline/tab/pipe delimited serialization paths (inline_to_markdown, to_plain_text) translate the break back to <br> so an in-cell break does not corrupt table row/column structure. Attributes (e.g. <br class="x">) and malformed forms (<br with no >, < br>, </br>) deterministically fall through to literal text.
A raw HTML <br> at the end of a GFM pipe-table cell parses to a trailing "\n" fragment, but layout_run strips a trailing newline out of the cell text so the break was silently dropped — the cell collapsed to a single line instead of rendering the trailing empty line GitHub shows. measure_table_cells now honors layout_run's has_new_line return: when the final run ends in a stripped newline, re-append it to the cell text so both the measure pass and the render pass emit the trailing empty line. Embedded breaks (interior newline) were never stripped and are unchanged.
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this 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 PR teaches the Markdown parser to treat raw <br> variants as hard line breaks in paragraphs and GFM table cells, then preserves trailing table-cell breaks during layout.
Concerns
- Exporting a table cell containing a parsed
<br>can still emit a raw newline in the editor GFM table export path, which corrupts the pipe-table row on copy/export. I left an inline comment on the parser-side serialization change because the corresponding editor table serializer needs the same newline-to-<br>handling. - Spec context: no approved or repository spec context was provided for this PR.
Verdict
Found: 0 critical, 1 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
The editor's table export path (BufferMarkdownParser::serialize_table_to_gfm_markdown) uses a local inline_to_markdown that emitted fragment text unchanged, so an in-cell hard break (raw HTML <br>, stored as an embedded newline) was written as a raw newline that split the pipe-table row. Apply the same newline-to-<br> conversion the markdown_parser serializer already performs, guarded on non-inline-code fragments.
|
Re: Spec Context: the issue was tagged Otherwise I've implemented the requested changes (as discussed in the inline comment thread), so I think I'm ready for another /oz-review |
|
I'm re-reviewing this 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 |
|
I'm re-reviewing this 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 PR teaches the Markdown inline parser to treat raw HTML <br> variants as hard line breaks, adds table serialization support so those breaks round-trip as <br>, and adjusts table-cell measurement for trailing breaks.
Concerns
- Table cells still drop a
<br>when the break is represented as a trailing newline on a non-final styled run, such as a break at the boundary between bold/link text and following plain text. - No approved spec context was provided, so there is no spec-drift finding.
- No security findings.
Verdict
Found: 0 critical, 1 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
There was a problem hiding this comment.
Overview
This PR teaches the Markdown parser to treat raw <br> variants as hard line breaks and updates table serialization/layout tests around preserving those breaks.
Concerns
- Table-cell rendering drops
<br>line breaks when the break is at a style boundary because only the last stripped trailing newline is preserved.
Verdict
Found: 0 critical, 1 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
TableCellOffsetMap::from_inline_and_source walked source char-by-char against rendered text, assuming a 1:1 correspondence. A raw HTML <br> is 4+ source chars but a single rendered newline, so the walk desynced: offsets after an in-cell break mapped past the break instead of to their true source positions, corrupting cursor rects, selection highlights, and clipboard slices for post-break text. Walk each fragment in newline-delimited segments, emitting one linear range per segment and consuming the <br>/<br/>/<br /> token as a source gap between segments.
serialize_table_cell_inline wrote a cell fragment's text verbatim, so an authored hard break (stored as an embedded newline) landed as a raw newline inside <td>. HTML collapses that to a space, silently dropping the in-cell break when copying a table as HTML. Split the fragment on newlines and emit a <br> element between the parts, mirroring the sibling guard in inline_to_markdown (ca6a4dd). Inline code is left verbatim since it can never contain such a newline.
clipboard_table_text_in_range flattened each cell with inline_to_text, which emits an authored hard break (stored as an embedded newline) as a raw newline. Inside the tab/newline-delimited clipboard format that raw newline splits the row, so pasting a copied table back into Warp re-parsed a spurious extra row. Translate the break back to a literal <br> when building each cell slice, scoped to the table-cell context rather than changing inline_to_text globally (other callers may legitimately want the newline).
The parser had no coverage combining <br> with inline style delimiters. Guard that a break inside bold, underline, or a link keeps the surrounding style on both sides of the embedded newline.
The plain-text clipboard flavor is rendered text and already strips styling, so an authored hard break must emit a real newline rather than the literal HTML string "<br>" the user never selected. The HTML flavor still carries the break as a real <br> element for Warp-to-Warp round-trip fidelity; the accepted cost is that a plain-text-only re-paste of such a row may split it, a standard TSV-class limitation.
|
Before re-review I ran a deeper pass over the surfaces this PR touches and hardened three more spots, each with a failing-test-first fix:
Also manually verified in a local build that styled breaks paint no stray underline/bold artifacts on macOS. /oz-review |
|
hey @kevinyang372 I accidentally wasted one of my Oz reviews earlier by double triggering, but I think the PR is finally complete. Could you please trigger oz again for me? or otherwise review this? Thanks! |
|
Closing in favor of #13870. |
1 participant
Closes #13732
The Markdown viewer rendered a raw
<br>as literal text, both mid-paragraph and inside GFM pipe-table cells. Inside a pipe-table cell<br>is the only way to author a forced line break at all, so this was the more painful half of the gap.This teaches the
.mdinline parser to recognize<br>,<br/>, and<br />(case-insensitive) and turn them into a real line break, in ordinary paragraphs and inside GFM table cells.Notes:
<br>so an in-cell break does not corrupt row/column structure on round-trip.<br class="x">) and malformed forms (<brwithout>,< br>,</br>) fall through to literal text.Tested: new unit tests in
markdown_parserpass, clippy and rustfmt clean, and exercised in a local dev build against the issue sample document — paragraphs, consecutive breaks, self-closing/case variants, table cells, and malformed forms all behave as described.Human-focused Test Document + Screenshot