Fixed selection attributes handling after softBreak.#19918
Fixed selection attributes handling after softBreak.#19918
Conversation
| for ( let node: ModelNode | null = startNode; node; node = getNextNode( node ) ) { | ||
| if ( node.is( 'element', 'softBreak' ) ) { | ||
| crossedSoftBreak = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I'm not sure whether the engine should be aware of the existence of the softBreak element. It's registered in an external plugin. Would a schema flag be better? What do you think?
There was a problem hiding this comment.
We discussed with @niegowski, and it might be good idea to store some info in schema / attribute properties. However, there might be another workaround related to storing attributes on soft-breaks itself. We need to discuss it a little bit more.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Engine core hardcodes plugin-specific element name
- Replaced the hardcoded
softBreakelement check with schema-based inline non-object handling that copies only$textattributes markedcopyOnEnter, removing plugin-name coupling in engine selection logic.
- Replaced the hardcoded
Or push these changes by commenting:
@cursor push 09cbb82ebf
Preview (09cbb82ebf)
diff --git a/packages/ckeditor5-engine/src/model/documentselection.ts b/packages/ckeditor5-engine/src/model/documentselection.ts
--- a/packages/ckeditor5-engine/src/model/documentselection.ts
+++ b/packages/ckeditor5-engine/src/model/documentselection.ts
@@ -1278,11 +1278,20 @@
return node.getAttributes();
}
- if ( node.is( 'element', 'softBreak' ) ) {
+ if ( !schema.isInline( node ) ) {
+ return null;
+ }
+
+ if ( !schema.isObject( node ) ) {
const attributes: Array<[ string, unknown ]> = [];
+ // Inline non-object elements create a "soft" attribute boundary and can optionally carry over
+ // text attributes marked as copyOnEnter.
for ( const [ key, value ] of node.getAttributes() ) {
- if ( schema.checkAttribute( '$text', key ) ) {
+ if (
+ schema.checkAttribute( '$text', key ) &&
+ schema.getAttributeProperties( key ).copyOnEnter
+ ) {
attributes.push( [ key, value ] );
}
}
@@ -1290,14 +1299,6 @@
return attributes;
}
- if ( !schema.isInline( node ) ) {
- return null;
- }
-
- if ( !schema.isObject( node ) ) {
- return [];
- }
-
const attributes: Array<[string, unknown]> = [];
// Collect all attributes that can be applied to the text node.This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| if ( !schema.isInline( node ) ) { | ||
| continue; |
There was a problem hiding this comment.
Non-inline elements skipped instead of halting backward/forward scan
Low Severity
When getTextAttributes encounters a non-inline element (a block element like <paragraph>) during a backward or forward scan, it uses continue to skip it. While the old code's getTextAttributes also returned null for non-inline elements (causing the old while-loop to similarly continue past them), the old loop additionally checked whether the starting node itself was truthy (while ( node && !attrs )), meaning it could only traverse real siblings. The new siblingNodes generator also handles null correctly, so in practice this produces the same result. However, the JSDoc comment at line 1237 now says "it checks if item is an inline object" but the function actually processes all inline elements (both object and non-object), not just inline objects. This could mislead future contributors about the function's scope.
| writer.removeAttribute( key, child ); | ||
| wasChanged = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterating attributes while removing them from Map
Low Severity
child.getAttributes() returns a live Map.entries() iterator, and writer.removeAttribute(key, child) deletes from that same Map during iteration. While deleting the current entry is generally safe in ES6 Map iteration, the writer.removeAttribute path goes through model.applyOperation, which fires events and could theoretically trigger listeners that add or remove other attributes on the same child, potentially skipping or double-visiting entries. Collecting keys to remove first (like into an array) and then removing them after the loop would be more robust.
3 participants
🚀 Summary
Improved document selection attribute inheritance around
<softBreak>so returning the caret after a soft break restores expected formatting.When selection attributes are recalculated across
<softBreak>, only attributes marked withcopyOnEnterare inherited. Other inline non-object elements still act as hard boundaries.📌 Related issues
💡 Additional information
Optional: Notes on decisions, edge cases, or anything helpful for reviewers.
🧾 Checklists
Use the following checklists to ensure important areas were not overlooked.
This does not apply to feature-branch merges.
If an item is not relevant to this type of change, simply leave it unchecked.
Author checklist
Reviewer checklist
t()(if any).