Merge pull request #15868 from ckeditor/ck/15757

Fix (html-support): Background color style should be properly preserved by GHS while the `FontBackgroundColor` plugin is enabled. It should be able to preserve a partly defined style. Closes #15757. Closes #10399.

packages/ckeditor5-engine/src/conversion/viewconsumable.ts

@@ -7,16 +7,14 @@
  * @module engine/conversion/viewconsumable
  */
 
-import { CKEditorError } from '@ckeditor/ckeditor5-utils';
+import { CKEditorError, toArray, type ArrayOrItem } from '@ckeditor/ckeditor5-utils';
 
 import type Element from '../view/element.js';
 import type Node from '../view/node.js';
 import type Text from '../view/text.js';
 import type DocumentFragment from '../view/documentfragment.js';
 import type { Match } from '../view/matcher.js';
 
-import { isArray } from 'lodash-es';
-
 /**
  * Class used for handling consumption of view {@link module:engine/view/element~Element elements},
  * {@link module:engine/view/text~Text text nodes} and {@link module:engine/view/documentfragment~DocumentFragment document fragments}.
@@ -558,8 +556,8 @@ export class ViewElementConsumables {
 	 * @param type Type of the consumable item: `attributes`, `classes` or `styles`.
 	 * @param item Consumable item or array of items.
 	 */
-	private _add( type: ConsumableType, item: string | Array<string> ) {
-		const items = isArray( item ) ? item : [ item ];
+	private _add( type: ConsumableType, item: ArrayOrItem<string> ) {
+		const items = toArray( item );
 		const consumables = this._consumables[ type ];
 
 		for ( const name of items ) {
@@ -603,8 +601,8 @@ export class ViewElementConsumables {
 	 * @returns Returns `true` if all items can be consumed, `null` when one of the items cannot be
 	 * consumed and `false` when one of the items is already consumed.
 	 */
-	private _test( type: ConsumableType, item: string | Array<string> ): boolean | null {
-		const items = isArray( item ) ? item : [ item ];
+	private _test( type: ConsumableType, item: ArrayOrItem<string> ): boolean | null {
+		const items = toArray( item );
 		const consumables = this._consumables[ type ];
 
 		for ( const name of items ) {
@@ -639,8 +637,8 @@ export class ViewElementConsumables {
 	 * @param type Type of the consumable item: `attributes`, `classes` or `styles`.
 	 * @param item Consumable item or array of items.
 	 */
-	private _consume( type: ConsumableType, item: string | Array<string> ) {
-		const items = isArray( item ) ? item : [ item ];
+	private _consume( type: ConsumableType, item: ArrayOrItem<string> ) {
+		const items = toArray( item );
 		const consumables = this._consumables[ type ];
 
 		for ( const name of items ) {
@@ -667,8 +665,8 @@ export class ViewElementConsumables {
 	 * @param type Type of the consumable item: `attributes`, `classes` or , `styles`.
 	 * @param item Consumable item or array of items.
 	 */
-	private _revert( type: ConsumableType, item: string | Array<string> ) {
-		const items = isArray( item ) ? item : [ item ];
+	private _revert( type: ConsumableType, item: ArrayOrItem<string> ) {
+		const items = toArray( item );
 		const consumables = this._consumables[ type ];
 
 		for ( const name of items ) {

packages/ckeditor5-engine/src/index.ts

@@ -191,7 +191,7 @@ export type { ViewDocumentSelectionChangeEvent } from './view/observer/selection
 export type { ViewRenderEvent, ViewScrollToTheSelectionEvent } from './view/view.js';
 
 // View / Styles.
-export { StylesProcessor, type BoxSides } from './view/stylesmap.js';
+export { default as StylesMap, StylesProcessor, type BoxSides } from './view/stylesmap.js';
 export * from './view/styles/background.js';
 export * from './view/styles/border.js';
 export * from './view/styles/margin.js';

packages/ckeditor5-engine/src/view/styles/padding.ts

@@ -11,7 +11,7 @@ import type { StylesProcessor } from '../stylesmap.js';
 import { getPositionShorthandNormalizer, getBoxSidesValueReducer } from './utils.js';
 
 /**
- * Adds a margin CSS styles processing rules.
+ * Adds a padding CSS styles processing rules.
  *
  * ```ts
  * editor.data.addStyleProcessorRules( addPaddingRules );

packages/ckeditor5-engine/src/view/stylesmap.ts

@@ -11,8 +11,6 @@ import { get, isObject, merge, set, unset } from 'lodash-es';
 
 /**
  * Styles map. Allows handling (adding, removing, retrieving) a set of style rules (usually, of an element).
- *
- * The styles map is capable of normalizing style names so e.g. the following operations are possible:
  */
 export default class StylesMap {
 	/**

packages/ckeditor5-html-support/src/datafilter.ts

@@ -11,10 +11,10 @@ import { Plugin, type Editor } from 'ckeditor5/src/core.js';
 
 import {
 	Matcher,
+	StylesMap,
 	type MatcherPattern,
 	type UpcastConversionApi,
 	type ViewElement,
-	type MatchResult,
 	type ViewConsumable,
 	type MatcherObjectPattern,
 	type DocumentSelectionChangeAttributeEvent
@@ -53,7 +53,7 @@ import {
 	type GHSViewAttributes
 } from './utils.js';
 
-import { isPlainObject, pull as removeItemFromArray } from 'lodash-es';
+import { isPlainObject } from 'lodash-es';
 
 import '../theme/datafilter.css';
 
@@ -311,11 +311,13 @@ export default class DataFilter extends Plugin {
 	 * - classes Set with matched class names.
 	 */
 	public processViewAttributes( viewElement: ViewElement, conversionApi: UpcastConversionApi ): GHSViewAttributes | null {
+		const { consumable } = conversionApi;
+
 		// Make sure that the disabled attributes are handled before the allowed attributes are called.
 		// For example, for block images the <figure> converter triggers conversion for <img> first and then for other elements, i.e. <a>.
-		consumeAttributes( viewElement, conversionApi, this._disallowedAttributes );
+		matchAndConsumeAttributes( viewElement, this._disallowedAttributes, consumable );
 
-		return consumeAttributes( viewElement, conversionApi, this._allowedAttributes );
+		return prepareGHSAttribute( viewElement, matchAndConsumeAttributes( viewElement, this._allowedAttributes, consumable ) );
 	}
 
 	/**
@@ -805,129 +807,133 @@ export interface DataFilterRegisterEvent {
 }
 
 /**
- * Matches and consumes the given view attributes.
+ * Matches and consumes matched attributes.
+ *
+ * @returns Object with following properties:
+ * - attributes Array with matched attribute names.
+ * - classes Array with matched class names.
+ * - styles Array with matched style names.
  */
-function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConversionApi, matcher: Matcher ) {
-	const matches = consumeAttributeMatches( viewElement, conversionApi, matcher );
-	const { attributes, styles, classes } = mergeMatchResults( matches );
-	const viewAttributes: GHSViewAttributes = {};
-
-	// Remove invalid DOM element attributes.
-	if ( attributes.size ) {
-		for ( const key of attributes ) {
-			if ( !isValidAttributeName( key as string ) ) {
-				attributes.delete( key );
+function matchAndConsumeAttributes(
+	viewElement: ViewElement,
+	matcher: Matcher,
+	consumable: ViewConsumable
+): {
+	attributes: Array<string>;
+	classes: Array<string>;
+	styles: Array<string>;
+} {
+	const matches = matcher.matchAll( viewElement ) || [];
+	const stylesProcessor = viewElement.document.stylesProcessor;
+
+	return matches.reduce( ( result, { match } ) => {
+		// Verify and consume styles.
+		for ( const style of match.styles || [] ) {
+			// Check longer forms of the same style as those could be matched
+			// but not present in the element directly.
+			// Consider only longhand (or longer than current notation) so that
+			// we do not include all sides of the box if only one side is allowed.
+			const sortedRelatedStyles = stylesProcessor.getRelatedStyles( style )
+				.filter( relatedStyle => relatedStyle.split( '-' ).length > style.split( '-' ).length )
+				.sort( ( a, b ) => b.split( '-' ).length - a.split( '-' ).length );
+
+			for ( const relatedStyle of sortedRelatedStyles ) {
+				if ( consumable.consume( viewElement, { styles: [ relatedStyle ] } ) ) {
+					result.styles.push( relatedStyle );
+				}
 			}
-		}
-	}
 
-	if ( attributes.size ) {
-		viewAttributes.attributes = iterableToObject( attributes, key => viewElement.getAttribute( key ) );
-	}
-
-	if ( styles.size ) {
-		viewAttributes.styles = iterableToObject( styles, key => viewElement.getStyle( key ) );
-	}
+			// Verify and consume style as specified in the matcher.
+			if ( consumable.consume( viewElement, { styles: [ style ] } ) ) {
+				result.styles.push( style );
+			}
+		}
 
-	if ( classes.size ) {
-		viewAttributes.classes = Array.from( classes );
-	}
+		// Verify and consume class names.
+		for ( const className of match.classes || [] ) {
+			if ( consumable.consume( viewElement, { classes: [ className ] } ) ) {
+				result.classes.push( className );
+			}
+		}
 
-	if ( !Object.keys( viewAttributes ).length ) {
-		return null;
-	}
+		// Verify and consume other attributes.
+		for ( const attributeName of match.attributes || [] ) {
+			if ( consumable.consume( viewElement, { attributes: [ attributeName ] } ) ) {
+				result.attributes.push( attributeName );
+			}
+		}
 
-	return viewAttributes;
+		return result;
+	}, {
+		attributes: [] as Array<string>,
+		classes: [] as Array<string>,
+		styles: [] as Array<string>
+	} );
 }
 
 /**
- * Consumes matched attributes.
- *
- * @returns Array with match information about found attributes.
+ * Prepares the GHS attribute value as an object with element attributes' values.
  */
-function consumeAttributeMatches( viewElement: ViewElement, { consumable }: UpcastConversionApi, matcher: Matcher ): Array<MatchResult> {
-	const matches = matcher.matchAll( viewElement ) || [];
-	const consumedMatches = [];
-
-	for ( const match of matches ) {
-		removeConsumedAttributes( consumable, viewElement, match );
-
-		// We only want to consume attributes, so element can be still processed by other converters.
-		delete match.match.name;
-
-		consumable.consume( viewElement, match.match );
-		consumedMatches.push( match );
+function prepareGHSAttribute(
+	viewElement: ViewElement,
+	{ attributes, classes, styles }: {
+		attributes: Array<string>;
+		classes: Array<string>;
+		styles: Array<string>;
+	}
+): GHSViewAttributes | null {
+	if ( !attributes.length && !classes.length && !styles.length ) {
+		return null;
 	}
 
-	return consumedMatches;
-}
+	return {
+		...( attributes.length && {
+			attributes: getAttributes( viewElement, attributes )
+		} ),
 
-/**
- * Removes attributes from the given match that were already consumed by other converters.
- */
-function removeConsumedAttributes( consumable: ViewConsumable, viewElement: ViewElement, match: MatchResult ) {
-	for ( const key of [ 'attributes', 'classes', 'styles' ] as const ) {
-		const attributes = match.match[ key ];
-
-		if ( !attributes ) {
-			continue;
-		}
+		...( styles.length && {
+			styles: getReducedStyles( viewElement, styles )
+		} ),
 
-		// Iterating over a copy of an array so removing items doesn't influence iteration.
-		for ( const value of Array.from( attributes ) ) {
-			if ( !consumable.test( viewElement, ( { [ key ]: [ value ] } ) ) ) {
-				removeItemFromArray( attributes, value );
-			}
-		}
-	}
+		...( classes.length && {
+			classes
+		} )
+	};
 }
 
 /**
- * Merges the result of {@link module:engine/view/matcher~Matcher#matchAll} method.
- *
- * @param matches
- * @returns Object with following properties:
- * - attributes Set with matched attribute names.
- * - styles Set with matched style names.
- * - classes Set with matched class names.
+ * Returns attributes as an object with names and values.
  */
-function mergeMatchResults( matches: Array<MatchResult> ):
-{
-	attributes: Set<string>;
-	styles: Set<string>;
-	classes: Set<string>;
-} {
-	const matchResult = {
-		attributes: new Set<string>(),
-		classes: new Set<string>(),
-		styles: new Set<string>()
-	};
+function getAttributes( viewElement: ViewElement, attributes: Iterable<string> ): Record<string, string> {
+	const attributesObject: Record<string, string> = {};
 
-	for ( const match of matches ) {
-		for ( const key in matchResult ) {
-			const values: Array<string> = match.match[ key as keyof typeof matchResult ] || [];
+	for ( const key of attributes ) {
+		const value = viewElement.getAttribute( key );
 
-			values.forEach( value => ( matchResult[ key as keyof typeof matchResult ] ).add( value ) );
+		if ( value !== undefined && isValidAttributeName( key ) ) {
+			attributesObject[ key ] = value;
 		}
 	}
 
-	return matchResult;
+	return attributesObject;
 }
 
 /**
- * Converts the given iterable object into an object.
+ * Returns styles as an object reduced to shorthand notation without redundant entries.
  */
-function iterableToObject( iterable: Set<string>, getValue: ( s: string ) => any ) {
-	const attributesObject: Record<string, any> = {};
+function getReducedStyles( viewElement: ViewElement, styles: Iterable<string> ): Record<string, string> {
+	// Use StyleMap to reduce style value to the minimal form (without shorthand and long-hand notation and duplication).
+	const stylesMap = new StylesMap( viewElement.document.stylesProcessor );
+
+	for ( const key of styles ) {
+		const styleValue = viewElement.getStyle( key );
 
-	for ( const prop of iterable ) {
-		const value = getValue( prop );
-		if ( value !== undefined ) {
-			attributesObject[ prop ] = getValue( prop );
+		if ( styleValue !== undefined ) {
+			stylesMap.set( key, styleValue );
 		}
 	}
 
-	return attributesObject;
+	return Object.fromEntries( stylesMap.getStylesEntries() );
 }
 
 /**
@@ -942,8 +948,8 @@ function splitPattern( pattern: MatcherObjectPattern, attributeName: 'attributes
 	const attributeValue = pattern[ attributeName ];
 
 	if ( isPlainObject( attributeValue ) ) {
-		return Object.entries( attributeValue as Record<string, unknown> ).map(
-			( [ key, value ] ) => ( {
+		return Object.entries( attributeValue as Record<string, unknown> )
+			.map( ( [ key, value ] ) => ( {
 				name,
 				[ attributeName ]: {
 					[ key ]: value
@@ -952,12 +958,11 @@ function splitPattern( pattern: MatcherObjectPattern, attributeName: 'attributes
 	}
 
 	if ( Array.isArray( attributeValue ) ) {
-		return attributeValue.map(
-			value => ( {
+		return attributeValue
+			.map( value => ( {
 				name,
 				[ attributeName ]: [ value ]
-			} )
-		);
+			} ) );
 	}
 
 	return [ pattern ];