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

Reconversion same event names overwrite existing events #9641

Closed
jonplata opened this issue May 6, 2021 · 4 comments
Closed

Reconversion same event names overwrite existing events #9641

jonplata opened this issue May 6, 2021 · 4 comments
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.

Comments

@jonplata
Copy link

jonplata commented May 6, 2021

📝 Provide detailed reproduction steps (if any)

Adding same triggerBy children elements in multiple custom plugins breaks reconversion. Only in one of many plugins reconversion triggers correctly.

// Test1.js
conversion.for( 'editingDowncast' ).elementToElement({
  model: 'test1Title',
  view: { name: 'div', classes: 'test1-title' },
  triggerBy: {
    children: [ 'paragraph' ],
  },
});
// Test2.js
conversion.for( 'editingDowncast' ).elementToElement({
  model: 'test2Title',
  view: { name: 'div', classes: 'test2-title' },
  triggerBy: {
    children: [ 'paragraph' ],
  },
});

If we log _reconversionEventsMapping we can see that test2 "insert:paragraph" event overwritten test1 event:
Screenshot 2021-05-06 at 22 02 12

@jonplata jonplata added the type:bug This issue reports a buggy (incorrect) behavior. label May 6, 2021
@Reinmar
Copy link
Member

Reinmar commented Aug 5, 2021

Hi @jonplata! We're working now on a new system for reconversion that will most likely phase out the current one. We hope to resolve issues like this in the new one.

Ref: #10294

@jonplata
Copy link
Author

jonplata commented Oct 1, 2021

For now, we had to use standard solutions instead of reconversion, but great to hear that it's being taken care of! 😄

@Reinmar
Copy link
Member

Reinmar commented Feb 11, 2022

cc @niegowski how does the new API affect this ticket?

@niegowski
Copy link
Contributor

cc @niegowski how does the new API affect this ticket?

There is no such problem anymore (on the master branch). The triggerBy API was replaced with a new approach.

We even covered this case with tests:

// https://github.com/ckeditor/ckeditor5/issues/9641
it( 'should convert on multiple similar child hooks', () => {
model.schema.register( 'simpleBlock2', {
allowIn: '$root',
allowChildren: 'paragraph'
} );
downcastHelpers.elementToStructure( {
model: {
name: 'simpleBlock2',
children: true
},
view: ( modelElement, { writer } ) => {
const viewElement = writer.createContainerElement( 'div', { class: 'second' } );
writer.insert( writer.createPositionAt( viewElement, 0 ), writer.createSlot() );
return viewElement;
}
} );
setModelData( model,
'<simpleBlock><paragraph>foo</paragraph></simpleBlock>' +
'<simpleBlock2><paragraph>bar</paragraph></simpleBlock2>'
);
const [ viewBefore0, paraBefore0, textBefore0 ] = getNodes( 0 );
const [ viewBefore1, paraBefore1, textBefore1 ] = getNodes( 1 );
model.change( writer => {
const paragraph = writer.createElement( 'paragraph' );
const text = writer.createText( 'abc' );
writer.insert( paragraph, modelRoot.getChild( 0 ), 1 );
writer.insert( text, paragraph, 0 );
} );
const [ viewAfter0, paraAfter0, textAfter0 ] = getNodes( 0 );
const [ viewAfter1, paraAfter1, textAfter1 ] = getNodes( 1 );
expectResult(
'<div><p>foo</p><p>abc</p></div>' +
'<div class="second"><p>bar</p></div>'
);
expect( viewAfter0, 'simpleBlock' ).to.not.equal( viewBefore0 );
expect( paraAfter0, 'para' ).to.equal( paraBefore0 );
expect( textAfter0, 'text' ).to.equal( textBefore0 );
expect( viewAfter1, 'simpleBlock' ).to.equal( viewBefore1 );
expect( paraAfter1, 'para' ).to.equal( paraBefore1 );
expect( textAfter1, 'text' ).to.equal( textBefore1 );
model.change( writer => {
const paragraph = writer.createElement( 'paragraph' );
const text = writer.createText( '123' );
writer.insert( paragraph, modelRoot.getChild( 1 ), 1 );
writer.insert( text, paragraph, 0 );
} );
const [ viewAfterAfter0, paraAfterAfter0, textAfterAfter0 ] = getNodes( 0 );
const [ viewAfterAfter1, paraAfterAfter1, textAfterAfter1 ] = getNodes( 1 );
expectResult(
'<div><p>foo</p><p>abc</p></div>' +
'<div class="second"><p>bar</p><p>123</p></div>'
);
expect( viewAfter0, 'simpleBlock' ).to.not.equal( viewBefore0 );
expect( paraAfter0, 'para' ).to.equal( paraBefore0 );
expect( textAfter0, 'text' ).to.equal( textBefore0 );
expect( viewAfter1, 'simpleBlock' ).to.equal( viewBefore1 );
expect( paraAfter1, 'para' ).to.equal( paraBefore1 );
expect( textAfter1, 'text' ).to.equal( textBefore1 );
expect( viewAfterAfter0, 'simpleBlock' ).to.equal( viewAfter0 );
expect( paraAfterAfter0, 'para' ).to.equal( paraAfter0 );
expect( textAfterAfter0, 'text' ).to.equal( textAfter0 );
expect( viewAfterAfter1, 'simpleBlock' ).to.not.equal( viewAfter1 );
expect( paraAfterAfter1, 'para' ).to.equal( paraAfter1 );
expect( textAfterAfter1, 'text' ).to.equal( textAfter1 );
} );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

3 participants