Merge remote-tracking branch 'origin/master' into s/ckeditor5/3

docs/framework/guides/contributing/git-commit-message-convention.md

@@ -24,8 +24,8 @@ Type (another-package-name): If the change affects more than one package, it's p
 
 Optional description.
 
-BREAKING CHANGE (package-name): If any breaking changes were done, they need to be listed here.
-BREAKING CHANGE (package-name): Another breaking change if needed. Closes #ZZZ.
+MAJOR BREAKING CHANGE (package-name): If any breaking changes were done, they need to be listed here.
+MINOR BREAKING CHANGE (package-name): Another breaking change if needed. Closes #ZZZ.
 ```
 
 ### Commit types
@@ -43,25 +43,72 @@ BREAKING CHANGE (package-name): Another breaking change if needed. Closes #ZZZ.
 
 Each commit can contain additional notes that will be inserted into the changelog:
 
-* `MAJOR BREAKING CHANGE` (alias: `BREAKING CHANGE`),
+* `MAJOR BREAKING CHANGE`,
 * `MINOR BREAKING CHANGE`.
 
-If any change contains the `MAJOR BREAKING CHANGE` note, the next release will be marked as `major` automatically.
+If any change contains the `MAJOR BREAKING CHANGE` note, the next release will automatically be marked as `major`.
 
 For reference on how to identify minor or major breaking change see the {@link framework/guides/support/versioning-policy versioning policy guide}.
 
-Each `BREAKING CHANGE` note must be followed by the package name.
+Each `MAJOR BREAKING CHANGE` or `MINOR BREAKING CHANGE` note must be followed by the package name.
+
+<info-box>
+	Remember to always specify whether the breaking change is major or minor. If you fail to do so, the system will assume all unspecified breaking changes are major.
+</info-box>
 
 ### Package name
 
 Most commits are related to one or more packages. Each affected package should be listed in parenthesis following the commit type. A package that was the most impacted by the change should be listed first.
 
 It is, however, possible to skip this part if many packages are affected. This usually indicates a generic change. In this case having all the packages listed would reduce the changelog readability.
 
-The package name is based on the npm package name, however, it has the `@ckeditor/ckeditor(5)-` prefix stripped.
+The package name is based on the npm package name, however, it has the `@ckeditor/ckeditor5-` prefix stripped.
 
 If your change is related to the main package only, use `ckeditor5` as the package name.
 
+<info-box>
+	If the commit introduces a breaking change across the entire project (a generic change), the package name does not have to be specified.
+</info-box>
+
+### Order of entries
+
+The proper order of sections for a commit message is as follows:
+* Entries that should be added to the changelog.
+* Entries that will not be added to the changelog.
+* Breaking change notes.
+
+All entries must be separated with a blank line, otherwise the lines will not be treated as separate entries.
+
+### Examples of correct and incorrect message formatting
+
+Example of a proper commit message:
+
+```
+Feature (package-name-1): Message 1.
+
+Fix (package-name-2): Message 2.
+
+Tests: A change across the entire project.
+```
+
+An example of an invalid commit message with incorrectly separated lines (the second line will just be treated as a part of the first line):
+
+```
+Feature (package-name-1): Message 1.
+Fix (package-name-2): Message 2.
+Tests: Message 3.
+```
+
+An example of an invalid commit message with an incorrect section order (the "internal" message will be treated as a part of the breaking change message):
+
+```
+Feature (package-name): Message 1.
+
+MINOR BREAKING CHANGE (package-name): A description.
+
+Internal: Message 2.
+```
+
 ### Example commits
 
 A new feature without any breaking changes.

packages/ckeditor5-engine/src/conversion/downcasthelpers.js

@@ -360,18 +360,19 @@ export default class DowncastHelpers extends ConversionHelpers {
 	 *
 	 *		editor.conversion.for( 'downcast' ).markerToHighlight( {
 	 *			model: 'comment',
-	 *			view: { classes: 'new-comment' },
+	 *			view: { classes: 'comment' },
 	 *			converterPriority: 'high'
 	 *		} );
 	 *
 	 *		editor.conversion.for( 'downcast' ).markerToHighlight( {
 	 *			model: 'comment',
-	 *			view: ( data, converstionApi ) => {
-	 *				// Assuming that the marker name is in a form of comment:commentType.
-	 *				const commentType = data.markerName.split( ':' )[ 1 ];
+	 *			view: ( data, conversionApi ) => {
+	 *				// Assuming that the marker name is in a form of comment:commentType:commentId.
+	 *				const [ , commentType, commentId ] = data.markerName.split( ':' );
 	 *
 	 *				return {
-	 *					classes: [ 'comment', 'comment-' + commentType ]
+	 *					classes: [ 'comment', 'comment-' + commentType ],
+	 *					attributes: { 'data-comment-id': commentId }
 	 *				};
 	 *			}
 	 *		} );

packages/ckeditor5-engine/tests/conversion/downcasthelpers.js

@@ -2673,25 +2673,23 @@ describe( 'DowncastHelpers', () => {
 		it( 'config.view is a function', () => {
 			downcastHelpers.markerToHighlight( {
 				model: 'comment',
-				view: ( data, conversionApi ) => {
-					const commentType = data.markerName.split( ':' )[ 1 ];
-
-					// To ensure conversion API is provided.
-					expect( conversionApi.writer ).to.instanceof( DowncastWriter );
-
+				view: data => {
+					// Assuming that the marker name is in a form of comment:commentType:commentId.
+					const [ , commentType, commentId ] = data.markerName.split( ':' );
 					return {
-						classes: [ 'comment', 'comment-' + commentType ]
+						classes: [ 'comment', 'comment-' + commentType ],
+						attributes: { 'data-comment-id': commentId }
 					};
 				}
 			} );
 
 			model.change( writer => {
 				writer.insertText( 'foo', modelRoot, 0 );
 				const range = writer.createRange( writer.createPositionAt( modelRoot, 0 ), writer.createPositionAt( modelRoot, 3 ) );
-				writer.addMarker( 'comment:abc', { range, usingOperation: false } );
+				writer.addMarker( 'comment:abc:id', { range, usingOperation: false } );
 			} );
 
-			expectResult( '<span class="comment comment-abc">foo</span>' );
+			expectResult( '<span class="comment comment-abc" data-comment-id="id">foo</span>' );
 		} );
 
 		describe( 'highlight', () => {

packages/ckeditor5-highlight/src/highlightui.js

@@ -94,8 +94,11 @@ export default class HighlightUI extends Plugin {
 	 */
 	_addRemoveHighlightButton() {
 		const t = this.editor.t;
+		const command = this.editor.commands.get( 'highlight' );
 
-		this._addButton( 'removeHighlight', t( 'Remove highlight' ), icons.eraser );
+		this._addButton( 'removeHighlight', t( 'Remove highlight' ), icons.eraser, null, button => {
+			button.bind( 'isEnabled' ).to( command, 'isEnabled' );
+		} );
 	}
 
 	/**
@@ -124,10 +127,11 @@ export default class HighlightUI extends Plugin {
 	 * @param {String} name The name of the button.
 	 * @param {String} label The label for the button.
 	 * @param {String} icon The button icon.
-	 * @param {Function} [decorateButton=()=>{}] Additional method for extending the button.
+	 * @param {*} value The `value` property passed to the executed command.
+	 * @param {Function} decorateButton A callback getting ButtonView instance so that it can be further customized.
 	 * @private
 	 */
-	_addButton( name, label, icon, value, decorateButton = () => {} ) {
+	_addButton( name, label, icon, value, decorateButton ) {
 		const editor = this.editor;
 
 		editor.ui.componentFactory.add( name, locale => {

packages/ckeditor5-highlight/tests/highlightui.js

@@ -58,6 +58,7 @@ describe( 'HighlightUI', () => {
 			} )
 			.then( newEditor => {
 				editor = newEditor;
+				command = editor.commands.get( 'highlight' );
 			} );
 	} );
 
@@ -67,11 +68,10 @@ describe( 'HighlightUI', () => {
 		return editor.destroy();
 	} );
 
-	describe( 'highlight Dropdown', () => {
+	describe( 'highlight dropdown', () => {
 		let dropdown;
 
 		beforeEach( () => {
-			command = editor.commands.get( 'highlight' );
 			dropdown = editor.ui.componentFactory.create( 'highlight' );
 		} );
 
@@ -246,4 +246,29 @@ describe( 'HighlightUI', () => {
 			}
 		} );
 	} );
+
+	describe( 'highlight remove', () => {
+		let removeHighlightButton;
+
+		beforeEach( () => {
+			removeHighlightButton = editor.ui.componentFactory.create( 'removeHighlight' );
+		} );
+
+		it( 'removeButton has the base properties', () => {
+			expect( editor.ui.componentFactory.has( 'removeHighlight' ) ).to.be.true;
+			expect( removeHighlightButton ).to.have.property( 'tooltip', true );
+			expect( removeHighlightButton ).to.have.property( 'label', 'Remove highlight' );
+			expect( removeHighlightButton ).to.have.property( 'icon', eraserIcon );
+		} );
+
+		describe( 'model to command binding', () => {
+			it( 'isEnabled', () => {
+				command.isEnabled = false;
+				expect( removeHighlightButton.isEnabled ).to.be.false;
+
+				command.isEnabled = true;
+				expect( removeHighlightButton.isEnabled ).to.be.true;
+			} );
+		} );
+	} );
 } );

packages/ckeditor5-paste-from-office/docs/features/paste-from-word.md

@@ -29,7 +29,7 @@ To test how Paste from Office works, download the [sample Word document](../../a
 {@snippet features/paste-from-office}
 
 ## Related features
- 
+
 CKEditor 5 supports a wider range of paste features, including:
 * {@link features/paste-from-google-docs Paste from Google Docs} – Paste content from Google Docs, maintaining the original formatting and structure.
 * {@link features/paste-plain-text Paste plain text} – Paste text without formatting that will inherit the style of the content it was pasted into.
@@ -64,7 +64,6 @@ ClassicEditor
 	.then( ... )
 	.catch( ... );
 ```
-
 ## Support for other applications
 
 At the current stage, the focus of the `@ckeditor/ckeditor5-paste-from-office` package is on supporting content that comes from Microsoft Word and {@link features/paste-from-google-docs Google Docs}. However, it does not mean that pasting from other similar applications (such as Microsoft Excel) is not supported.
@@ -73,10 +72,18 @@ By default, CKEditor 5 will support pasting rich-text content from these applica
 
 You can find more information regarding compatibility with other applications in [this ticket](https://github.com/ckeditor/ckeditor5/issues/1184#issuecomment-409828069).
 
-If you think that support for any of the applications needs improvements, please add 👍 and comments in the following issues:
+If you think that support for any of the applications needs improvements, please add 👍  and comments in the following issues:
 
 * [Support pasting from Excel](https://github.com/ckeditor/ckeditor5/issues/2513).
 * [Support pasting from Libre Office](https://github.com/ckeditor/ckeditor5/issues/2520).
 * [Support pasting from Pages](https://github.com/ckeditor/ckeditor5/issues/2527).
 
 Feel free to open a [new feature request](https://github.com/ckeditor/ckeditor5/issues/new/choose) for other similar applications, too!
+
+## Known issues
+
+If the pasted document contains both images and styled text (e.g. headings), it may happen that the images are not pasted properly. Unfortunately, for some operating system, browser and Word versions the image data is not available in the clipboard in this case.
+
+It is advised to try and paste the image separately from the body of the text if this error occurs.
+
+If the image is represented in the Word content by the VML syntax (like this one: `<v:shape><v:imagedata src="...."/></v:shape>`), it will not be pasted either as this notation is not supported by CKEditor 5. If you'd like to see this feature implemented, add a 👍&nbsp; reaction to [this GitHub issue](https://github.com/ckeditor/ckeditor5/issues/9245).

packages/ckeditor5-widget/src/utils.js

@@ -124,16 +124,47 @@ export function toWidget( element, writer, options = {} ) {
 		addSelectionHandle( element, writer );
 	}
 
-	setHighlightHandling(
-		element,
-		writer,
-		( element, descriptor, writer ) => writer.addClass( toArray( descriptor.classes ), element ),
-		( element, descriptor, writer ) => writer.removeClass( toArray( descriptor.classes ), element )
-	);
+	setHighlightHandling( element, writer, addHighlight, removeHighlight );
 
 	return element;
 }
 
+// Default handler for adding a highlight on a widget.
+// It adds CSS class and attributes basing on the given highlight descriptor.
+//
+// @param {module:engine/view/element~Element} element
+// @param {module:engine/conversion/downcasthelpers~HighlightDescriptor} descriptor
+// @param {module:engine/view/downcastwriter~DowncastWriter} writer
+function addHighlight( element, descriptor, writer ) {
+	if ( descriptor.classes ) {
+		writer.addClass( toArray( descriptor.classes ), element );
+	}
+
+	if ( descriptor.attributes ) {
+		for ( const key in descriptor.attributes ) {
+			writer.setAttribute( key, descriptor.attributes[ key ], element );
+		}
+	}
+}
+
+// Default handler for removing a highlight from a widget.
+// It removes CSS class and attributes basing on the given highlight descriptor.
+//
+// @param {module:engine/view/element~Element} element
+// @param {module:engine/conversion/downcasthelpers~HighlightDescriptor} descriptor
+// @param {module:engine/view/downcastwriter~DowncastWriter} writer
+function removeHighlight( element, descriptor, writer ) {
+	if ( descriptor.classes ) {
+		writer.removeClass( toArray( descriptor.classes ), element );
+	}
+
+	if ( descriptor.attributes ) {
+		for ( const key in descriptor.attributes ) {
+			writer.removeAttribute( key, element );
+		}
+	}
+}
+
 /**
  * Sets highlight handling methods. Uses {@link module:widget/highlightstack~HighlightStack} to
  * properly determine which highlight descriptor should be used at given time.

packages/ckeditor5-widget/tests/utils.js

@@ -74,7 +74,7 @@ describe( 'widget utils', () => {
 			expect( getLabel( element ) ).to.equal( 'foo bar baz label' );
 		} );
 
-		it( 'should set default highlight handling methods', () => {
+		it( 'should set default highlight handling methods - CSS class', () => {
 			toWidget( element, writer );
 
 			const set = element.getCustomProperty( 'addHighlight' );
@@ -108,6 +108,24 @@ describe( 'widget utils', () => {
 			expect( element.hasClass( 'foo' ) ).to.be.false;
 		} );
 
+		it( 'should set default highlight handling methods - attributes', () => {
+			toWidget( element, writer );
+
+			const set = element.getCustomProperty( 'addHighlight' );
+			const remove = element.getCustomProperty( 'removeHighlight' );
+
+			expect( typeof set ).to.equal( 'function' );
+			expect( typeof remove ).to.equal( 'function' );
+
+			set( element, { priority: 1, attributes: { foo: 'bar', abc: 'xyz' }, id: 'highlight' }, writer );
+			expect( element.getAttribute( 'foo' ) ).to.equal( 'bar' );
+			expect( element.getAttribute( 'abc' ) ).to.equal( 'xyz' );
+
+			remove( element, 'highlight', writer );
+			expect( element.hasAttribute( 'foo' ) ).to.be.false;
+			expect( element.hasAttribute( 'abc' ) ).to.be.false;
+		} );
+
 		it( 'should add element a selection handle to widget if hasSelectionHandle=true is passed', () => {
 			toWidget( element, writer, { hasSelectionHandle: true } );