Conversation
packages/ckeditor5-core/src/editor/utils/normalizerootsconfig.ts
Outdated
Show resolved
Hide resolved
I know, I have it marked locally |
|
bugbot run |
Bugbot couldn't runBugbot is not enabled for your user on this team. Ask your team administrator to increase your team's hard limit for Bugbot seats or add you to the allowlist in the Cursor dashboard. |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Falsy empty string skips conflicting initialData validation
- Updated root conflict checks to treat empty strings as provided source data by checking for null/undefined explicitly before validating element type.
- ✅ Fixed: Test config places
baroutsiderootsobject- Moved the
barroot test configuration underrootsso the test correctly validates both root entries.
- Moved the
Or push these changes by commenting:
@cursor push 8d233d8ee3
Preview (8d233d8ee3)
diff --git a/packages/ckeditor5-core/src/editor/utils/normalizerootsconfig.ts b/packages/ckeditor5-core/src/editor/utils/normalizerootsconfig.ts
--- a/packages/ckeditor5-core/src/editor/utils/normalizerootsconfig.ts
+++ b/packages/ckeditor5-core/src/editor/utils/normalizerootsconfig.ts
@@ -69,6 +69,7 @@
for ( const rootName of rootNames ) {
const rootConfig = rootsConfig[ rootName ] || Object.create( null );
const sourceElementOrDataForRoot = sourceElementIsPlainObject ? sourceElementsOrData[ rootName ] : sourceElementsOrData;
+ const sourceElementOrDataForRootIsDefined = sourceElementOrDataForRoot !== undefined && sourceElementOrDataForRoot !== null;
// No dedicated initial data for the root.
if ( rootConfig.initialData === undefined ) {
@@ -78,7 +79,7 @@
rootConfig.initialData = getInitialData( sourceElementOrDataForRoot );
}
// If both `config.initialData` is set and initial data is passed as the constructor parameter, then throw.
- else if ( sourceElementOrDataForRoot && !isElement( sourceElementOrDataForRoot ) ) {
+ else if ( sourceElementOrDataForRootIsDefined && !isElement( sourceElementOrDataForRoot ) ) {
// Documented in core/editor/editorconfig.ts.
// eslint-disable-next-line ckeditor5-rules/ckeditor-error-message
throw new CKEditorError( 'editor-create-initial-data', null );
@@ -89,7 +90,7 @@
}
}
// If both `rootConfig.initialData` is set and initial data is passed as the constructor parameter, then throw.
- else if ( sourceElementOrDataForRoot && !isElement( sourceElementOrDataForRoot ) ) {
+ else if ( sourceElementOrDataForRootIsDefined && !isElement( sourceElementOrDataForRoot ) ) {
// Documented in core/editor/editorconfig.ts.
// eslint-disable-next-line ckeditor5-rules/ckeditor-error-message
throw new CKEditorError( 'editor-create-initial-data', null );
diff --git a/packages/ckeditor5-core/tests/editor/utils/normalizerootsconfig.js b/packages/ckeditor5-core/tests/editor/utils/normalizerootsconfig.js
--- a/packages/ckeditor5-core/tests/editor/utils/normalizerootsconfig.js
+++ b/packages/ckeditor5-core/tests/editor/utils/normalizerootsconfig.js
@@ -42,8 +42,9 @@
it( 'should use initialData from config.root if set', () => {
config.set( 'root', { initialData: '<p>bar</p>' } );
+ const sourceElement = document.createElement( 'div' );
- normalizeRootsConfig( '', config );
+ normalizeRootsConfig( sourceElement, config );
const roots = config.get( 'roots' );
diff --git a/packages/ckeditor5-editor-multi-root/tests/multirooteditor.js b/packages/ckeditor5-editor-multi-root/tests/multirooteditor.js
--- a/packages/ckeditor5-editor-multi-root/tests/multirooteditor.js
+++ b/packages/ckeditor5-editor-multi-root/tests/multirooteditor.js
@@ -143,7 +143,7 @@
expect( () => {
// eslint-disable-next-line no-new
new MultiRootEditor( { foo: '<p>Foo</p>', bar: '<p>Bar</p>' },
- { roots: { foo: { initialData: '<p>Bar</p>' } }, bar: { initialData: '<p>Abc</p>' } }
+ { roots: { foo: { initialData: '<p>Bar</p>' }, bar: { initialData: '<p>Abc</p>' } } }
);
} ).to.throw( CKEditorError, 'editor-create-initial-data' );
} );This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| // No legacy initial data for the root, either. | ||
| if ( legacyInitialData[ rootName ] === undefined ) { | ||
| // Use source element data or data itself as a string. | ||
| rootConfig.initialData = getInitialData( sourceElementOrDataForRoot ); |
There was a problem hiding this comment.
Undefined passed to getInitialData for missing source roots
Medium Severity
When sourceElementIsPlainObject is true and a root name exists in config.roots or legacyInitialData but not in the source elements object, sourceElementsOrData[rootName] evaluates to undefined. This undefined is passed to getInitialData, which has a return type of string but will silently return undefined at runtime (since isElement(undefined) is false, it returns sourceElementOrData which is undefined). This sets rootConfig.initialData = undefined, violating the expected string type for initialData.
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| */ | ||
| private _initUsingData = true; | ||
| // TODO clean it | ||
| // private _initUsingData = true; |
There was a problem hiding this comment.
Accidentally committed WiP TODO comments and debug code
Medium Severity
Multiple WiP artifacts remain: /** * TODO */ placeholder doc comments on _editorAttachTo and _isSingleRootEditor, a commented-out // private _initUsingData = true; with // TODO clean it, // TODO make it nicer, // TODO is this correct for lazy root?, // TODO keep the documentation, // TODO update comment, placeholder?: string; // TODO, // TODO add tests, and TODO breaking change in doc text. These indicate unfinished work accidentally included.
Additional Locations (2)
| editableElements: sourceIsData ? undefined : sourceElementsOrData as Record<string, HTMLElement>, | ||
| label: this.config.get( 'label' ) | ||
| editableElements: this.sourceElements, | ||
| label: extractRootsConfigField( this.config.get( 'roots' )!, 'label' ) |
There was a problem hiding this comment.
Multi-root editor passes empty object instead of undefined for editableElements
Medium Severity
When the multi-root editor is initialized with string data (no DOM elements), this.sourceElements is {}. The old code passed undefined for editableElements in this case, but the new code always passes this.sourceElements. Since {} is truthy, MultiRootEditorUIView may treat it differently from undefined, potentially trying to bind to nonexistent DOM elements rather than creating new ones.



🚀 Summary
A brief summary of what this PR changes.
📌 Related issues
root/rootseditor config option #19885💡 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).Note
Medium Risk
Medium risk because it changes how all editor types derive
initialData,placeholder, andlabel, and it rewires multi-root lazy loading/attributes and watchdog restart logic; mis-normalization could affect editor initialization and restoration flows.Overview
Adds aggregated per-root configuration via new
EditorConfig.root/EditorConfig.rootsandRootConfig, while deprecating legacyconfig.initialData,config.placeholder, andconfig.labelin favor of per-root equivalents.Introduces
normalizeRootsConfig()in core and updates all editor constructors/UIs (classic/inline/balloon/decoupled/multi-root, plus tests) to normalize config and readinitialData/placeholder/labelfromconfig.roots(with legacy values mapped into roots and new validation errors for conflicting sources).Updates multi-root configuration to move
lazyRootsandrootsAttributesintoroots.<name>.lazyLoadandroots.<name>.modelAttributes, throwing on deprecated options, and adjusts multi-root initialization anddata.init()to consume initial data extracted from root configs. Watchdog restart logic is updated to preserve/normalize root configs (including legacy placeholders) and no longer depends on multi-root as a runtime dependency.Written by Cursor Bugbot for commit f82c56f. This will update automatically on new commits. Configure here.