#10358 Introducing a downcast helper elementToStructure with reconversion support #10466

niegowski · 2021-09-01T17:27:11Z

Suggested merge commit message (convention)

Feature (engine): Added DowncastHelpers#elementToStructure() downcast helper with reconversion support. Closes #10358.

Feature (engine): It's now possible to trigger a nested conversion while downcasting an element.

Other (engine): The DowncastDispatcher#convert() method introduced as a replacement to previously used convertInsert(). The new method handles not only nodes conversion but also markers.

Internal (heading): Code adjusted to changes in DowncastDispatcher API.

Internal (html-embed): Code adjusted to the removal of triggerBy conversion option.

BREAKING CHANGE (engine): The DowncastDispatcher#conversionApi property is no longer available. The instances of DowncastConversionApi being created at the start of conversion.

BREAKING CHANGE (engine): The support for the triggerBy option for downcast helpers is removed and replaced with the new elementToStructure() options.

Additional information

This PR removes support for previous reconversion implementation and it's a part of an epic to reintroduce it in some downcast helpers.

…ranch.

…erting elements (text nodes are not supported).

Reinmar · 2021-09-08T13:50:45Z

Things to verify manually (cc @ckeditor/qa-team):

HTML embed feature
The getBody() function of the Title plugin combined with stuff that uses markers (TC, comments, etc)

Missing (cc @niegowski):

Tests for deferring removing mappings in the Mapper

Asking @scofalik to review this:

packages/ckeditor5-engine/src/conversion/modelconsumable.js
Overview of changes in DowncastHelpers

niegowski · 2021-09-08T15:09:16Z

Tests for deferring removing mappings in the Mapper

Added.

scofalik · 2021-09-08T16:24:56Z

packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

-		} );
-
-		const convertedViewElement = mapper.toViewElement( element );
+	convert( range, markers, writer, options = {} ) {


Do we need to pass markers here or can they be taken from range (range.root to be precise). I am not sure if RootElement has a reference to Model instance. Wouldn't it be nice if you didn't have to evaluate those markers on your own (like in data processor)?

The problem there is for example with Title plugin that for getBody() finds only intersecting parts of markers.

And in DataController we pass there only _getMarkersRelativeToElement.

Couldn't we evaluate it in the dispatcher? Relative to range in this case, I guess, or let passing range or element to the dispatcher?

Not sure how Title works.

scofalik · 2021-09-08T16:28:47Z

packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

@@ -335,21 +217,20 @@ export default class DowncastDispatcher {
 	convertSelection( selection, markers, writer ) {


Same here, would be nice if we could skip markers parameter ;).

Reinmar · 2021-09-09T20:12:16Z

TBD:

Regarding slotFor(), I'm still unsure about:
- slotFor( 'children' ) – can this accept something else than 'children'? I know it can be a callback too, but then it's still children, just not all. It's a bit odd to me.
- Do we check whether multiple slots with callbacks will not filter children in such a way that the order of them in the view will differ from the order in which they are in the model? This is a potentially dangerous case because mapping of a selection from the model to the view may and back may give weird results (like reversed selection positions).
- What will happen if a callback for slotFor() allocates text nodes in a weird manner (split to multiple places)? Maybe we should limit this and only allow for elements that has only elements as their children?
Consider splitting downcasthelpers.js into multiple files. It got too long for my taste ;| (and will get longer)

I'll ping you f2f about this. None of this is a blocker for merging the PR.

scofalik · 2021-09-10T10:30:23Z

packages/ckeditor5-engine/src/conversion/downcasthelpers.js

+			}
+		}
+
+		return [ node ];


Why null or single-item array instead of true or false?

I see this function is used as default parameter later in the code. Will there be cases when the user will write this function on their own, and then the function would return bigger arrays?

Or is this for future purposes? Or artifact from the past :)

This was for future purposes but in a recent spike, I found a better way to handle it. In the next PoC it's true/false ck/10294/reconversion...ck/10381-imperative-reconversion

scofalik · 2021-09-10T10:44:48Z

packages/ckeditor5-engine/src/conversion/downcasthelpers.js

+			return false;
+		}
+
+		return events.every( event => consumable.consume( node, event ) );


I think that forEach() + return true would be more clear.

scofalik · 2021-09-10T10:50:13Z

packages/ckeditor5-engine/src/conversion/downcasthelpers.js

+// @param {module:engine/model/element~Element} element
+// @param {Map.<module:engine/view/element~Element,Array.<module:engine/model/node~Node>>} slotsMap
+// @param {module:engine/conversion/downcastdispatcher~DowncastConversionApi} conversionApi
+// @returns {Function}


I am missing here at least a sentence about the parameter which the function can take.

scofalik · 2021-09-10T11:05:04Z

packages/ckeditor5-engine/src/conversion/downcasthelpers.js

+		if ( options.reconversion && viewChildNode && viewChildNode.root != viewElement.root ) {
+			writer.move(
+				writer.createRangeOn( viewChildNode ),
+				mapper.toViewPosition( ModelPosition._createBefore( modelChildNode ) )
+			);
+		} else {
+			conversionApi.convertInsert( ModelRange._createOn( modelChildNode ) );
+		}


Clarification needed.

Are there two modes because this function is sometimes used for the "first insertion" and sometimes for reconversion?

Do we check viewChildNode when there's reconversion flag because of text nodes?

Why do we check roots?

This is for cases when reconversion is triggered by adding/removing child nodes. The old view elements are restored and only the added child is converted.

We want to restore old view elements only for reconversion and not while the initial insert.

To make sure that that view element is not already added to the view structure.

scofalik · 2021-09-10T11:29:11Z

I like the API, I like or am neutral with almost all of the changes.

Other than what I commented:

Good points by PK.
I agree that slotFor( 'children' ) could simply be slotFor().
I don't like slotFor() name, I'd go with createSlot().
I also don't like "reduce" name, but I'd have to re-read this PR to be sure about that.
Sometimes I wasn't sure how given part of code works with text nodes (or whether text nodes (as a value of a variable) are even possible at given place in code), but I would have to re-read again to understand those cases.
Please re-read all comments and API docs, there are a lot of mistakes there (by a lot I mean more than one :D). Also, maybe you will spot places that can be further clarified.

Also, maybe you already considered that, but the sample with table is still a bit complex. Maybe slots could be named, so that if there is anything to put in the slot, a proper view element would be created automatically?

// second parameter is attributes, the usage looks like creating a regular view element
const tableHead = createSlot(
    'thead',
    null,
    element => element.is( 'element', 'tableRow' ) && element.index < headingRows
);

writer.insert( writer.createPositionAt( tableElement, 'end' ), tableHead );

Instead of:

if ( headingRows > 0 ) {
    const tableHead = writer.createContainerElement( 'thead' );
    const headSlot = slotFor( element => element.is( 'element', 'tableRow' ) && element.index < headingRows );

    writer.insert( writer.createPositionAt( tableElement, 'end' ), tableHead );
    writer.insert( writer.createPositionAt( tableHead, 0 ), headSlot );
}

I think it looks cool that you can create "view element" that takes callback instead of children ;). However it may be confusing (if you don't know how slots work, you may think that you will get an empty element, OTOH, you might want to have an empty element and in such case you need to go with the "regular way" to create elements).

niegowski · 2021-09-10T11:45:22Z

Also, maybe you already considered that, but the sample with table is still a bit complex. Maybe slots could be named, so that if there is anything to put in the slot, a proper view element would be created automatically?

This would force to wrap elements with a container element (thead in the example) and we would not be able to create such structure:

<figure class="image">
	<img/>
	<$slot for="children"/>
</figure>

In such case the slot does not need a wrapper, view elements should be inserted directly into the figure element at the specified view positions (so after the img).

scofalik · 2021-09-10T11:51:32Z

I never meant this should be the only way to create slots.

createSlot();
createSlot( callback );
createSlot( name, attributes, callback );

niegowski added 30 commits August 4, 2021 20:37

The elementToStructure downcast helper PoC extracted from other PoC b…

a7a0dfd

…ranch.

The elementToElement with a backward compatible reconversion support.

73af915

Simplified slot conversion.

dbe5571

Reconversion could be triggered by some ancestor change.

0ad44b9

Table downcast by reconversion.

490c302

Updated reconversion trigger conditions for table paragraph.

280d2c5

Removed support for legacy reconversion and simplified code for reins…

d0f0bd0

…erting elements (text nodes are not supported).

Cleaned DowncastDispatcher API.

e8ab9b7

Code cleanup.

cf242d5

Update html embed downcast conversion.

daae0cf

Merge branch 'master' into ck/10294-elementToStructure

5bf5ad3

Fixed marker double conversion.

34f06b3

Code cleaning. Added JSDocs.

4d90152

Merge branch 'master' into ck/10294-elementToStructure

89d803a

Adding code comments.

fa34826

Adding JSDocs.

614e1af

Merge branch 'master' into ck/10294-elementToStructure

aa36632

Added docs.

5a95106

Merge branch 'master' into ck/10294-elementToStructure

01afbce

API cleaning.

29ac32b

Removed some PoC changes.

ba694cb

Code cleaning. Added tests for DowncastDispatcher.

b0d0b2d

Ported tests for legacy "triggerBy" to test for elementToStructure.

1a7e155

Added tests.

bbc9e79

Added tests.

2e0f997

Merge branch 'master' into ck/10358-elementToStructure

6c18937

Reverted changes.

0053bd0

Tuned slot conversion manual test.

50bc5e5

Fixed tests.

edcdaa8

Code cleanup.

82f0176

niegowski and others added 6 commits September 7, 2021 18:52

Code cleanup.

818a93a

Only items that need conversion should be added to ModelConsumable.

1f2df2b

Merge branch 'master' into ck/10358-elementToStructure

d6dfe5a

Fix the schema definition.

9563fb2

Improved errors documentation.

f72e900

Wording.

ab03609

Reinmar requested review from scofalik and Reinmar September 8, 2021 13:50

Added tests for deferred unbinding.

dadf715

scofalik reviewed Sep 8, 2021

View reviewed changes

Improved the docs.

6cc2c0c

Reinmar merged commit eeb30a3 into ck/10294/reconversion Sep 9, 2021

Reinmar deleted the ck/10358-elementToStructure branch September 9, 2021 20:31

scofalik reviewed Sep 10, 2021

View reviewed changes

dawidurbanski mentioned this pull request Feb 8, 2022

New reconversion system #11211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#10358 Introducing a downcast helper elementToStructure with reconversion support #10466

#10358 Introducing a downcast helper elementToStructure with reconversion support #10466

niegowski commented Sep 1, 2021 •

edited

Loading

Reinmar commented Sep 8, 2021

niegowski commented Sep 8, 2021

scofalik Sep 8, 2021

niegowski Sep 8, 2021

niegowski Sep 8, 2021

scofalik Sep 8, 2021 •

edited

Loading

scofalik Sep 8, 2021

Reinmar commented Sep 9, 2021 •

edited

Loading

scofalik Sep 10, 2021 •

edited

Loading

scofalik Sep 10, 2021

scofalik Sep 10, 2021

niegowski Sep 10, 2021

scofalik Sep 10, 2021

scofalik Sep 10, 2021

scofalik Sep 10, 2021

niegowski Sep 10, 2021

scofalik commented Sep 10, 2021 •

edited

Loading

niegowski commented Sep 10, 2021

scofalik commented Sep 10, 2021

		@@ -335,21 +217,20 @@ export default class DowncastDispatcher {
		convertSelection( selection, markers, writer ) {

#10358 Introducing a downcast helper elementToStructure with reconversion support #10466

#10358 Introducing a downcast helper elementToStructure with reconversion support #10466

Conversation

niegowski commented Sep 1, 2021 • edited Loading

Suggested merge commit message (convention)

Additional information

Reinmar commented Sep 8, 2021

niegowski commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Sep 9, 2021 • edited Loading

scofalik Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik commented Sep 10, 2021 • edited Loading

niegowski commented Sep 10, 2021

scofalik commented Sep 10, 2021

niegowski commented Sep 1, 2021 •

edited

Loading

scofalik Sep 8, 2021 •

edited

Loading

Reinmar commented Sep 9, 2021 •

edited

Loading

scofalik Sep 10, 2021 •

edited

Loading

scofalik commented Sep 10, 2021 •

edited

Loading