Fix hidden caret by avoiding bogus-char selection#4783
Conversation
📝 WalkthroughWalkthroughAdds handling for a ZERO WIDTH NO-BREAK SPACE (bogus) span used as a cursor placeholder: creates/stores the bogus span, removes the placeholder and adjusts the caret on input/selection changes, treats the bogus span as the styling target for collapsed ranges, and attaches/detaches a selectionchange listener with cleanup on destroy. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Editor
participant BrowserSelection as SelectionAPI
participant DOM
rect rgb(235,245,255)
Note over Editor,SelectionAPI: Initialization
Editor->>SelectionAPI: attach selectionchange listener
end
rect rgb(245,255,235)
Note over User,DOM: Typing / Input
User->>DOM: input (may include ZWNBSP)
DOM-->>Editor: input event
Editor->>DOM: detect bogus span containing ZWNBSP
alt caret inside bogus span
Editor->>SelectionAPI: collapse range to position inside bogus
Editor->>DOM: remove ZWNBSP char and clear bogus data
else
Editor->>DOM: remove ZWNBSP if present (no caret adjust)
end
end
rect rgb(255,250,230)
Note over User,Editor: Styling action (e.g., fontSize)
User->>Editor: request style change
Editor->>SelectionAPI: get current range
alt range collapsed inside bogus span
Editor->>DOM: treat bogus span as styling target (collapse-then-select)
else
Editor->>DOM: compute affected spans and apply styles
end
end
rect rgb(250,235,255)
Note over SelectionAPI,Editor: Cursor moves
SelectionAPI->>Editor: selectionchange event
Editor->>DOM: if caret moved outside bogus -> remove bogus span and cleanup
end
rect rgb(235,245,255)
Note over Editor,SelectionAPI: Destroy
Editor->>SelectionAPI: detach selectionchange listener
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/base/module/Editor.spec.js (1)
11-11: Verify if thedomimport is necessary.The
dommodule is imported but doesn't appear to be directly used in the test code. If it's not required, consider removing it to keep imports clean.#!/bin/bash # Verify if dom is used elsewhere in the test file rg -n '\bdom\.' test/base/module/Editor.spec.jssrc/js/module/Editor.js (1)
392-434: Review the early return logic and secondary bogus element cleanup mechanism.The modified input handler has a design consideration regarding early returns:
Early return on lines 394-396: When content is limited, the function returns early without executing the
removeData(KEY_BOGUS)call at line 54. TheapplySnapshotrestores the DOM to a previous state, but this does not guarantee that the data reference is cleared. This is mitigated by the secondaryselectionchangehandler (lines 62-70) which removes bogus spans when the cursor leaves them and also callsremoveData(KEY_BOGUS). However, there's a potential edge case ifselectionchangedoesn't fire due to unexpected input flow.Single-node assumption (line 52): The
breakstatement assumes only one text node contains the ZWNBSP character. For the bogus span's intended purpose (temporary styling), this is a reasonable assumption, but could theoretically miss edge cases if DOM content gets split into multiple text nodes through manipulation.Cursor adjustment (lines 43-49): The logic correctly adjusts cursor position when ZWNBSP is removed.
Consider whether explicitly calling
removeData(KEY_BOGUS)before the early return on line 395 would improve robustness, or documenting the reliance on theselectionchangehandler as the backup cleanup mechanism.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/module/Editor.jstest/base/module/Editor.spec.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/js/module/Editor.js (1)
src/js/core/dom.js (3)
node(551-551)node(583-583)node(621-621)
test/base/module/Editor.spec.js (3)
src/js/core/dom.js (1)
textNode(904-904)src/styles/lite/summernote-lite.js (1)
editor(10-10)src/styles/bs4/summernote-bs4.js (1)
editor(7-7)
🔇 Additional comments (4)
test/base/module/Editor.spec.js (1)
460-483: LGTM! Well-structured test for bogus element cleanup.The test effectively verifies the core behavior: when selection moves away from a styled empty span containing only the bogus character, the cleanup logic correctly removes the span and clears the stored reference. The test structure follows established patterns in the suite and appropriately uses
nextTick()for async operations.src/js/module/Editor.js (3)
436-452: LGTM! Clean selectionchange handler with proper scope.The selectionchange handler is well-designed:
- Uses a document-level listener (appropriate for selection events)
- The
cursorIsInsidehelper cleanly encapsulates the containment check- Properly cleans up the bogus span when the cursor moves away
- Only removes spans that contain just the placeholder character
The logic correctly addresses the cursor visibility issue described in the PR objectives.
503-505: LGTM! Proper event listener cleanup.The destroy method correctly removes the selectionchange listener, preventing memory leaks. The defensive check for
this._onSelectionChangeensures cleanup is safe even if the listener wasn't attached.
929-943: The collapse().select() change on line 941 is correct and consistent with codebase patterns.The code change correctly implements cursor positioning for the bogus span. The pattern
collapse().select()is already used elsewhere in the codebase (line 652 in thesaveRangemethod), confirming this is an established and appropriate approach within the project. The collapse() call positions the cursor without selecting the ZERO_WIDTH_NBSP character, then select() applies that position—exactly as intended to keep the caret visible.
joelamos
force-pushed
the
cursor-fix-and-bogus-cleanup
branch
from
3d80b8d to
1468ad4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/module/Editor.js (1)
928-930: Useconstinstead ofvarfor consistency.The rest of the file uses
const/let(ES6 style). Usingvarhere is inconsistent.🔎 Proposed fix
- var bogusSpan = this.$editable.data(KEY_BOGUS); - var spans = rng.isCollapsed() && bogusSpan?.contains(rng.sc) ? + const bogusSpan = this.$editable.data(KEY_BOGUS); + const spans = rng.isCollapsed() && bogusSpan?.contains(rng.sc) ? [bogusSpan] : this.style.styleNodes(rng);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/module/Editor.jstest/base/module/Editor.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/base/module/Editor.spec.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/module/Editor.js (1)
src/js/core/dom.js (3)
node(551-551)node(583-583)node(621-621)
🔇 Additional comments (4)
src/js/module/Editor.js (4)
396-433: LGTM! Input handler correctly strips ZWNBSP once real content arrives.The logic properly handles:
- Early return when no bogus element exists
- Early return when content is still only the placeholder or already clean
- Cursor position adjustment when removing the ZWNBSP character from a mixed-content node
436-451: LGTM! Selection change handler properly cleans up orphaned bogus spans.The handler correctly:
- Only removes the bogus span when the cursor leaves it
- Preserves spans that contain user-typed content (removes only placeholder-only spans)
- Clears the bogus reference to prevent stale tracking
500-505: LGTM! Proper cleanup of the document-level listener.The conditional guard prevents errors if
destroy()is called beforeinitialize(), and correctly removes the listener to avoid memory leaks.
936-943: LGTM! Collapse prevents selecting the invisible ZWNBSP character.Using
collapse()beforeselect()positions the caret at the start of the styled span without selecting the invisible placeholder character, which was causing the hidden caret issue. This is the correct fix for the reported bug.
Stop selecting the ZERO_WIDTH_NBSP/FEFF helper character when applying font-size with a collapsed range, since selecting that invisible node can make the caret appear hidden. Also clean up the tracked KEY_BOGUS span on selection changes: once the cursor leaves the helper span, remove it (when it only contains the bogus char) and clear the KEY_BOGUS reference.
joelamos
force-pushed
the
cursor-fix-and-bogus-cleanup
branch
from
1468ad4 to
1189a8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/module/Editor.js (1)
928-930: Preferconstovervarfor better scoping.The logic correctly prefers the bogus span as the styling target when the range is collapsed inside it. However, consider using
constinstead ofvarfor modern JavaScript best practices.🔎 Proposed refactor
- var bogusSpan = this.$editable.data(KEY_BOGUS); - var spans = rng.isCollapsed() && bogusSpan?.contains(rng.sc) ? - [bogusSpan] : this.style.styleNodes(rng); + const bogusSpan = this.$editable.data(KEY_BOGUS); + const spans = rng.isCollapsed() && bogusSpan?.contains(rng.sc) ? + [bogusSpan] : this.style.styleNodes(rng);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/module/Editor.jstest/base/module/Editor.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/base/module/Editor.spec.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/module/Editor.js (1)
src/js/core/dom.js (3)
node(551-551)node(583-583)node(621-621)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/js/module/Editor.js (4)
396-434: LGTM! Solid implementation of ZWNBSP cleanup.The logic correctly strips the bogus character after real text arrives, with proper cursor adjustment when the caret is positioned within the affected text node. The early returns, TreeWalker usage, and offset calculation are all appropriate.
436-451: LGTM! Clean implementation of bogus span cleanup.The
cursorIsInsidehelper correctly uses the Selection API to detect cursor position, and the selectionchange listener appropriately removes the bogus span only when the cursor exits and the span contains solely the placeholder character.
502-504: LGTM! Proper cleanup on destroy.Correctly removes the document-level selectionchange listener to prevent memory leaks.
940-940: LGTM! This is the core fix for the hidden caret issue.Changing from
select()tocollapse().select()correctly positions the cursor without selecting the ZWNBSP character, which would otherwise make the caret appear hidden. This change directly addresses the PR objectives.
1 participant
What does this PR do / why do we need it?
Stop selecting the ZERO_WIDTH_NBSP/FEFF helper character when applying font-size with a collapsed range, since selecting that invisible node can make the caret appear hidden.
Also clean up the tracked KEY_BOGUS span on selection changes: once the cursor leaves the helper span, remove it (when it only contains the bogus char) and clear the KEY_BOGUS reference. Otherwise, these bogus spans can become orphaned if you change the font-size and update the selection without content changes.
Which issue(s) this PR fixes?
Fixes #4101, #4490, #4697
Checklist
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.