Repository navigation
Preserve content when pasting multiple HTML tables - #128
cyberianin wants to merge 2 commits into
Conversation
|
Hi maintainers, a gentle follow-up on this PR. The "Build" workflow is still showing "action_required" and appears to be waiting for approval to run for a first-time external contributor. Could someone approve the workflow run when convenient? The patch passed local verification ("npm test": 60 passed; "npm run lint" and "npm run build": passed). I'm happy to address any review feedback. Thanks! |
There was a problem hiding this comment.
🟡 Changes recommended
Paste validation and fallback still mishandle indented tables, skipped layout tables, CRLF text, and literal numbered paragraphs, causing incorrect rendering or paste behavior.
4 open issues
What changed in this PR
Updates table handling in the paste-markdown pipeline to preserve content during paste and drop when clipboard HTML contains multiple tables. It uses verified Markdown when possible and otherwise preserves the original HTML source.
Changes:
- Adds conservative plaintext validation and HTML fallbacks for complex or ambiguous table fragments.
- Makes later handlers respect
defaultPreventedto avoid duplicate insertion. - Expands regression coverage for content preservation, malformed tables, and event handling.
| File | Description |
|---|---|
test/test.js |
Adds paste/drop regressions and makes synthetic paste events cancelable. |
src/paste-markdown-text.ts |
Skips paste events already handled elsewhere. |
src/paste-markdown-table.ts |
Adds complete-plaintext validation, source-preserving fallbacks, and event guards. |
src/paste-markdown-link.ts |
Avoids processing already-handled paste events. |
src/paste-markdown-image-link.ts |
Respects handled paste and drop events. |
src/paste-markdown-html.ts |
Avoids processing already-handled paste events. |
Review effort: Max
|
Additional context on the scope of this PR: The initial implementation passed its local regression tests, lint, and build checks. Subsequent A/B testing against GitHub's production editor provided additional coverage of interactions between paste handlers. These tests confirmed that the multi-table content loss is addressed by this PR. They also reproduced a separate, pre-existing duplicate insertion issue in the unmodified implementation, involving multiple handlers processing the same paste operation. To keep the fix focused and independently reviewable, the follow-up commit removes changes to unrelated handlers while retaining the verified multi-table correction and incorporating the review feedback. The duplicate insertion behavior will be investigated separately, with dedicated reproduction cases and tests for handler interactions. It is not considered resolved by this PR. |

Problem
When clipboard HTML contains two tables,
generateText()takes the source before the first<table, converts only the first parsed table, and resumes after the last</table>. This removes the intervening content and the second table. The same function handles paste and drop.Fix
For multiple, nested, or ambiguously bounded tables, accept
text/plainonly when a conservative check establishes that simple paragraphs and independent tables match the Markdown blocks in order. Otherwise, preserve the original HTML string in the editor. Fragments containing skipped tables are left to the normal paste/drop chain. The existing single-table conversion path remains.Minimal reproduction
Clipboard
text/html:Clipboard
text/plain:Previously the table handler removed
Betweenand the second table. With this change, it preserves the complete verified Markdown; if the plain representation is absent, incomplete, or ambiguous, it preserves the original HTML instead.Review and production testing
Following review and additional testing against GitHub's production editor, this PR has been narrowed to the multi-table content preservation fix. Earlier changes to unrelated paste/drop handlers have been reverted from the final diff. The follow-up commit also addresses the four review findings concerning skipped tables, indented Markdown, CRLF line endings, and ordered-list markers.
Production A/B testing showed that the original table handler loses intermediate content and a subsequent table in a complex paste, while the revised implementation preserves both tables and their structure. The testing also exposed a separate, pre-existing duplicate insertion behavior involving competing paste handlers. That issue is deliberately left out of this PR and will be investigated in a follow-up change. Unrelated Markdown Preview rendering limitations are likewise outside this PR's scope.
Verification and limitations
npm test: 44 passed, 0 failed.npm run lint,npm run build, andgit diff --check: passed.