Skip to content

[INF-529] Fix ruby tags keyboard navigation. - #72

Open
KirylHatalski wants to merge 7 commits into
developfrom
fix/INF-529/ruby-navigation-fix
Open

[INF-529] Fix ruby tags keyboard navigation.#72
KirylHatalski wants to merge 7 commits into
developfrom
fix/INF-529/ruby-navigation-fix

Conversation

@KirylHatalski

@KirylHatalski KirylHatalski commented Jul 22, 2026

Copy link
Copy Markdown

Depends from #71, need to be merged after.

Ticket: https://oat-sa.atlassian.net/browse/INF-529

chrome_qJK6sAM1sa

Extended key navigation control, changed navigation order.

Summary by CodeRabbit

  • Bug Fixes
    • Improved furigana/ruby editing: caret navigation, deletion behavior, and more reliable ruby-to-base-text unwrapping.
    • Better cleanup of invisible spacing markers after ruby changes.
    • More consistent toolbar/command state updates while editing furigana.
  • Documentation
    • Expanded CK Playground instructions for furigana/ruby development and step-by-step qtiCreator testing.
  • New Features
    • Added a utility script to sync, inspect, and restore CKEditor assets for local qtiCreator testing.

Karol-Stelmaczonek and others added 5 commits July 21, 2026 12:38
Preserve inline markup when unwrapping ruby, document path setup before sync, and harden status when ckeditor.js is missing.
… missing

Parse all rb child nodes when unwrapping ruby, and make link-to-tao status exit non-zero when ckeditor.js is absent.
@KirylHatalski
KirylHatalski requested review from a team, Karol-Stelmaczonek, bartlomiejmarszal, pnal and tikhanovichA and removed request for a team July 22, 2026 13:46
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b8db83c0-3d52-4fcd-9ce6-ffb827831bee

📥 Commits

Reviewing files that changed from the base of the PR and between ac0b4a1 and 4c6ce26.

📒 Files selected for processing (1)
  • plugins/taofurigana/plugin.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/taofurigana/plugin.js

Walkthrough

The PR refines CKEditor furigana caret, deletion, unwrapping, ZWS cleanup, and toolbar state behavior. It also adds a CKEditor-to-TAO synchronization script and documents furigana and qtiCreator testing workflows.

Changes

Furigana editing and testing

Layer / File(s) Summary
Ruby caret, deletion, and unwrapping
plugins/taofurigana/plugin.js, plugins/taofurigana/lang/en.js
Ruby navigation, empty-reading deletion, unwrapping, inline markup preservation, and orphan ZWS cleanup are revised. Language registration now uses the taofurigana plugin name.
Command state and event integration
plugins/taofurigana/plugin.js
Command-state refreshes are debounced, toolbar disabling is scheduled, and keyboard, selection, data, change, mouse, and blur handlers use normalized ruby state.
qtiCreator synchronization workflow
scripts/link-to-tao.sh, README.md
A strict Bash utility validates paths, syncs built CKEditor assets with backups, restores vendor assets, reports status, and is documented in the CK Playground workflow.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CKEditorSelection
  participant taofurigana
  participant RubyDOM
  CKEditorSelection->>taofurigana: caret or deletion event
  taofurigana->>RubyDOM: normalize ruby caret and inspect rt/rb
  taofurigana->>RubyDOM: unwrap ruby or clean orphan ZWS
  taofurigana->>CKEditorSelection: place caret after update
Loading

Possibly related PRs

  • oat-sa/ckeditor-dev#71: Overlaps with the ruby Backspace/Delete, empty rt unwrapping, and orphan ZWS cleanup changes.

Suggested reviewers: tikhanovicha, bartlomiejmarszal, pnal, karol-stelmaczonek

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing ruby/furigana keyboard navigation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/INF-529/ruby-navigation-fix

Comment @coderabbitai help to get the list of available commands.

@KirylHatalski
KirylHatalski marked this pull request as ready for review July 23, 2026 07:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
plugins/taofurigana/plugin.js (1)

664-671: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant change event fired for the empty-rt unwrap path.

guardEmptyRtUnwrap fires editor.fire('change') here, and it is invoked inside the contentChanged group of the keydown handler (directly at Line 1709 and indirectly via guardRtLeadingDelete at Line 703). That group re-fires change at Line 1724, so each empty-rt Backspace/Delete triggers change twice, running ensureZwsAnchorsAfterRuby/refreshHasRubyFlag redundantly. Drop the internal fire so the outer handler owns it, or drop the guard from the contentChanged group. Note that guardBackspaceAfterRuby (Line 1029) legitimately fires change internally because it is in the caretOnly group where the outer handler does not.

Proposed change
 			editor.fire('saveSnapshot');
 			editor.fire('lockSnapshot');
 			try {
 				unwrapRubyToBaseText(rubyElement, selection);
-				editor.fire('change');
 			} finally {
 				editor.fire('unlockSnapshot');
 			}
🤖 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 664 - 671, Remove the internal
editor.fire('change') call from the empty-rt unwrap flow in guardEmptyRtUnwrap,
while preserving the snapshot locking and unlock behavior. Keep the outer
contentChanged group in the keydown handler responsible for firing change; do
not alter guardBackspaceAfterRuby, which still needs its internal change event.
🤖 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 186-187: Update cmd_restore to validate only the TAO target and
backup requirements, removing its dependency on require_shared_path and
CKEDITOR_DEV. Split source-checkout/build validation into a separate helper or
path used exclusively by sync, and ensure sync still performs that validation
before copying.

---

Nitpick comments:
In `@plugins/taofurigana/plugin.js`:
- Around line 664-671: Remove the internal editor.fire('change') call from the
empty-rt unwrap flow in guardEmptyRtUnwrap, while preserving the snapshot
locking and unlock behavior. Keep the outer contentChanged group in the keydown
handler responsible for firing change; do not alter guardBackspaceAfterRuby,
which still needs its internal change event.
🪄 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: c0fcc6d5-3f58-46da-a853-356f27a40873

📥 Commits

Reviewing files that changed from the base of the PR and between bb7c58c and ac0b4a1.

📒 Files selected for processing (4)
  • README.md
  • plugins/taofurigana/lang/en.js
  • plugins/taofurigana/plugin.js
  • scripts/link-to-tao.sh

Comment thread scripts/link-to-tao.sh
When caret is on the next line after ruby, find the ruby across the break and move caret back after it instead of letting Chrome delete the whole previous line.
@Karol-Stelmaczonek

Copy link
Copy Markdown

Refreshing PR head — branch tip includes Enter+Backspace fix (2335426) but GitHub PR metadata was stuck on ac0b4a1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants