8
0
فهرست منبع

Merge branch 'master' into i/3287

Piotrek Koszuliński 6 سال پیش
والد
کامیت
252b7a1455

+ 1 - 1
packages/ckeditor5-engine/.github/PULL_REQUEST_TEMPLATE.md

@@ -1,6 +1,6 @@
 ### Suggested merge commit message ([convention](https://github.com/ckeditor/ckeditor5-design/wiki/Git-commit-message-convention))
 
-Type: Message. Closes #000.
+Type: Message. Closes ckeditor/ckeditor5#000.
 
 ---
 

+ 13 - 0
packages/ckeditor5-engine/src/conversion/downcasthelpers.js

@@ -204,6 +204,19 @@ export default class DowncastHelpers extends ConversionHelpers {
 	 *			view: modelAttributeValue => ( { key: 'class', value: 'styled-' + modelAttributeValue } )
 	 *		} );
 	 *
+	 * *Note:* Downcasting to a style property requires providing `value` as an object:
+	 *
+	 *		editor.conversion.for( 'downcast' ).attributeToAttribute( {
+	 *			model: 'lineHeight',
+	 *			view: modelAttributeValue => ( {
+	 *				key: 'style',
+	 *				value: {
+	 *					'line-height': modelAttributeValue,
+	 *					'border-bottom': '1px dotted #ba2'
+	 *				}
+	 *			} )
+	 *		} );
+	 *
 	 * See {@link module:engine/conversion/conversion~Conversion#for `conversion.for()`} to learn how to add a converter
 	 * to the conversion process.
 	 *

+ 27 - 0
packages/ckeditor5-engine/src/conversion/upcasthelpers.js

@@ -237,6 +237,33 @@ export default class UpcastHelpers extends ConversionHelpers {
 	 *			}
 	 *		} );
 	 *
+	 * Converting styles works a bit differently as it requires `view.styles` to be an object and by default
+	 * a model attribute will be set to `true` by such a converter. You can set the model attribute to any value by providing the `value`
+	 * callback that returns the desired value.
+	 *
+	 *		// Default conversion of font-weight style will result in setting bold attribute to true.
+	 *		editor.conversion.for( 'upcast' ).attributeToAttribute( {
+	 *			view: {
+	 *				styles: {
+	 *					'font-weight': 'bold'
+	 *				}
+	 *			},
+	 *			model: 'bold'
+	 *		} );
+	 *
+	 *		// This converter will pass any style value to the `lineHeight` model attribute.
+	 *		editor.conversion.for( 'upcast' ).attributeToAttribute( {
+	 *			view: {
+	 *				styles: {
+	 *					'line-height': /[\s\S]+/
+	 *				}
+	 *			},
+	 *			model: {
+	 *				key: 'lineHeight',
+	 *				value: viewElement => viewElement.getStyle( 'line-height' )
+	 *			}
+	 *		} );
+	 *
 	 * See {@link module:engine/conversion/conversion~Conversion#for `conversion.for()`} to learn how to add a converter
 	 * to the conversion process.
 	 *

+ 0 - 1
packages/ckeditor5-engine/src/model/utils/deletecontent.js

@@ -23,7 +23,6 @@ import DocumentSelection from '../documentselection';
  * should be performed.
  * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
  * Selection of which the content should be deleted.
- * @param {module:engine/model/batch~Batch} batch Batch to which the operations will be added.
  * @param {Object} [options]
  * @param {Boolean} [options.leaveUnmerged=false] Whether to merge elements after removing the content of the selection.
  *

+ 13 - 1
packages/ckeditor5-engine/src/view/domconverter.js

@@ -807,11 +807,23 @@ export default class DomConverter {
 	 *
 	 * **Note:**: For the `'nbsp'` mode the method also checks context of a node so it cannot be a detached node.
 	 *
+	 * **Note:** A special case in the `'nbsp'` mode exists where the `<br>` in `<p><br></p>` is treated as a block filler.
+	 *
 	 * @param {Node} domNode DOM node to check.
 	 * @returns {Boolean} True if a node is considered a block filler for given mode.
 	 */
 	isBlockFiller( domNode ) {
-		return this.blockFillerMode == 'br' ? domNode.isEqualNode( BR_FILLER_REF ) : isNbspBlockFiller( domNode, this.blockElements );
+		if ( this.blockFillerMode == 'br' ) {
+			return domNode.isEqualNode( BR_FILLER_REF );
+		}
+
+		// Special case for <p><br></p> in which case the <br> should be treated as filler even
+		// when we're in the 'nbsp' mode. See ckeditor5#5564.
+		if ( domNode.tagName === 'BR' && hasBlockParent( domNode, this.blockElements ) && domNode.parentNode.childNodes.length === 1 ) {
+			return true;
+		}
+
+		return isNbspBlockFiller( domNode, this.blockElements );
 	}
 
 	/**

+ 49 - 0
packages/ckeditor5-engine/tests/tickets/5564.js

@@ -0,0 +1,49 @@
+/**
+ * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
+ */
+
+import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import ShiftEnter from '@ckeditor/ckeditor5-enter/src/shiftenter';
+
+import { getData as getModelData, setData as setModelData } from '../../src/dev-utils/model';
+
+describe( 'Bug ckeditor5#5564', () => {
+	let editor;
+
+	beforeEach( () => {
+		return VirtualTestEditor
+			.create( { plugins: [ Paragraph, ShiftEnter ] } )
+			.then( newEditor => {
+				editor = newEditor;
+			} );
+	} );
+
+	afterEach( () => {
+		return editor.destroy();
+	} );
+
+	it( 'does not create an excessive new line when loading <p>x</p><p><br></p><p>x</p>', () => {
+		editor.setData( '<p>x</p><p><br></p><p>x</p>' );
+
+		expect( getModelData( editor.model ) ).to.equal(
+			'<paragraph>[]x</paragraph><paragraph></paragraph><paragraph>x</paragraph>'
+		);
+	} );
+
+	it( 'preserves a soft break in an empty paragraph', () => {
+		setModelData( editor.model, '<paragraph>x</paragraph><paragraph><softBreak /></paragraph><paragraph>x</paragraph>' );
+
+		const expectedData = '<p>x</p><p><br>&nbsp;</p><p>x</p>';
+		const actualData = editor.getData();
+
+		expect( actualData ).to.equal( expectedData );
+
+		// Loading this data into the editor will actually create an excessive space as &nbsp; here isn't recognized as a filler.
+		// It's a known issue.
+		editor.setData( actualData );
+
+		expect( editor.getData() ).to.equal( expectedData );
+	} );
+} );

+ 56 - 11
packages/ckeditor5-engine/tests/view/domconverter/domconverter.js

@@ -297,37 +297,82 @@ describe( 'DomConverter', () => {
 				converter = new DomConverter( { blockFillerMode: 'nbsp' } );
 			} );
 
-			it( 'should return true if the node is an instance of the NBSP block filler', () => {
-				converter = new DomConverter( { blockFillerMode: 'nbsp' } );
+			it( 'should return true if the node is an nbsp filler and is a single child of a block level element', () => {
 				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
-				// NBSP must be check inside a context.
+
 				const context = document.createElement( 'div' );
 				context.appendChild( nbspFillerInstance );
 
 				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.true;
 			} );
 
-			it( 'should return false if the node is an instance of the BR block filler', () => {
-				const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap
+			it( 'should return false if the node is an nbsp filler and is not a single child of a block level element', () => {
+				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
 
-				expect( converter.isBlockFiller( brFillerInstance ) ).to.be.false;
+				const context = document.createElement( 'div' );
+				context.appendChild( nbspFillerInstance );
+				context.appendChild( document.createTextNode( 'a' ) );
+
+				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false;
 			} );
 
-			it( 'should return false if the nbsp filler is inside context', () => {
-				converter = new DomConverter( { blockFillerMode: 'nbsp' } );
+			it( 'should return false if there are two nbsp fillers in a block element', () => {
 				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
-				// NBSP must be check inside a context.
+
 				const context = document.createElement( 'div' );
 				context.appendChild( nbspFillerInstance );
-				// eslint-disable-next-line new-cap
-				context.appendChild( NBSP_FILLER( document ) );
+				context.appendChild( NBSP_FILLER( document ) ); // eslint-disable-line new-cap
+
+				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false;
+			} );
+
+			it( 'should return false filler is placed in a non-block element', () => {
+				const nbspFillerInstance = NBSP_FILLER( document ); // eslint-disable-line new-cap
+
+				const context = document.createElement( 'span' );
+				context.appendChild( nbspFillerInstance );
 
 				expect( converter.isBlockFiller( nbspFillerInstance ) ).to.be.false;
 			} );
 
+			it( 'should return false if the node is an instance of the BR block filler', () => {
+				const brFillerInstance = BR_FILLER( document ); // eslint-disable-line new-cap
+
+				expect( converter.isBlockFiller( brFillerInstance ) ).to.be.false;
+			} );
+
 			it( 'should return false for inline filler', () => {
 				expect( converter.isBlockFiller( document.createTextNode( INLINE_FILLER ) ) ).to.be.false;
 			} );
+
+			it( 'should return false for a normal <br> element', () => {
+				const context = document.createElement( 'div' );
+				context.innerHTML = 'x<br>x';
+
+				expect( converter.isBlockFiller( context.childNodes[ 1 ] ) ).to.be.false;
+			} );
+
+			// SPECIAL CASE (see ckeditor5#5564).
+			it( 'should return true for a <br> element which is the only child of its block parent', () => {
+				const context = document.createElement( 'div' );
+				context.innerHTML = '<br>';
+
+				expect( converter.isBlockFiller( context.firstChild ) ).to.be.true;
+			} );
+
+			it( 'should return false for a <br> element which is the only child of its non-block parent', () => {
+				const context = document.createElement( 'span' );
+				context.innerHTML = '<br>';
+
+				expect( converter.isBlockFiller( context.firstChild ) ).to.be.false;
+			} );
+
+			it( 'should return false for a <br> element which is followed by an nbsp', () => {
+				const context = document.createElement( 'span' );
+				context.innerHTML = '<br>&nbsp;';
+
+				expect( converter.isBlockFiller( context.firstChild ) ).to.be.false;
+			} );
 		} );
 
 		describe( 'mode "br"', () => {