fix(editor): invalid caret in note-edgeless-block on focus #14229
Conversation
📝 WalkthroughWalkthroughDetects clicks on a note block title and routes title clicks to the native range handler; non-title clicks either create a paragraph when the model is empty or clamp coordinates to the container and delegate to the native range handler. The caret range is normalized to the end of the last meaningful text node. Changes
Sequence DiagramsequenceDiagram
participant User
participant NoteBlock as Note Block
participant TitleEl as Title Element
participant RangeHandler as Range Handler
participant TextNodes as Text Nodes
User->>NoteBlock: Click at (x,y)
NoteBlock->>TitleEl: query `edgeless-page-block-title` & getBoundingClientRect()
TitleEl-->>NoteBlock: rect / no rect
alt Click inside title rect
NoteBlock->>RangeHandler: handleNativeRangeAtPoint(original coords)
else Click outside title rect
alt Model has no children
NoteBlock->>NoteBlock: create `affine:paragraph`
NoteBlock->>NoteBlock: focus new paragraph (if created)
else Model has children
NoteBlock->>NoteBlock: compute container rect & clamp coords (apply zoom offsets)
NoteBlock->>RangeHandler: handleNativeRangeAtPoint(clamped coords or original)
end
end
RangeHandler->>TextNodes: obtain caret range
RangeHandler->>TextNodes: lastMeaningfulTextNode(...)
TextNodes-->>RangeHandler: node / none
RangeHandler->>RangeHandler: normalizeCaretRange() -> collapse to end of meaningful node
RangeHandler-->>NoteBlock: return normalized range
NoteBlock->>User: place cursor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
congzhou09
changed the title
![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
🤖 Fix all issues with AI agents
In @blocksuite/affine/shared/src/utils/dom/point-to-range.ts:
- Around line 120-148: normalizeCaretRange currently reads
startContainer.childNodes[startOffset - 1] without guarding startOffset==0;
change normalizeCaretRange so it computes a safe offset index (e.g., if
startOffset > 0 use startOffset - 1, otherwise use 0 or handle separately) and
use that to get offsetEl, or simply check startOffset > 0 before accessing
childNodes[startOffset - 1]; then proceed to the existing checks against
offsetEl, lastMeaningfulTextNode and range.setStart as before so the element at
index 0 is not skipped when startOffset is 0.
🧹 Nitpick comments (1)
blocksuite/affine/blocks/note/src/note-edgeless-block.ts (1)
493-494: Consider extracting magic numbers as named constants.The offset values
8and2(scaled by zoom) appear to be padding or margin adjustments, but their purpose isn't immediately clear from the code.♻️ Proposed refactor for clarity
Define constants at the class or file level:
+const CLICK_CLAMP_OFFSET_Y = 8; +const CLICK_CLAMP_OFFSET_X = 2; + handleSelection: ({ std, gfx, view, model }) => { return { onSelect(context) { // ... existing code ... if (rect) { - const offsetY = 8 * gfx.viewport.zoom; - const offsetX = 2 * gfx.viewport.zoom; + const offsetY = CLICK_CLAMP_OFFSET_Y * gfx.viewport.zoom; + const offsetX = CLICK_CLAMP_OFFSET_X * gfx.viewport.zoom; const x = clamp( e.clientX, rect.left + offsetX, rect.right - offsetX );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
blocksuite/affine/blocks/note/src/note-edgeless-block.tsblocksuite/affine/shared/src/utils/dom/point-to-range.ts
🔇 Additional comments (4)
blocksuite/affine/shared/src/utils/dom/point-to-range.ts (2)
93-96: LGTM!The conditional normalization of the caret range is a clean addition that ensures range validity before processing.
104-118: LGTM!The helper correctly uses
TreeWalkerto locate the last meaningful text node, filtering out empty or whitespace-only nodes.blocksuite/affine/blocks/note/src/note-edgeless-block.ts (2)
457-472: LGTM!The title click detection correctly uses the bounding rectangle and safely handles cases where the title element might not exist.
474-510: LGTM! Well-structured conditional logic.The branching correctly handles three scenarios:
- Title clicks now receive proper caret placement without creating new blocks
- Empty notes still auto-create a paragraph block
- Notes with content clamp coordinates within the content area before placing the caret
This implementation effectively addresses both issues described in the PR objectives.
congzhou09
force-pushed
the
fix/caret-in-note-edgeless-block
branch
2 times, most recently
from
3d668bc to
fc472dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocksuite/affine/shared/src/utils/dom/point-to-range.ts (1)
120-151: Critical issue from past review has been addressed.The guard for
startOffset > 0at lines 125-128 correctly handles the edge case wherestartOffsetis 0, preventing negative array access. The normalization logic appropriately finds the last meaningful text node and positions the caret at its end, which should resolve the vanishing caret issue.Optional: Consider adding a clarifying comment.
The logic
startOffset - 1might benefit from a brief inline comment explaining that offset N points between child N-1 and N (i.e., "before child N"), so we look at child N-1.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
blocksuite/affine/blocks/note/src/note-edgeless-block.tsblocksuite/affine/shared/src/utils/dom/point-to-range.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/blocks/note/src/note-edgeless-block.ts
🔇 Additional comments (2)
blocksuite/affine/shared/src/utils/dom/point-to-range.ts (2)
93-95: LGTM! Clean integration of range normalization.The conditional normalization is correctly guarded and integrates well with the existing flow.
104-118: Function logic is sound.The TreeWalker approach efficiently finds the last non-whitespace text node. The filter correctly excludes whitespace-only text nodes, which aligns with the goal of preventing vanishing carets at line ends.
congzhou09
force-pushed
the
fix/caret-in-note-edgeless-block
branch
from
fc472dc to
fd27da8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocksuite/affine/blocks/note/src/note-edgeless-block.ts (1)
477-510: LGTM! Logic correctly handles different scenarios.The code properly branches based on whether the model has children:
- Empty model (lines 477-486): Creates a paragraph and focuses it only if creation succeeds, preventing focus errors.
- Has children with container rect (lines 488-505): Clamps click coordinates to the container bounds (with zoom-adjusted offsets) before calling the range handler, ensuring clicks near edges don't produce unexpected caret positions.
- Has children without rect (lines 506-508): Falls back to original coordinates, providing graceful degradation.
The zoom-based offset scaling maintains consistent behavior across different viewport zoom levels.
♻️ Optional: Extract magic numbers to named constants
The offset values
8and2at lines 493-494 could be extracted to named constants for clarity:+const CLAMPING_OFFSET_Y_BASE = 8; +const CLAMPING_OFFSET_X_BASE = 2; + ... -const offsetY = 8 * gfx.viewport.zoom; -const offsetX = 2 * gfx.viewport.zoom; +const offsetY = CLAMPING_OFFSET_Y_BASE * gfx.viewport.zoom; +const offsetX = CLAMPING_OFFSET_X_BASE * gfx.viewport.zoom;This would make the intent clearer and simplify future adjustments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
blocksuite/affine/blocks/note/src/note-edgeless-block.tsblocksuite/affine/shared/src/utils/dom/point-to-range.ts
🧰 Additional context used
🧬 Code graph analysis (1)
blocksuite/affine/blocks/note/src/note-edgeless-block.ts (3)
blocksuite/affine/gfx/group/src/renderer/utils.ts (1)
titleBound(61-65)tests/blocksuite/e2e/utils/asserts.ts (1)
Bound(106-106)blocksuite/affine/shared/src/utils/dom/point-to-range.ts (1)
handleNativeRangeAtPoint(91-102)
🔇 Additional comments (5)
blocksuite/affine/shared/src/utils/dom/point-to-range.ts (3)
93-96: LGTM!The integration of
normalizeCaretRangeafter obtaining the caret range ensures consistent cursor positioning across browsers. The null check before normalization is appropriate.
104-118: LGTM!The
lastMeaningfulTextNodehelper correctly traverses text nodes and filters out whitespace-only content. The whitespace filtering is appropriate for caret placement, as it ensures the caret lands in content that provides visual feedback to the user.
120-154: LGTM! Past critical issue has been resolved.The
normalizeCaretRangefunction correctly handles the edge case flagged in the previous review. The guard at lines 125-128 prevents negative array indexing whenstartOffsetis 0, ensuring:
- When
startOffset > 0: accesseschildNodes[startOffset - 1]and places caret at its end- When
startOffset === 0: accesseschildNodes[0]and places caret at its startThe fallback chain (offsetEl as text → last text in offsetEl → last text in startContainer) provides robust handling for various DOM structures.
blocksuite/affine/blocks/note/src/note-edgeless-block.ts (2)
457-472: LGTM!The title click detection correctly:
- Queries the title element and retrieves its bounding rectangle
- Constructs a
Boundobject from the DOMRect coordinates- Checks point containment using
isPointInBound- Safely handles the case where
titleRectis nullThis ensures clicks on the title region are properly intercepted before falling through to other handlers.
474-476: LGTM!Title clicks are routed directly to
handleNativeRangeAtPointwith the original click coordinates, allowing the browser's native caret positioning (now enhanced with the normalization logic frompoint-to-range.ts) to handle title interactions.
1 participant
Problem
●In edgeless mode, when starting to edit,
note-blockexhibits two types of invalid caret behavior:(1)Title Region Misalignment: Clicking on the title region incorrectly generates the caret in the first line of the note content, rather than in the title itself.
(2)Vanishing Caret at Line End: When clicking in the empty space beyond the end of a text section, the caret appears momentarily at the line's end but disappears immediately.
●The following video demonstrates these issues:
before.mp4
Solution
●Title Click Interception: Added a check to determine if the click coordinates fall in the title region. If so, the caret positioning is now handled by a dedicated logic path. Otherwise, it falls back to the existing note-content logic as before.
●Range Normalization: When the generated
range.startContaineris not aTextNode, try to find a most appropriateTextNodeand update therangeaccordingly.After
●The video below shows the behavior after this fix.
after-compressed.mp4
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.