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

[Conversion] Write down a summary of changes #11268

Closed
Reinmar opened this issue Feb 14, 2022 · 4 comments
Closed

[Conversion] Write down a summary of changes #11268

Reinmar opened this issue Feb 14, 2022 · 4 comments
Assignees
Labels
package:engine release:blocker This issue blocks the upcoming release (is critical). squad:core Issue to be handled by the Core team. type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 14, 2022

Why

#10294 came with a large number of changes that are hard to discover without going really deep into the issue tracker.

Currently, the only place where we could send people is #11211.

What

Let's write a blog-post-like (developer-oriented) summary of what has changed including:

  • The "whys" behind each of the change
  • The "whats"

The idea is to make it digestible so we should:

  • Limit ourselves to what matters to people
  • Compile multiple smaller thoughts into topics (avoid explaining every single small change independently)
@Reinmar Reinmar added type:task This issue reports a chore (non-production change) and other types of "todos". package:engine type:docs This issue reports a task related to documentation (e.g. an idea for a guide). squad:core Issue to be handled by the Core team. labels Feb 14, 2022
@Reinmar
Copy link
Member Author

Reinmar commented Feb 28, 2022

Decision: Option 2.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 28, 2022

Some ideas:

Some of the important new features:

  • Support for triggering an e2E converter based on attributes of the element – replaces multiple smaller converters (e2E + a2A or the case with <hx> based on <heading level=x>) and triggerBy.
  • The entire e2S. Previously required playing with doubled-binding between single model element and multiple view).

@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Feb 28, 2022
@Reinmar Reinmar added the release:blocker This issue blocks the upcoming release (is critical). label Mar 1, 2022
@dawidurbanski dawidurbanski self-assigned this Mar 1, 2022
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Mar 1, 2022
@dawidurbanski
Copy link
Contributor

@dawidurbanski
Copy link
Contributor

dawidurbanski commented Mar 8, 2022

Summary of changes

To see all changes in v33.0.0 check the migration guide

Downcast conversion helpers and API changes

New downcast conversion helper: elementToStructure()

We have found that in many cases you may want to create a single view element with one or many children elements (a structure) in one go.

For this purpose we introduced a new downcast conversion helper: elementToStructure().

Previously you would probably use the elementToElement() conversion helper to perform this task like such:

// Convert <horizontalLine> model element
// into <div> <hr /> </div> structure.
editor.conversion.for( 'dataDowncast' ).elementToElement( {
	model: 'horizontalLine',
	view: ( modelElement, { writer } ) => {
		const viewWrapperElement = writer.createContainerElement( 'div' );
		const viewHrElement = writer.createEmptyElement( 'hr' );

		writer.insert( writer.createPositionAt( viewWrapperElement, 0 ), viewHrElement );
				
		return viewWrapperElement;
	}
} );

Currently this is discouraged and whenever possible you should use elementToStructure() instead.

Additionally we added an option to pass an array of children elements to writer.createContainerElement() in order to further simplify the process:

// Convert <horizontalLine> model element
// into <div> <hr /> </div> structure.
editor.conversion.for( 'dataDowncast' ).elementToStructure( {
	model: 'horizontalLine',
	view: ( modelElement, { writer } ) => {
		return writer.createContainerElement( 'div', null, [
			writer.createEmptyElement( 'hr' )
		] );
	}
} );

New, seamless reconversion mechanism

The reconversion API has been in beta for a while now. With CKEditor 33.0.0, it finally reached the final state. There are some notable changes to it.

Removal of the triggerBy option

Starting from CKEditor 23.1.0 you were able to replace a set of atomic converters (elementToElement() + attributeToAttribute()) with one converter handling any element + attributes combo using triggerBy option (see more).

// Convert <myElement type="single" ownerId="1"> model element
// into <div class="my-element my-element-single" data-owner-id="1"></div>.
editor.conversion.for( 'downcast' ).elementToElement( {
	model: 'myElement',
	view: ( modelElement, { writer } ) => {
		return writer.createContainerElement( 'div', {
			 'data-owner-id': modelElement.getAttribute( 'ownerId' ),
			class: `my-element my-element-${ modelElement.getAttribute( 'type' ) }`
		} );
	},
	triggerBy: {
		attributes: [ 'ownerId', 'type' ]
	}
} );

In CKEditor 33.3.0 we changed this API and from now on you should use attributes property on a model instead.

// Convert <myElement type="single" ownerId="1"> model element
// into <div class="my-element my-element-single" data-owner-id="1"></div>.
editor.conversion.for( 'downcast' ).elementToElement( {
	model: {
		name: 'myElement',
		attributes: [ 'ownerId', 'type' ]
	},
	view: ( modelElement, { writer } ) => {
		return writer.createContainerElement( 'div', {
			'data-owner-id': modelElement.getAttribute( 'ownerId' ),
			class: `my-element my-element-${ modelElement.getAttribute( 'type' ) }`
		} );
	}
} );

In the above, the <myElement> model element will be reconverted every time either ownerId or type is updated.

Additionally you can react to changes in the element children using children property.

// Convert <myElement><paragraph>A</paragraph></myElement> model element
// into <div class="my-element" data-type="single">...</div>.
// But if more paragraph are added like so:
// <myElement><paragraph>A</paragraph><paragraph>B</paragraph></myElement>
// then convert into <div class="my-element" data-type="multiple">...</div>.
editor.conversion.for( 'downcast' ).elementToElement( {
	model: {
		name: 'myElement',
		children: true
	},
	view: ( modelElement, conversionApi ) => {
		const { writer } = conversionApi;

		return writer.createContainerElement( 'div', {
			class: 'my-element',
			'data-type': modelElement.childCount == 1 ? 'single' : 'multiple'
		} );
	}
} );

Both attributes and children can be used together and are available in both elementToElement() and elementToStructure() conversion helpers.

The brand new slots API

Until now the “slots” concept was only briefly discussed (see: #1589). With the addition of the elementToStructure() conversion helper it became apparent, that we need a proper slots API in place.

Slots will allow you to place some view elements in one container element while putting some other element in another one.

For example:

editor.conversion.for( 'dataDowncast' ).elementToStructure( {
	model: {
		name: 'items',
		attributes: [ 'highlightedItems' ]
	},
	view: ( modelElement, { writer } ) => {
		const highlightedItems = modelElement.getAttribute( 'highlightedItems' ) || 0;

		return writer.createContainerElement( 'div', { class: 'items' }, [
			writer.createContainerElement( 'div', { class: 'highlighted' }, [
				writer.createSlot( element => element.index < highlightedItems )
			] ),
			writer.createContainerElement( 'div', null, [
				writer.createSlot( element => element.index >= highlightedItems )
			] )
		] );
	}
} );

Will result in such conversion:

Model:

<items highlightedItems="2">
	<paragraph>One</paragraph>
	<paragraph>Two</paragraph>
	<paragraph>Three</paragraph>
</items>

To View:

<div class="items">
	<div class="highlighted">
		<p>One</p>
		<p>Two</p>
	</div>
	<div>
		<p>Three</p>
	</div>
</div>

Known issues

There is a problem when you try to put $text nodes directly inside slots. It has been raised and is tracked in #11149.

The workaround for now is to use elementToElement() conversion helper to create multiple elements instead of elementToStructure().

DowncastDispatcher API changes

Changes to the flow of events in the downcast pipeline

This change was driven by two main factors:

  • Converters using the new slots API will trigger events in a recursive way while in any other cases DowncastDispatcher will fire events in a flat structure, one by one,
  • The opposite UpcastDispatcher triggers events in the recursive way for all converters so we aim to unify both of them with this change.

You can read details about it in #10376

Set of new methods used to control events flow

With this change converters can ask DowncastDispatcher to start converting children or attributes using the conversionApi's convertItem()convertChildren() and convertAttributes() methods.

Thanks to this converters can control the flow of events.

Implications of this change

In most cases this should be transparent to developers but it may have some impact if you were cancelling events in this flow.

Long story short, do not use evt.stop() inside conversion event listeners unless you know what you are doing.

editor.conversion.for( 'dataDowncast' ).add( dispatcher => {
	return dispatcher.on( `attribute:xyz`, ( evt, data, conversionApi ) => {
		...

		evt.stop(); // ❌  DO NOT CANCEL EVENTS!
	} );
} );

Change convertInsert() to convert()

The aim was to make this API much cleaner and the convert() method can now convert markers as well, you don’t have to trigger it separately. It’s mostly used internally in our code so we won’t go into details about this one.

New error conversion-model-consumable-not-consumed

We’ve been seeing a numerous pesky issues caused by the fact that some elements in the converter were not consumed. This was very hard to debug and was causing other, unrelated converters to accidentally consume these previously unconsumed elements.

I has been discussed multiple times in the past, mainly in #3818 and we finally introduced conversion-model-consumable-not-consumed error in #10377 which you can check for more info.

What to avoid (notice no consumable.consume in the converter):

editor.conversion.for( 'dataDowncast' ).add( dispatcher => {
	return dispatcher.on( `attribute:xyz`, ( evt, data, conversionApi ) => {
		const { item, attributeNewValue } = data;
		const { mapper, writer } = conversionApi;

		if ( !conversionApi.consumable.test( item, evt.name ) ) {
			return;
		}
		
		const viewElement = mapper.toViewElement( item );

		writer.setStyle( 'xyz', attributeNewValue, viewElement );
	} );
} );

✅  What to to instead:

editor.conversion.for( 'dataDowncast' ).add( dispatcher => {
	return dispatcher.on( `attribute:xyz`, ( evt, data, conversionApi ) => {
		const { item, attributeNewValue } = data;
		const { mapper, writer } = conversionApi;
		
		// ✅ Consume item immediately when you check if consumable.
		if ( !conversionApi.consumable.consume( item, evt.name ) ) {
			return;
		}
		
		const viewElement = mapper.toViewElement( item );

		writer.setStyle( 'xyz', attributeNewValue, viewElement );
	} );
} );

--- OR ---:

editor.conversion.for( 'dataDowncast' ).add( dispatcher => {
	return dispatcher.on( `attribute:xyz`, ( evt, data, conversionApi ) => {
		const { item, attributeNewValue } = data;
		const { mapper, writer } = conversionApi;
		
		if ( !conversionApi.consumable.test( item, evt.name ) ) {
			return;
		}
		
		const viewElement = mapper.toViewElement( item );

		writer.setStyle( 'xyz', attributeNewValue, viewElement );

		// ✅ Consume item afterwards.
		conversionApi.consumable.consume( item, evt.name );
	} );
} );

EditingController API changes

New methods reconvertItem() and reconvertMarker()

For many reasons (very nicely explained in #10659) we introduced these two new methods.

We need them mainly if something changed outside of the model or in a way that you need to manually trigger the reconversion.

It’s usually used in very complex cases so there’s very little change you may be affected by this change.

@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Mar 8, 2022
@CKEditorBot CKEditorBot added this to the upcoming milestone Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine release:blocker This issue blocks the upcoming release (is critical). squad:core Issue to be handled by the Core team. type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

3 participants