Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dropping image over the page break crashes the editor #8788

Closed
niegowski opened this issue Jan 9, 2021 · 1 comment · Fixed by #8858
Closed

Dropping image over the page break crashes the editor #8788

niegowski opened this issue Jan 9, 2021 · 1 comment · Fixed by #8858
Assignees
Labels
package:clipboard squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@niegowski
Copy link
Contributor

📝 Provide detailed reproduction steps (if any)

  1. https://ckeditor.com/docs/ckeditor5/latest/features/page-break.html
  2. Drag and drop some image over the page break widget

✔️ Expected result

An image should be properly dropped to the editor.

❌ Actual result

Uncaught CKEditorError: model-nodelist-offset-out-of-bounds {"offset":9,"nodeList":[]}
Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-model-nodelist-offset-out-of-bounds
    at offsetToIndex (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:141138)
    at c.offsetToIndex (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:44806)
    at a (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:15894)
    at r.get textNode [as textNode] (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:10039)
    at $e (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:426548)
    at qe._validateSelectionRange (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:426384)
    at m.<anonymous> (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:94898)
    at m.fire (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:28590)
    at m._setRanges (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:143980)
    at m.setTo (https://ckeditor.com/docs/ckeditor5/latest/snippets/features/page-break/snippet.js:4:143626)

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@niegowski niegowski added type:bug This issue reports a buggy (incorrect) behavior. package:clipboard squad:core Issue to be handled by the Core team. labels Jan 9, 2021
@niegowski
Copy link
Contributor Author

niegowski commented Jan 9, 2021

This is because the page break feature is converted to container and text elements instead of UI Element

conversion.for( 'editingDowncast' ).elementToElement( {
model: 'pageBreak',
view: ( modelElement, { writer } ) => {
const label = t( 'Page break' );
const viewWrapper = writer.createContainerElement( 'div' );
const viewLabelElement = writer.createContainerElement( 'span' );
const innerText = writer.createText( t( 'Page break' ) );
writer.addClass( 'page-break', viewWrapper );
writer.setCustomProperty( 'pageBreak', true, viewWrapper );
writer.addClass( 'page-break__label', viewLabelElement );
writer.insert( writer.createPositionAt( viewWrapper, 0 ), viewLabelElement );
writer.insert( writer.createPositionAt( viewLabelElement, 0 ), innerText );
return toPageBreakWidget( viewWrapper, writer, label );
}
} );

And there is no text positions mapping between view and model.

This could be easily fixed like this:

conversion.for( 'editingDowncast' ).elementToElement( {
	model: 'pageBreak',
	view: ( modelElement, { writer } ) => {
		const label = t( 'Page break' );
		const viewWrapper = writer.createContainerElement( 'div' );
		const viewLabelElement = writer.createUIElement( 'span', { class: 'page-break__label' }, function( domDocument ) {
			const domElement = this.toDomElement( domDocument );

			domElement.innerText = t( 'Page break' );

			return domElement;
		} );

		writer.addClass( 'page-break', viewWrapper );
		writer.setCustomProperty( 'pageBreak', true, viewWrapper );

		writer.insert( writer.createPositionAt( viewWrapper, 0 ), viewLabelElement );

		return toPageBreakWidget( viewWrapper, writer, label );
	}

@oleq oleq added this to the nice-to-have milestone Jan 11, 2021
@maxbarnas maxbarnas self-assigned this Jan 18, 2021
@maxbarnas maxbarnas modified the milestones: nice-to-have, iteration 39 Jan 18, 2021
niegowski added a commit that referenced this issue Jan 19, 2021
Fix (page-break): Dropping an image on the page break widget should not crash the editor. Closes #8788.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:clipboard squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants