[COMMS-572] Pasting rich text into CKEditor crashes it#124
Draft
op-chomper wants to merge 1 commit into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI-generated PR! Please review it for accuracy and then remove this line.
What does this PR change?
Fixes a crash where pasting formatted rich text into CKEditor (e.g. an AppSignal error page with a syntax-highlighted code block) threw
CKEditorError: can't access property "replace", e6.data is undefined, which the watchdog answered by restarting the editor — wiping everything the user had typed.The crash came from the code-block upcast converter assuming
<code>'s first child was a text node (child.data.replace(...)). When the pasted code is wrapped in highlight<span>s, that child is an element and.dataisundefined. It now extracts text via a recursivetextContentOf()helper that only reads.dataoff real text nodes.It also adds a defensive paste boundary so this class of failure can never crash the editor again.
What approach did you choose and why?
Two layers: (1) the targeted converter fix, which resolves the reported repro; and (2) a new
OPPasteGuardplugin that runs the clipboard→model conversion in atry/catchbefore the pipeline's default insertion. On any converter error it aborts the rich paste, inserts the plain-text fallback, and emitsop:clipboard-paste-error— so the editor stays alive and existing content is preserved even for a future, unknown converter bug. The companion OpenProject frontend change turns that event (and watchdog errors) into a user-facing flash instead of failing silently.Screenshots
No visual changes (error-handling/flash messaging only).
Tests
Added unit tests for
textContentOfcovering plain text, single-text<code>, the regression case (element-wrapped<code>, asserts no throw), deep nesting, and empty elements. Manual verification: paste the AppSignal incident content into a meeting agenda editor — content pastes, no console error, previously typed text retained; Ctrl+Shift+V still works.Merge checklist
npm run build) and bump@openproject/commonmark-ckeditor-buildin the OpenProject frontend — the fix isn't live until the generatedbuild/+src/ckeditor.jsare regenerated.js.editor.error_paste_failedlocale key) merged together.