fix: orphan ZWS after linebreak deletes whole line on Backspace#71
Open
Karol-Stelmaczonek wants to merge 3 commits into
Open
fix: orphan ZWS after linebreak deletes whole line on Backspace#71Karol-Stelmaczonek wants to merge 3 commits into
Karol-Stelmaczonek wants to merge 3 commits into
Conversation
WalkthroughThe plugin updates ruby cleanup, caret handling, command-state scheduling, and editor event coordination. A new ChangesFurigana and qtiCreator workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant TaofuriganaPlugin
participant RubyDom
participant Toolbar
Editor->>TaofuriganaPlugin: Trigger keyboard or selection event
TaofuriganaPlugin->>RubyDom: Normalize ruby content and anchors
TaofuriganaPlugin->>Toolbar: Refresh command and button state
TaofuriganaPlugin->>Editor: Apply caret, selection, or change result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Karol-Stelmaczonek
requested review from
a team,
KirylHatalski,
bartlomiejmarszal,
pnal and
tikhanovichA
and removed request for
a team
July 21, 2026 10:43
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/taofurigana/plugin.js`:
- Around line 369-377: Update unwrapRubyToBaseText around rbElement and
replacement so unwrapping preserves inline markup contained in rb instead of
flattening it through rbElement.getText(). Reuse the existing HTML-preserving
createFromHtml approach from the blur cleanup path, while retaining the current
empty-rb handling and replacement/cleanup flow.
In `@README.md`:
- Around line 48-51: Update the qtiCreator testing workflow documentation to add
a configuration step before the build command, instructing users to set
CKEDITOR_DEV and TAO_CORE_VIEWS via exports or the appropriate configuration
edit. Keep the existing build, sync, refresh, and restore steps unchanged.
In `@scripts/link-to-tao.sh`:
- Line 124: Update the size calculation in cmd_status so a missing ckeditor.js
does not cause the command to exit under set -euo pipefail. Handle the failed
wc/input-redirection operation before assigning size, while preserving the
existing ${size:-?} fallback for absent files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c2c3877-f904-4c4a-a62e-072018ca4449
📒 Files selected for processing (4)
README.mdplugins/taofurigana/lang/en.jsplugins/taofurigana/plugin.jsscripts/link-to-tao.sh
Preserve inline markup when unwrapping ruby, document path setup before sync, and harden status when ckeditor.js is missing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/taofurigana/plugin.js (1)
371-391: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve every
rbchild when unwrapping.
createFromHtml()yields one node. For mixed base content such as漢<em>字</em>, replacing the ruby with that single node drops remaining siblings. Replace the ruby through a fragment/all parsed children and place the caret after the final inserted node.#!/bin/bash set -euo pipefail fd -a '^element\.js$' core | while read -r file; do rg -n -C8 'createFromHtml' "$file" done rg -n -C4 'createFromHtml\(rbInnerHtml|replacement\.replace\(rubyElement\)' \ plugins/taofurigana/plugin.js🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/taofurigana/plugin.js` around lines 371 - 391, Update the ruby-unwrapping logic around replacement and replacement.replace(rubyElement) to parse rbInnerHtml as a fragment or otherwise collect all parsed child nodes, preserving mixed text and inline markup such as 漢<em>字</em> instead of retaining only the first node. Insert every collected node in order, and position the caret after the final inserted node; retain the existing text fallback when parsing fails or produces no usable children.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/link-to-tao.sh`:
- Around line 123-127: Update the ckeditor.js handling around the ckeditor_js
and size variables to add an explicit missing-file branch: print an error when
the file does not exist and return a non-zero status. Preserve the existing size
calculation and successful flow when ckeditor.js is present, preventing the
later Active copy message from reporting an empty size.
---
Duplicate comments:
In `@plugins/taofurigana/plugin.js`:
- Around line 371-391: Update the ruby-unwrapping logic around replacement and
replacement.replace(rubyElement) to parse rbInnerHtml as a fragment or otherwise
collect all parsed child nodes, preserving mixed text and inline markup such as
漢<em>字</em> instead of retaining only the first node. Insert every collected
node in order, and position the caret after the final inserted node; retain the
existing text fallback when parsing fails or produces no usable children.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 218336ec-c703-48cb-a919-8a7972a949c7
📒 Files selected for processing (3)
README.mdplugins/taofurigana/plugin.jsscripts/link-to-tao.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
… missing Parse all rb child nodes when unwrapping ruby, and make link-to-tao status exit non-zero when ckeditor.js is absent.
Karol-Stelmaczonek
changed the title
KirylHatalski
approved these changes
Jul 21, 2026
bartlomiejmarszal
approved these changes
Jul 22, 2026
| var rtElement = startNode.getAscendant('rt', true); | ||
|
|
||
| // Do not re-seed ZWS into an empty rt — that causes Backspace to thrash | ||
| // textLen 0↔1 forever (see INF-530 probe). New rubies get anchors at create time. |
There was a problem hiding this comment.
Plese remove ticket reference from comment
| var node = getNextNodeCrossingBoundaries(fromNode); | ||
| var guard = 0; | ||
|
|
||
| while (node && guard < 50) { |
There was a problem hiding this comment.
Why 50? Should we use const for it?
Comment on lines
+673
to
+688
| if (isOrphanZwsAnchor(node)) { | ||
| cleanupZwsAnchor(node); | ||
| return true; | ||
| } | ||
|
|
||
| if (isEmptyTextNode(node)) { | ||
| node = getNextNodeCrossingBoundaries(node); | ||
| continue; | ||
| } | ||
|
|
||
| if (node.type === CKEDITOR.NODE_ELEMENT) { | ||
| var name = node.getName && node.getName(); | ||
| if (name === 'br') { | ||
| node = getNextNodeCrossingBoundaries(node); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Can we comment each condition so we know what they are executing and why?
| } | ||
|
|
||
| /** | ||
| * Chrome can delete a whole line when Backspace hits an orphan ZWS left after |
Comment on lines
+720
to
+727
| if (keyCode !== keyCodeBackspace || !selection || !selection.isCollapsed()) { | ||
| return false; | ||
| } | ||
|
|
||
| var range = selection.getRanges()[0]; | ||
| if (!range || !range.startContainer) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can we describe this conditions to understand why they are executed?
| /** | ||
| * Change command state according to the current selection content | ||
| * Change command state according to the current selection content. | ||
| * Debounced: uncanceled setTimeout(150) on every keyup was stacking and making |
There was a problem hiding this comment.
DO we really need historical code state in this comment?
| statelessButtons.forEach(function (button) { | ||
| button.setState(CKEDITOR.TRISTATE_OFF); | ||
| }); | ||
| }, 150); |
There was a problem hiding this comment.
Should we extract this value (150) into Const?
There was a problem hiding this comment.
Why do we need this script here?
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
3 participants
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.
Ticket : https://oat-sa.atlassian.net/browse/INF-530
Summary
rtunwrap, caret placement).scripts/link-to-tao.shfor syncing a built CKEditor into tao-core shared-libs (paths must be configured; placeholders fail loudly).Test plan
rt/ after ruby — caret and deletion behave as expectedrtunwraps cleanly without sluggish thrashing./scripts/link-to-tao.sh statuswithout configured paths prints setup help and exits non-zeroVideo
Screen.Recording.2026-07-21.at.11.45.28.mov
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Chores