Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Commit 22d387c

Browse files
ReinmarReinmar
authored
Merge pull request #23 from ckeditor/t/10-b
Fix: Content autoparagraphing has been improved. "Inline" view elements (converted to attributes or elements) will be now correctly handled and autoparagraphed. Closes #10. Closes #11.
2 parents c42d33e + 4740488 commit 22d387c

File tree

2 files changed

+166
-110
lines changed

2 files changed

+166
-110
lines changed

‎src/paragraph.js‎

Lines changed: 111 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
1212

1313
import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
1414
import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position';
15-
import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
16-
import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element';
17-
import ViewRange from '@ckeditor/ckeditor5-engine/src/view/range';
1815

19-
import modelWriter from '@ckeditor/ckeditor5-engine/src/model/writer';
2016
import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter';
2117
import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';
2218

@@ -43,6 +39,8 @@ export default class Paragraph extends Plugin {
4339
const data = editor.data;
4440
const editing = editor.editing;
4541

42+
editor.commands.set( 'paragraph', new ParagraphCommand( editor ) );
43+
4644
// Schema.
4745
doc.schema.registerItem( 'paragraph', '$block' );
4846

@@ -56,25 +54,29 @@ export default class Paragraph extends Plugin {
5654
.fromElement( 'p' )
5755
.toElement( 'paragraph' );
5856

59-
// Autoparagraph text.
60-
data.viewToModel.on( 'text', ( evt, data, consumable, conversionApi ) => {
61-
autoparagraphText( doc, evt, data, consumable, conversionApi );
62-
}, { priority: 'lowest' } );
57+
// Content autoparagraphing. --------------------------------------------------
6358

64-
// Post-fix potential subsequent paragraphs created by autoparagraphText().
65-
data.viewToModel.on( 'element', mergeSubsequentParagraphs, { priority: 'lowest' } );
66-
data.viewToModel.on( 'documentFragment', mergeSubsequentParagraphs, { priority: 'lowest' } );
59+
// Step 1.
60+
// "Second chance" converters for elements and texts which were not allowed in their original locations.
61+
// They check if this element/text could be converted if it was in a paragraph.
62+
// Forcefully converted items will be temporarily in an invalid context. It's going to be fixed in step 2.
6763

68-
// Convert paragraph-like elements to paragraphs if they weren't consumed.
69-
// It's a 'low' priority in order to hook in before the default 'element' converter
70-
// which would then convert children before handling this element.
71-
data.viewToModel.on( 'element', ( evt, data, consumable, conversionApi ) => {
72-
autoparagraphParagraphLikeElements( doc, evt, data, consumable, conversionApi );
73-
}, { priority: 'low' } );
64+
// Executed after converter added by a feature, but before "default" to-model-fragment converter.
65+
data.viewToModel.on( 'element', convertAutoparagraphableItem, { priority: 'low' } );
66+
// Executed after default text converter.
67+
data.viewToModel.on( 'text', convertAutoparagraphableItem, { priority: 'lowest' } );
7468

75-
editor.commands.set( 'paragraph', new ParagraphCommand( editor ) );
69+
// Step 2.
70+
// After an item is "forced" to be converted by `convertAutoparagraphableItem`, we need to actually take
71+
// care of adding the paragraph (assumed in `convertAutoparagraphableItem`) and wrap that item in it.
72+
73+
// Executed after all converters (even default ones).
74+
data.viewToModel.on( 'element', autoparagraphItems, { priority: 'lowest' } );
75+
data.viewToModel.on( 'documentFragment', autoparagraphItems, { priority: 'lowest' } );
76+
77+
// Empty roots autoparagraphing. -----------------------------------------------
7678

77-
// Post-fixer that takes care of adding empty paragraph elements to empty roots.
79+
// Post-fixer which takes care of adding empty paragraph elements to empty roots.
7880
// Besides fixing content on #changesDone we also need to handle #dataReady because
7981
// if initial data is empty or setData() wasn't even called there will be no #change fired.
8082
doc.on( 'change', ( evt, type, changes, batch ) => findEmptyRoots( doc, batch ) );
@@ -133,108 +135,124 @@ Paragraph.paragraphLikeElements = new Set( [
133135
'td'
134136
] );
135137

136-
const paragraphsToMerge = new WeakSet();
137-
138-
function autoparagraphText( doc, evt, data, consumable, conversionApi ) {
139-
// If text wasn't consumed by the default converter...
140-
if ( !consumable.test( data.input ) ) {
138+
// This converter forces a conversion of a non-consumed view item, if that item would be allowed by schema and converted it if was
139+
// inside a paragraph element. The converter checks whether conversion would be possible if there was a paragraph element
140+
// between `data.input` item and its parent. If the conversion would be allowed, the converter adds `"paragraph"` to the
141+
// context and fires conversion for `data.input` again.
142+
function convertAutoparagraphableItem( evt, data, consumable, conversionApi ) {
143+
// If the item wasn't consumed by some ot the dedicated converters...
144+
if ( !consumable.test( data.input, { name: data.input.name } ) ) {
141145
return;
142146
}
143147

144-
// And paragraph is allowed in this context...
145-
if ( !doc.schema.check( { name: 'paragraph', inside: data.context } ) ) {
148+
// But would be allowed if it was in a paragraph...
149+
if ( !isParagraphable( data.input, data.context, conversionApi.schema, false ) ) {
146150
return;
147151
}
148152

149-
// Let's do autoparagraphing.
150-
151-
const paragraph = new ModelElement( 'paragraph' );
152-
153-
paragraphsToMerge.add( paragraph );
154-
155-
data.context.push( paragraph );
156-
157-
const text = conversionApi.convertItem( data.input, consumable, data );
158-
159-
if ( text ) {
160-
data.output = paragraph;
161-
paragraph.appendChildren( text );
162-
}
163-
153+
// Convert that item in a paragraph context.
154+
data.context.push( 'paragraph' );
155+
const item = conversionApi.convertItem( data.input, consumable, data );
164156
data.context.pop();
157+
158+
data.output = item;
165159
}
166160

167-
function autoparagraphParagraphLikeElements( doc, evt, data, consumable, conversionApi ) {
168-
// If this is a paragraph-like element...
169-
if ( !Paragraph.paragraphLikeElements.has( data.input.name ) ) {
161+
// This converter checks all children of an element or document fragment that has been converted and wraps
162+
// children in a paragraph element if it is allowed by schema.
163+
//
164+
// Basically, after an item is "forced" to be converted by `convertAutoparagraphableItem`, we need to actually take
165+
// care of adding the paragraph (assumed in `convertAutoparagraphableItem`) and wrap that item in it.
166+
function autoparagraphItems( evt, data, consumable, conversionApi ) {
167+
// Autoparagraph only if the element has been converted.
168+
if ( !data.output ) {
170169
return;
171170
}
172171

173-
// Which wasn't consumed by its own converter...
174-
if ( !consumable.test( data.input, { name: true } ) ) {
172+
const isParagraphLike = Paragraph.paragraphLikeElements.has( data.input.name ) && !data.output.is( 'element' );
173+
174+
// Keep in mind that this converter is added to all elements and document fragments.
175+
// This means that we have to make a smart decision in which elements (at what level) auto-paragraph should be inserted.
176+
// There are three situations when it is correct to add paragraph:
177+
// - we are converting a view document fragment: this means that we are at the top level of conversion and we should
178+
// add paragraph elements for "bare" texts (unless converting in $clipboardHolder, but this is covered by schema),
179+
// - we are converting an element that was converted to model element: this means that it will be represented in model
180+
// and has added its context when converting children - we should add paragraph for those items that passed
181+
// in `convertAutoparagraphableItem`, because it is correct for them to be autoparagraphed,
182+
// - we are converting "paragraph-like" element, which children should always be autoparagraphed (if it is allowed by schema,
183+
// so we won't end up with, i.e., paragraph inside paragraph, if paragraph was in paragraph-like element).
184+
const shouldAutoparagraph =
185+
( data.input.is( 'documentFragment' ) ) ||
186+
( data.input.is( 'element' ) && data.output.is( 'element' ) ) ||
187+
isParagraphLike;
188+
189+
if ( !shouldAutoparagraph ) {
175190
return;
176191
}
177192

178-
// And there are no other paragraph-like elements inside this tree...
179-
if ( hasParagraphLikeContent( data.input ) ) {
180-
return;
181-
}
193+
// Take care of proper context. This is important for `isParagraphable` checks.
194+
const needsNewContext = data.output.is( 'element' );
182195

183-
// And paragraph is allowed in this context...
184-
if ( !doc.schema.check( { name: 'paragraph', inside: data.context } ) ) {
185-
return;
196+
if ( needsNewContext ) {
197+
data.context.push( data.output );
186198
}
187199

188-
// Let's convert this element to a paragraph and then all its children.
189-
190-
consumable.consume( data.input, { name: true } );
191-
192-
const paragraph = new ModelElement( 'paragraph' );
193-
194-
data.context.push( paragraph );
195-
196-
const convertedChildren = conversionApi.convertChildren( data.input, consumable, data );
197-
198-
paragraph.appendChildren( modelWriter.normalizeNodes( convertedChildren ) );
199-
200-
// Remove the created paragraph from the stack for other converters.
201-
// See https://github.com/ckeditor/ckeditor5-engine/issues/736
202-
data.context.pop();
203-
204-
data.output = paragraph;
205-
}
200+
// `paragraph` element that will wrap auto-paragraphable children.
201+
let autoParagraph = null;
202+
203+
// Check children and wrap them in a `paragraph` element if they need to be wrapped.
204+
// Be smart when wrapping children and put all auto-paragraphable siblings in one `paragraph` parent:
205+
// foo<$text bold="true">bar</$text><paragraph>xxx</paragraph>baz --->
206+
// <paragraph>foo<$text bold="true">bar</$text></paragraph><paragraph>xxx</paragraph><paragraph>baz</paragraph>
207+
for ( let i = 0; i < data.output.childCount; i++ ) {
208+
const child = data.output.getChild( i );
209+
210+
if ( isParagraphable( child, data.context, conversionApi.schema, isParagraphLike ) ) {
211+
// If there is no wrapping `paragraph` element, create it.
212+
if ( !autoParagraph ) {
213+
autoParagraph = new ModelElement( 'paragraph' );
214+
data.output.insertChildren( child.index, autoParagraph );
215+
}
216+
// Otherwise, use existing `paragraph` and just fix iterator.
217+
// Thanks to reusing `paragraph` element, multiple siblings ends up in same container.
218+
else {
219+
i--;
220+
}
206221

207-
// Merges subsequent paragraphs if they should be merged (see shouldMerge).
208-
function mergeSubsequentParagraphs( evt, data ) {
209-
if ( !data.output ) {
210-
return;
222+
child.remove();
223+
autoParagraph.appendChildren( child );
224+
} else {
225+
// That was not a paragraphable children, reset `paragraph` wrapper - following auto-paragraphable children
226+
// need to be placed in a new `paragraph` element.
227+
autoParagraph = null;
228+
}
211229
}
212230

213-
let node = data.output.getChild( 0 );
231+
if ( needsNewContext ) {
232+
data.context.pop();
233+
}
234+
}
214235

215-
while ( node && node.nextSibling ) {
216-
const nextSibling = node.nextSibling;
236+
function isParagraphable( node, context, schema, insideParagraphLikeElement ) {
237+
const name = node.name || '$text';
217238

218-
if ( paragraphsToMerge.has( node ) && paragraphsToMerge.has( nextSibling ) ) {
219-
modelWriter.insert( ModelPosition.createAt( node, 'end' ), Array.from( nextSibling.getChildren() ) );
220-
modelWriter.remove( ModelRange.createOn( nextSibling ) );
221-
} else {
222-
node = node.nextSibling;
223-
}
239+
// Node is paragraphable if it is inside paragraph like element, or...
240+
// It is not allowed at this context...
241+
if ( !insideParagraphLikeElement && schema.check( { name: name, inside: context } ) ) {
242+
return false;
224243
}
225-
}
226244

227-
// Checks whether an element has paragraph-like descendant.
228-
function hasParagraphLikeContent( element ) {
229-
const range = ViewRange.createIn( element );
245+
// And paragraph is allowed in this context...
246+
if ( !schema.check( { name: 'paragraph', inside: context } ) ) {
247+
return false;
248+
}
230249

231-
for ( const value of range ) {
232-
if ( value.item instanceof ViewElement && Paragraph.paragraphLikeElements.has( value.item.name ) ) {
233-
return true;
234-
}
250+
// And a node would be allowed in this paragraph...
251+
if ( !schema.check( { name: name, inside: context.concat( 'paragraph' ) } ) ) {
252+
return false;
235253
}
236254

237-
return false;
255+
return true;
238256
}
239257

240258
// Looks through all roots created in document and marks every empty root, saving which batch made it empty.

‎tests/paragraph.js‎

Lines changed: 55 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ import {
1313
} from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
1414
import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
1515

16-
import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';
1716
import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter';
17+
import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';
1818

1919
import ModelDocumentFragment from '@ckeditor/ckeditor5-engine/src/model/documentfragment';
2020
import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
@@ -78,6 +78,19 @@ describe( 'Paragraph feature', () => {
7878
expect( editor.getData() ).to.equal( '<p>foo</p>' );
7979
} );
8080

81+
it( 'should autoparagraph any inline element', () => {
82+
editor.document.schema.registerItem( 'span', '$inline' );
83+
editor.document.schema.allow( { name: '$text', inside: '$inline' } );
84+
85+
buildModelConverter().for( editor.editing.modelToView, editor.data.modelToView ).fromElement( 'span' ).toElement( 'span' );
86+
buildViewConverter().for( editor.data.viewToModel ).fromElement( 'span' ).toElement( 'span' );
87+
88+
editor.setData( '<span>foo</span>' );
89+
90+
expect( getModelData( doc, { withoutSelection: true } ) ).to.equal( '<paragraph><span>foo</span></paragraph>' );
91+
expect( editor.getData() ).to.equal( '<p><span>foo</span></p>' );
92+
} );
93+
8194
it( 'should not autoparagraph text (in clipboard holder)', () => {
8295
const modelFragment = editor.data.parse( 'foo', '$clipboardHolder' );
8396

@@ -170,20 +183,6 @@ describe( 'Paragraph feature', () => {
170183
expect( modelFragment.getChild( 0 ).getChild( 0 ) ).to.be.instanceOf( ModelText );
171184
} );
172185

173-
it( 'does not break converting inline elements', () => {
174-
doc.schema.allow( { name: '$inline', attributes: [ 'bold' ] } );
175-
buildViewConverter().for( editor.data.viewToModel )
176-
.fromElement( 'b' )
177-
.toAttribute( 'bold', true );
178-
179-
const modelFragment = editor.data.parse( 'foo<b>bar</b>bom' );
180-
181-
// The result of this test is wrong due to https://github.com/ckeditor/ckeditor5-paragraph/issues/10.
182-
// It's meant to catch the odd situation in mergeSubsequentParagraphs when data.output may be an array
183-
// for a while
184-
expect( stringifyModel( modelFragment ) ).to.equal( '<paragraph>foobarbom</paragraph>' );
185-
} );
186-
187186
// This test was taken from the list package.
188187
it( 'does not break when some converter returns nothing', () => {
189188
editor.data.viewToModel.on( 'element:li', ( evt, data, consumable ) => {
@@ -194,6 +193,45 @@ describe( 'Paragraph feature', () => {
194193

195194
expect( stringifyModel( modelFragment ) ).to.equal( '' );
196195
} );
196+
197+
describe( 'should not strip attribute elements when autoparagraphing texts', () => {
198+
beforeEach( () => {
199+
doc.schema.allow( { name: '$inline', attributes: [ 'bold' ] } );
200+
buildViewConverter().for( editor.data.viewToModel )
201+
.fromElement( 'b' )
202+
.toAttribute( 'bold', true );
203+
} );
204+
205+
it( 'inside document fragment', () => {
206+
const modelFragment = editor.data.parse( 'foo<b>bar</b>bom' );
207+
208+
expect( stringifyModel( modelFragment ) ).to.equal( '<paragraph>foo<$text bold="true">bar</$text>bom</paragraph>' );
209+
} );
210+
211+
it( 'inside converted element', () => {
212+
doc.schema.registerItem( 'blockquote' );
213+
doc.schema.allow( { name: 'blockquote', inside: '$root' } );
214+
doc.schema.allow( { name: '$block', inside: 'blockquote' } );
215+
216+
buildModelConverter().for( editor.editing.modelToView, editor.data.modelToView )
217+
.fromElement( 'blockquote' )
218+
.toElement( 'blockquote' );
219+
220+
buildViewConverter().for( editor.data.viewToModel ).fromElement( 'blockquote' ).toElement( 'blockquote' );
221+
222+
const modelFragment = editor.data.parse( '<blockquote>foo<b>bar</b>bom</blockquote>' );
223+
224+
expect( stringifyModel( modelFragment ) )
225+
.to.equal( '<blockquote><paragraph>foo<$text bold="true">bar</$text>bom</paragraph></blockquote>' );
226+
} );
227+
228+
it( 'inside paragraph-like element', () => {
229+
const modelFragment = editor.data.parse( '<h1>foo</h1><h2><b>bar</b>bom</h2>' );
230+
231+
expect( stringifyModel( modelFragment ) )
232+
.to.equal( '<paragraph>foo</paragraph><paragraph><$text bold="true">bar</$text>bom</paragraph>' );
233+
} );
234+
} );
197235
} );
198236

199237
describe( 'generic block converter (paragraph-like element handling)', () => {
@@ -252,7 +290,7 @@ describe( 'Paragraph feature', () => {
252290
const modelFragment = editor.data.parse( '<ul><li>a<ul><li>b</li><li>c</li></ul></li></ul>', '$clipboardHolder' );
253291

254292
expect( stringifyModel( modelFragment ) )
255-
.to.equal( 'a<paragraph>b</paragraph><paragraph>c</paragraph>' );
293+
.to.equal( '<paragraph>a</paragraph><paragraph>b</paragraph><paragraph>c</paragraph>' );
256294
} );
257295

258296
it( 'should convert ul>li>p,text', () => {
@@ -268,7 +306,7 @@ describe( 'Paragraph feature', () => {
268306
const modelFragment = editor.data.parse( '<ul><li><p>a</p>b</li></ul>', '$clipboardHolder' );
269307

270308
expect( stringifyModel( modelFragment ) )
271-
.to.equal( '<paragraph>a</paragraph>b' );
309+
.to.equal( '<paragraph>a</paragraph><paragraph>b</paragraph>' );
272310
} );
273311

274312
it( 'should convert td', () => {

0 commit comments

Comments
 (0)