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

Commit

Permalink
Merge pull request #23 from ckeditor/t/10-b
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@Reinmar
Reinmar committed May 3, 2017
2 parents c42d33e + 4740488 commit 22d387c
Show file tree
Hide file tree
Showing 2 changed files with 166 additions and 110 deletions.
204 changes: 111 additions & 93 deletions src/paragraph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';

import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position';
import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element';
import ViewRange from '@ckeditor/ckeditor5-engine/src/view/range';

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

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

editor.commands.set( 'paragraph', new ParagraphCommand( editor ) );

// Schema.
doc.schema.registerItem( 'paragraph', '$block' );

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

// Autoparagraph text.
data.viewToModel.on( 'text', ( evt, data, consumable, conversionApi ) => {
autoparagraphText( doc, evt, data, consumable, conversionApi );
}, { priority: 'lowest' } );
// Content autoparagraphing. --------------------------------------------------

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

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

editor.commands.set( 'paragraph', new ParagraphCommand( editor ) );
// Step 2.
// After an item is "forced" to be converted by `convertAutoparagraphableItem`, we need to actually take
// care of adding the paragraph (assumed in `convertAutoparagraphableItem`) and wrap that item in it.

// Executed after all converters (even default ones).
data.viewToModel.on( 'element', autoparagraphItems, { priority: 'lowest' } );
data.viewToModel.on( 'documentFragment', autoparagraphItems, { priority: 'lowest' } );

// Empty roots autoparagraphing. -----------------------------------------------

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

const paragraphsToMerge = new WeakSet();

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

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

// Let's do autoparagraphing.

const paragraph = new ModelElement( 'paragraph' );

paragraphsToMerge.add( paragraph );

data.context.push( paragraph );

const text = conversionApi.convertItem( data.input, consumable, data );

if ( text ) {
data.output = paragraph;
paragraph.appendChildren( text );
}

// Convert that item in a paragraph context.
data.context.push( 'paragraph' );
const item = conversionApi.convertItem( data.input, consumable, data );
data.context.pop();

data.output = item;
}

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

// Which wasn't consumed by its own converter...
if ( !consumable.test( data.input, { name: true } ) ) {
const isParagraphLike = Paragraph.paragraphLikeElements.has( data.input.name ) && !data.output.is( 'element' );

// Keep in mind that this converter is added to all elements and document fragments.
// This means that we have to make a smart decision in which elements (at what level) auto-paragraph should be inserted.
// There are three situations when it is correct to add paragraph:
// - we are converting a view document fragment: this means that we are at the top level of conversion and we should
// add paragraph elements for "bare" texts (unless converting in $clipboardHolder, but this is covered by schema),
// - we are converting an element that was converted to model element: this means that it will be represented in model
// and has added its context when converting children - we should add paragraph for those items that passed
// in `convertAutoparagraphableItem`, because it is correct for them to be autoparagraphed,
// - we are converting "paragraph-like" element, which children should always be autoparagraphed (if it is allowed by schema,
// so we won't end up with, i.e., paragraph inside paragraph, if paragraph was in paragraph-like element).
const shouldAutoparagraph =
( data.input.is( 'documentFragment' ) ) ||
( data.input.is( 'element' ) && data.output.is( 'element' ) ) ||
isParagraphLike;

if ( !shouldAutoparagraph ) {
return;
}

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

// And paragraph is allowed in this context...
if ( !doc.schema.check( { name: 'paragraph', inside: data.context } ) ) {
return;
if ( needsNewContext ) {
data.context.push( data.output );
}

// Let's convert this element to a paragraph and then all its children.

consumable.consume( data.input, { name: true } );

const paragraph = new ModelElement( 'paragraph' );

data.context.push( paragraph );

const convertedChildren = conversionApi.convertChildren( data.input, consumable, data );

paragraph.appendChildren( modelWriter.normalizeNodes( convertedChildren ) );

// Remove the created paragraph from the stack for other converters.
// See https://github.com/ckeditor/ckeditor5-engine/issues/736
data.context.pop();

data.output = paragraph;
}
// `paragraph` element that will wrap auto-paragraphable children.
let autoParagraph = null;

// Check children and wrap them in a `paragraph` element if they need to be wrapped.
// Be smart when wrapping children and put all auto-paragraphable siblings in one `paragraph` parent:
// foo<$text bold="true">bar</$text><paragraph>xxx</paragraph>baz --->
// <paragraph>foo<$text bold="true">bar</$text></paragraph><paragraph>xxx</paragraph><paragraph>baz</paragraph>
for ( let i = 0; i < data.output.childCount; i++ ) {
const child = data.output.getChild( i );

if ( isParagraphable( child, data.context, conversionApi.schema, isParagraphLike ) ) {
// If there is no wrapping `paragraph` element, create it.
if ( !autoParagraph ) {
autoParagraph = new ModelElement( 'paragraph' );
data.output.insertChildren( child.index, autoParagraph );
}
// Otherwise, use existing `paragraph` and just fix iterator.
// Thanks to reusing `paragraph` element, multiple siblings ends up in same container.
else {
i--;
}

// Merges subsequent paragraphs if they should be merged (see shouldMerge).
function mergeSubsequentParagraphs( evt, data ) {
if ( !data.output ) {
return;
child.remove();
autoParagraph.appendChildren( child );
} else {
// That was not a paragraphable children, reset `paragraph` wrapper - following auto-paragraphable children
// need to be placed in a new `paragraph` element.
autoParagraph = null;
}
}

let node = data.output.getChild( 0 );
if ( needsNewContext ) {
data.context.pop();
}
}

while ( node && node.nextSibling ) {
const nextSibling = node.nextSibling;
function isParagraphable( node, context, schema, insideParagraphLikeElement ) {
const name = node.name || '$text';

if ( paragraphsToMerge.has( node ) && paragraphsToMerge.has( nextSibling ) ) {
modelWriter.insert( ModelPosition.createAt( node, 'end' ), Array.from( nextSibling.getChildren() ) );
modelWriter.remove( ModelRange.createOn( nextSibling ) );
} else {
node = node.nextSibling;
}
// Node is paragraphable if it is inside paragraph like element, or...
// It is not allowed at this context...
if ( !insideParagraphLikeElement && schema.check( { name: name, inside: context } ) ) {
return false;
}
}

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

for ( const value of range ) {
if ( value.item instanceof ViewElement && Paragraph.paragraphLikeElements.has( value.item.name ) ) {
return true;
}
// And a node would be allowed in this paragraph...
if ( !schema.check( { name: name, inside: context.concat( 'paragraph' ) } ) ) {
return false;
}

return false;
return true;
}

// Looks through all roots created in document and marks every empty root, saving which batch made it empty.
Expand Down
72 changes: 55 additions & 17 deletions tests/paragraph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import {
} from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';

import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';
import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter';
import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';

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

it( 'should autoparagraph any inline element', () => {
editor.document.schema.registerItem( 'span', '$inline' );
editor.document.schema.allow( { name: '$text', inside: '$inline' } );

buildModelConverter().for( editor.editing.modelToView, editor.data.modelToView ).fromElement( 'span' ).toElement( 'span' );
buildViewConverter().for( editor.data.viewToModel ).fromElement( 'span' ).toElement( 'span' );

editor.setData( '<span>foo</span>' );

expect( getModelData( doc, { withoutSelection: true } ) ).to.equal( '<paragraph><span>foo</span></paragraph>' );
expect( editor.getData() ).to.equal( '<p><span>foo</span></p>' );
} );

it( 'should not autoparagraph text (in clipboard holder)', () => {
const modelFragment = editor.data.parse( 'foo', '$clipboardHolder' );

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

it( 'does not break converting inline elements', () => {
doc.schema.allow( { name: '$inline', attributes: [ 'bold' ] } );
buildViewConverter().for( editor.data.viewToModel )
.fromElement( 'b' )
.toAttribute( 'bold', true );

const modelFragment = editor.data.parse( 'foo<b>bar</b>bom' );

// The result of this test is wrong due to https://github.com/ckeditor/ckeditor5-paragraph/issues/10.
// It's meant to catch the odd situation in mergeSubsequentParagraphs when data.output may be an array
// for a while
expect( stringifyModel( modelFragment ) ).to.equal( '<paragraph>foobarbom</paragraph>' );
} );

// This test was taken from the list package.
it( 'does not break when some converter returns nothing', () => {
editor.data.viewToModel.on( 'element:li', ( evt, data, consumable ) => {
Expand All @@ -194,6 +193,45 @@ describe( 'Paragraph feature', () => {

expect( stringifyModel( modelFragment ) ).to.equal( '' );
} );

describe( 'should not strip attribute elements when autoparagraphing texts', () => {
beforeEach( () => {
doc.schema.allow( { name: '$inline', attributes: [ 'bold' ] } );
buildViewConverter().for( editor.data.viewToModel )
.fromElement( 'b' )
.toAttribute( 'bold', true );
} );

it( 'inside document fragment', () => {
const modelFragment = editor.data.parse( 'foo<b>bar</b>bom' );

expect( stringifyModel( modelFragment ) ).to.equal( '<paragraph>foo<$text bold="true">bar</$text>bom</paragraph>' );
} );

it( 'inside converted element', () => {
doc.schema.registerItem( 'blockquote' );
doc.schema.allow( { name: 'blockquote', inside: '$root' } );
doc.schema.allow( { name: '$block', inside: 'blockquote' } );

buildModelConverter().for( editor.editing.modelToView, editor.data.modelToView )
.fromElement( 'blockquote' )
.toElement( 'blockquote' );

buildViewConverter().for( editor.data.viewToModel ).fromElement( 'blockquote' ).toElement( 'blockquote' );

const modelFragment = editor.data.parse( '<blockquote>foo<b>bar</b>bom</blockquote>' );

expect( stringifyModel( modelFragment ) )
.to.equal( '<blockquote><paragraph>foo<$text bold="true">bar</$text>bom</paragraph></blockquote>' );
} );

it( 'inside paragraph-like element', () => {
const modelFragment = editor.data.parse( '<h1>foo</h1><h2><b>bar</b>bom</h2>' );

expect( stringifyModel( modelFragment ) )
.to.equal( '<paragraph>foo</paragraph><paragraph><$text bold="true">bar</$text>bom</paragraph>' );
} );
} );
} );

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

expect( stringifyModel( modelFragment ) )
.to.equal( 'a<paragraph>b</paragraph><paragraph>c</paragraph>' );
.to.equal( '<paragraph>a</paragraph><paragraph>b</paragraph><paragraph>c</paragraph>' );
} );

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

expect( stringifyModel( modelFragment ) )
.to.equal( '<paragraph>a</paragraph>b' );
.to.equal( '<paragraph>a</paragraph><paragraph>b</paragraph>' );
} );

it( 'should convert td', () => {
Expand Down

0 comments on commit 22d387c

Please sign in to comment.