Skip to content

Commit

Permalink
Merge pull request #13287 from ckeditor/ck/13285
Browse files Browse the repository at this point in the history
Fix (engine, list, table): Markers were not properly set on list items and on elements in table cells, resulting in losing comments and suggestions after re-loading the document. Closes #13285.
  • Loading branch information
scofalik authored Jan 21, 2023
2 parents 30159ba + fe78ca0 commit 1fa7ffc
Show file tree
Hide file tree
Showing 11 changed files with 763 additions and 58 deletions.
4 changes: 2 additions & 2 deletions packages/ckeditor5-clipboard/tests/dragdrop.js
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ describe( 'Drag and Drop', () => {
expect( spyClipboardOutput.firstCall.firstArg.method ).to.equal( 'dragstart' );
expect( spyClipboardOutput.firstCall.firstArg.dataTransfer ).to.equal( dataTransferMock );
expect( stringifyView( spyClipboardOutput.firstCall.firstArg.content ) ).to.equal(
'<figure class="table"><table><tbody><tr><td>abc</td></tr></tbody></table></figure>'
'<figure class="table"><table><tbody><tr><td><p>abc</p></td></tr></tbody></table></figure>'
);

dataTransferMock.dropEffect = 'move';
Expand Down Expand Up @@ -929,7 +929,7 @@ describe( 'Drag and Drop', () => {
expect( spyClipboardOutput.firstCall.firstArg.method ).to.equal( 'dragstart' );
expect( spyClipboardOutput.firstCall.firstArg.dataTransfer ).to.equal( dataTransferMock );
expect( stringifyView( spyClipboardOutput.firstCall.firstArg.content ) ).to.equal(
'<figure class="table"><table><tbody><tr><td>abc</td></tr></tbody></table></figure>'
'<figure class="table"><table><tbody><tr><td><p>abc</p></td></tr></tbody></table></figure>'
);
} );

Expand Down
6 changes: 4 additions & 2 deletions packages/ckeditor5-engine/src/view/domconverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import {
indexOf,
getAncestors,
isText,
isComment
isComment,
first
} from '@ckeditor/ckeditor5-utils';

import type ViewNode from './node';
Expand Down Expand Up @@ -558,7 +559,8 @@ export default class DomConverter {
}

const transparentRendering = childView.is( 'element' ) &&
( childView.getCustomProperty( 'dataPipeline:transparentRendering' ) as boolean | undefined );
!!childView.getCustomProperty( 'dataPipeline:transparentRendering' ) &&
!first( childView.getAttributes() );

if ( transparentRendering && this.renderingMode == 'data' ) {
yield* this.viewChildrenToDom( childView, options );
Expand Down
62 changes: 62 additions & 0 deletions packages/ckeditor5-engine/tests/view/domconverter/view-to-dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -1291,6 +1291,68 @@ describe( 'DomConverter', () => {
sinon.match.string // Link to the documentation
);
} );

it( 'should not be transparent in the data pipeline if has any attribute', () => {
converter.renderingMode = 'data';
converter.blockFillerMode = 'nbsp';

const warnStub = testUtils.sinon.stub( console, 'warn' );

const viewList = parse(
'<container:div>' +
'<attribute:ul>' +
'<attribute:li>' +
'<container:p class="style">foo</container:p>' +
'</attribute:li>' +
'<attribute:li>' +
'<container:p data-foo="123">bar</container:p>' +
'</attribute:li>' +
'<attribute:li>' +
'<container:p>baz</container:p>' +
'</attribute:li>' +
'</attribute:ul>' +
'</container:div>'
);

const bogusParagraph1 = viewList.getChild( 0 ).getChild( 0 ).getChild( 0 );
const bogusParagraph2 = viewList.getChild( 0 ).getChild( 1 ).getChild( 0 );
const bogusParagraph3 = viewList.getChild( 0 ).getChild( 2 ).getChild( 0 );

bogusParagraph1._setCustomProperty( 'dataPipeline:transparentRendering', true );
bogusParagraph2._setCustomProperty( 'dataPipeline:transparentRendering', true );
bogusParagraph3._setCustomProperty( 'dataPipeline:transparentRendering', true );

const domDivChildren = Array.from( converter.viewChildrenToDom( viewList ) );

expect( domDivChildren.length ).to.equal( 1 );
expect( domDivChildren[ 0 ].tagName.toLowerCase() ).to.equal( 'ul' );

const domUlChildren = Array.from( domDivChildren[ 0 ].childNodes );

expect( domUlChildren.length ).to.equal( 3 );
expect( domUlChildren[ 0 ].tagName.toLowerCase() ).to.equal( 'li' );
expect( domUlChildren[ 1 ].tagName.toLowerCase() ).to.equal( 'li' );
expect( domUlChildren[ 2 ].tagName.toLowerCase() ).to.equal( 'li' );

const domUl1Children = Array.from( domUlChildren[ 0 ].childNodes );
const domUl2Children = Array.from( domUlChildren[ 1 ].childNodes );
const domUl3Children = Array.from( domUlChildren[ 2 ].childNodes );

expect( domUl1Children.length ).to.equal( 1 );
expect( domUl1Children[ 0 ].tagName.toLowerCase() ).to.equal( 'p' );
expect( domUl1Children[ 0 ].getAttribute( 'class' ) ).to.equal( 'style' );
expect( domUl1Children[ 0 ].firstChild.data ).to.equal( 'foo' );

expect( domUl2Children.length ).to.equal( 1 );
expect( domUl2Children[ 0 ].tagName.toLowerCase() ).to.equal( 'p' );
expect( domUl2Children[ 0 ].getAttribute( 'data-foo' ) ).to.equal( '123' );
expect( domUl2Children[ 0 ].firstChild.data ).to.equal( 'bar' );

expect( domUl3Children.length ).to.equal( 1 );
expect( domUl3Children[ 0 ].data ).to.equal( 'baz' );

sinon.assert.notCalled( warnStub );
} );
} );
} );

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1157,8 +1157,8 @@ describe( 'HtmlComment integration', () => {
'<tbody>' +
'<tr>' +
'<td>' +
'<!-- c1 -->' +
'&nbsp;' +
'<!-- c1 -->' +
'</td>' +
'</tr>' +
'</tbody>' +
Expand Down
11 changes: 7 additions & 4 deletions packages/ckeditor5-list/src/documentlist/converters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -369,12 +369,15 @@ export function bogusParagraphCreator(
return null;
}

const viewElement = writer.createContainerElement( 'span', { class: 'ck-list-bogus-paragraph' } );

if ( dataPipeline ) {
writer.setCustomProperty( 'dataPipeline:transparentRendering', true, viewElement );
if ( !dataPipeline ) {
return writer.createContainerElement( 'span', { class: 'ck-list-bogus-paragraph' } );
}

// Using `<p>` in case there are some markers on it and transparentRendering will render it anyway.
const viewElement = writer.createContainerElement( 'p' );

writer.setCustomProperty( 'dataPipeline:transparentRendering', true, viewElement );

return viewElement;
};
}
Expand Down
Loading

0 comments on commit 1fa7ffc

Please sign in to comment.