From 11a71be5a459e2fbd60463e0c691258de9e169e6 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 15 Feb 2024 16:13:54 +0100 Subject: [PATCH 01/13] GHS should collect styles using StylesMap. --- packages/ckeditor5-engine/src/index.ts | 2 +- .../src/view/styles/padding.ts | 2 +- .../ckeditor5-html-support/src/datafilter.ts | 58 ++++++-- .../tests/datafilter.js | 132 +++++++++++++++++- 4 files changed, 170 insertions(+), 24 deletions(-) diff --git a/packages/ckeditor5-engine/src/index.ts b/packages/ckeditor5-engine/src/index.ts index 109bc0f1372..33215db0623 100644 --- a/packages/ckeditor5-engine/src/index.ts +++ b/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'; diff --git a/packages/ckeditor5-engine/src/view/styles/padding.ts b/packages/ckeditor5-engine/src/view/styles/padding.ts index ce24cd2b475..667092ebb77 100644 --- a/packages/ckeditor5-engine/src/view/styles/padding.ts +++ b/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 ); diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index 0b4b587b567..71dcb2e396a 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -11,10 +11,11 @@ import { Plugin, type Editor } from 'ckeditor5/src/core.js'; import { Matcher, + StylesMap, type MatcherPattern, + type Match, type UpcastConversionApi, type ViewElement, - type MatchResult, type ViewConsumable, type MatcherObjectPattern, type DocumentSelectionChangeAttributeEvent @@ -808,6 +809,7 @@ export interface DataFilterRegisterEvent { * Matches and consumes the given view attributes. */ function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConversionApi, matcher: Matcher ) { + const stylesProcessor = viewElement.document.stylesProcessor; const matches = consumeAttributeMatches( viewElement, conversionApi, matcher ); const { attributes, styles, classes } = mergeMatchResults( matches ); const viewAttributes: GHSViewAttributes = {}; @@ -825,14 +827,39 @@ function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConve viewAttributes.attributes = iterableToObject( attributes, key => viewElement.getAttribute( key ) ); } - if ( styles.size ) { - viewAttributes.styles = iterableToObject( styles, key => viewElement.getStyle( key ) ); - } - if ( classes.size ) { viewAttributes.classes = Array.from( classes ); } + if ( styles.size ) { + // Gather all consumed styles. + const stylesMap = new StylesMap( stylesProcessor ); + + for ( const key of styles ) { + const styleValue = viewElement.getStyle( key ); + + if ( styleValue !== undefined ) { + stylesMap.set( key, styleValue ); + + continue; + } + + // Find other longhand styles that were consumed but can not be returned for shorthand name. + for ( const relatedKey of stylesProcessor.getRelatedStyles( key ) ) { + const relatedValue = viewElement.getStyle( relatedKey ); + + // Set all styles on the StyleMap, so later we can get the minimal set of styles + // (without duplication of multiple shorthand notations). + if ( relatedValue !== undefined ) { + stylesMap.set( relatedKey, relatedValue ); + } + } + } + + // Output styles in the reduced form. + viewAttributes.styles = Object.fromEntries( stylesMap.getStylesEntries() ); + } + if ( !Object.keys( viewAttributes ).length ) { return null; } @@ -845,17 +872,17 @@ function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConve * * @returns Array with match information about found attributes. */ -function consumeAttributeMatches( viewElement: ViewElement, { consumable }: UpcastConversionApi, matcher: Matcher ): Array { +function consumeAttributeMatches( viewElement: ViewElement, { consumable }: UpcastConversionApi, matcher: Matcher ): Array { const matches = matcher.matchAll( viewElement ) || []; - const consumedMatches = []; + const consumedMatches: Array = []; - for ( const match of matches ) { + 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; + delete match.name; - consumable.consume( viewElement, match.match ); + consumable.consume( viewElement, match ); consumedMatches.push( match ); } @@ -865,9 +892,9 @@ function consumeAttributeMatches( viewElement: ViewElement, { consumable }: Upca /** * Removes attributes from the given match that were already consumed by other converters. */ -function removeConsumedAttributes( consumable: ViewConsumable, viewElement: ViewElement, match: MatchResult ) { +function removeConsumedAttributes( consumable: ViewConsumable, viewElement: ViewElement, match: Match ) { for ( const key of [ 'attributes', 'classes', 'styles' ] as const ) { - const attributes = match.match[ key ]; + const attributes = match[ key ]; if ( !attributes ) { continue; @@ -891,7 +918,7 @@ function removeConsumedAttributes( consumable: ViewConsumable, viewElement: View * - styles Set with matched style names. * - classes Set with matched class names. */ -function mergeMatchResults( matches: Array ): +function mergeMatchResults( matches: Array ): { attributes: Set; styles: Set; @@ -905,7 +932,7 @@ function mergeMatchResults( matches: Array ): for ( const match of matches ) { for ( const key in matchResult ) { - const values: Array = match.match[ key as keyof typeof matchResult ] || []; + const values: Array = match[ key as keyof typeof matchResult ] || []; values.forEach( value => ( matchResult[ key as keyof typeof matchResult ] ).add( value ) ); } @@ -922,8 +949,9 @@ function iterableToObject( iterable: Set, getValue: ( s: string ) => any for ( const prop of iterable ) { const value = getValue( prop ); + if ( value !== undefined ) { - attributesObject[ prop ] = getValue( prop ); + attributesObject[ prop ] = value; } } diff --git a/packages/ckeditor5-html-support/tests/datafilter.js b/packages/ckeditor5-html-support/tests/datafilter.js index 4e73be87cd1..38248aef647 100644 --- a/packages/ckeditor5-html-support/tests/datafilter.js +++ b/packages/ckeditor5-html-support/tests/datafilter.js @@ -16,7 +16,7 @@ import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_uti import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model.js'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view.js'; import { getModelDataWithAttributes } from './_utils/utils.js'; -import { addBackgroundRules } from '@ckeditor/ckeditor5-engine/src/view/styles/background.js'; +import { addBackgroundRules, addBorderRules, addMarginRules, addPaddingRules } from '@ckeditor/ckeditor5-engine'; import { getLabel } from '@ckeditor/ckeditor5-widget/src/utils.js'; import GeneralHtmlSupport from '../src/generalhtmlsupport.js'; @@ -3925,15 +3925,133 @@ describe( 'DataFilter', () => { }, /data-filter-invalid-definition/, null, definition ); } ); - it( 'should handle expanded styles by matcher', () => { - editor.data.addStyleProcessorRules( addBackgroundRules ); + describe( 'expanded styles (shorthand vs longhand notation)', () => { + it( 'should handle expanded styles by matcher', () => { + editor.data.addStyleProcessorRules( addBackgroundRules ); - dataFilter.allowElement( 'p' ); - dataFilter.allowAttributes( { name: 'p', styles: true } ); + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: true } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle longhand style for shorthand filter (background vs background-color)', () => { + editor.data.addStyleProcessorRules( addBackgroundRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'background' } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle partial padding for generic padding filter (single box side)', () => { + editor.data.addStyleProcessorRules( addPaddingRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'padding' } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle partial padding for generic padding filter (multiple sides)', () => { + editor.data.addStyleProcessorRules( addPaddingRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'padding' } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle partial padding for generic padding filter (box top side)', () => { + editor.data.addStyleProcessorRules( addPaddingRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'padding' } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle partial border for generic border filter (box bottom side)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border' } ); - editor.setData( '

foobar

' ); + editor.setData( '

foobar

' ); - expect( editor.getData() ).to.equal( '

foobar

' ); + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle partial border for generic border filter (mixed)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border' } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + // All related style properties are consumed and we can not handle other properties. + it.skip( 'should handle partial border for generic border filter (partly consumed)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border' } ); + + editor.conversion.for( 'upcast' ).add( dispatcher => { + dispatcher.on( 'element:p', ( evt, data, { consumable } ) => { + consumable.consume( data.viewItem, { styles: [ 'border-left-width' ] } ); + } ); + } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + // All related style properties are consumed and we can not handle other properties. + it.skip( 'should handle partial margin consumed for generic margin filter', () => { + editor.data.addStyleProcessorRules( addMarginRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'margin' } ); + + editor.model.schema.extend( 'paragraph', { allowAttributes: 'indent' } ); + + editor.conversion.for( 'upcast' ).attributeToAttribute( { + view: { + styles: { 'margin-left': /./ } + }, + model: { + key: 'indent', + value: viewElement => `${ parseInt( viewElement.getStyle( 'margin-left' ) ) * 2 }px` + } + } ); + + editor.conversion.for( 'downcast' ).attributeToAttribute( { + model: 'indent', + view: value => ( { + key: 'style', + value: { 'margin-left': value } + } ) + } ); + + editor.setData( '

foobar

' ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); } ); describe( 'attribute coupling', () => { From 7955eef040176deea4562a9e0a0d141af1a8c0b6 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sat, 17 Feb 2024 15:50:28 +0100 Subject: [PATCH 02/13] Fixed cases when box side style was partly consumed. --- .../ckeditor5-html-support/src/datafilter.ts | 23 ++- .../tests/datafilter.js | 150 +++++++++++++++++- 2 files changed, 154 insertions(+), 19 deletions(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index 71dcb2e396a..c2e0197ff71 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -832,7 +832,7 @@ function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConve } if ( styles.size ) { - // Gather all consumed styles. + // Use StyleMap to reduce style value to the minimal form (without shorthand and long-hand notation and duplication). const stylesMap = new StylesMap( stylesProcessor ); for ( const key of styles ) { @@ -840,19 +840,6 @@ function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConve if ( styleValue !== undefined ) { stylesMap.set( key, styleValue ); - - continue; - } - - // Find other longhand styles that were consumed but can not be returned for shorthand name. - for ( const relatedKey of stylesProcessor.getRelatedStyles( key ) ) { - const relatedValue = viewElement.getStyle( relatedKey ); - - // Set all styles on the StyleMap, so later we can get the minimal set of styles - // (without duplication of multiple shorthand notations). - if ( relatedValue !== undefined ) { - stylesMap.set( relatedKey, relatedValue ); - } } } @@ -877,6 +864,14 @@ function consumeAttributeMatches( viewElement: ViewElement, { consumable }: Upca const consumedMatches: Array = []; for ( const { match } of matches ) { + // Inject other forms of the same style as those could be matched but not present in the element directly. + if ( match.styles ) { + // Iterating over a copy of an array so adding items doesn't influence iteration. + for ( const style of Array.from( match.styles ) ) { + match.styles.push( ...viewElement.document.stylesProcessor.getRelatedStyles( style ) ); + } + } + removeConsumedAttributes( consumable, viewElement, match ); // We only want to consume attributes, so element can be still processed by other converters. diff --git a/packages/ckeditor5-html-support/tests/datafilter.js b/packages/ckeditor5-html-support/tests/datafilter.js index 38248aef647..21e7653dbba 100644 --- a/packages/ckeditor5-html-support/tests/datafilter.js +++ b/packages/ckeditor5-html-support/tests/datafilter.js @@ -3934,6 +3934,17 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'background-color': 'red' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); @@ -3945,6 +3956,17 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'background-color': 'red' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); @@ -3956,6 +3978,17 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'padding-left': '10px' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); @@ -3967,6 +4000,18 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'padding-bottom': '20px', + 'padding-left': '10px' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); @@ -3978,6 +4023,17 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'padding-top': '10px' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); @@ -3989,6 +4045,17 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-bottom': '3px dotted red' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); @@ -4000,11 +4067,23 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-bottom-width': '3px', + 'border-color': 'red', + 'border-style': 'dotted' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); - // All related style properties are consumed and we can not handle other properties. - it.skip( 'should handle partial border for generic border filter (partly consumed)', () => { + it( 'should handle partial border for generic border filter (partly consumed)', () => { editor.data.addStyleProcessorRules( addBorderRules ); dataFilter.allowElement( 'p' ); @@ -4018,11 +4097,33 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); - expect( editor.getData() ).to.equal( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-color': 'red', + 'border-style': 'dotted', + 'border-bottom-width': '3px', + 'border-right-width': '3px', + 'border-top-width': '3px' + } + } + } + } ); + + expect( editor.getData() ).to.equal( + '

foobar

' + ); } ); - // All related style properties are consumed and we can not handle other properties. - it.skip( 'should handle partial margin consumed for generic margin filter', () => { + it( 'should handle partial margin consumed for generic margin filter', () => { editor.data.addStyleProcessorRules( addMarginRules ); dataFilter.allowElement( 'p' ); @@ -4050,8 +4151,47 @@ describe( 'DataFilter', () => { editor.setData( '

foobar

' ); + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'margin-top': '20px', + 'margin-right': '20px', + 'margin-bottom': '20px' + } + } + } + } ); + expect( editor.getData() ).to.equal( '

foobar

' ); } ); + + it( 'should store only reduced styles in model attribute (without duplicated long and shorthand)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: [ 'border' ] } ); + + editor.setData( + '

foobar

' + ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-bottom': '2px solid red' + } + } + } + } ); + + expect( editor.getData() ).to.equal( + '

foobar

' + ); + } ); } ); describe( 'attribute coupling', () => { From a29d72fcdc71b1ca5f1407934da7f98a05b14901 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Sat, 17 Feb 2024 16:31:46 +0100 Subject: [PATCH 03/13] Fixed consuming. --- .../ckeditor5-html-support/src/datafilter.ts | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index c2e0197ff71..d81f12eb077 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -861,27 +861,36 @@ function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConve */ function consumeAttributeMatches( viewElement: ViewElement, { consumable }: UpcastConversionApi, matcher: Matcher ): Array { const matches = matcher.matchAll( viewElement ) || []; - const consumedMatches: Array = []; + const stylesProcessor = viewElement.document.stylesProcessor; - for ( const { match } of matches ) { + return matches.reduce( ( consumedMatches, { match } ) => { // Inject other forms of the same style as those could be matched but not present in the element directly. if ( match.styles ) { - // Iterating over a copy of an array so adding items doesn't influence iteration. - for ( const style of Array.from( match.styles ) ) { - match.styles.push( ...viewElement.document.stylesProcessor.getRelatedStyles( style ) ); + for ( const style of match.styles ) { + for ( const relatedStyle of stylesProcessor.getRelatedStyles( style ) ) { + consumedMatches.push( consumeAttributeMatch( viewElement, consumable, { + styles: [ relatedStyle ] + } ) ); + } } } - removeConsumedAttributes( consumable, viewElement, match ); + return consumedMatches.concat( consumeAttributeMatch( viewElement, consumable, match ) ); + }, [] as Array ); +} - // We only want to consume attributes, so element can be still processed by other converters. - delete match.name; +/** + * TODO + */ +function consumeAttributeMatch( viewElement: ViewElement, consumable: ViewConsumable, match: Match ): Match { + removeConsumedAttributes( consumable, viewElement, match ); - consumable.consume( viewElement, match ); - consumedMatches.push( match ); - } + // We only want to consume attributes, so element can be still processed by other converters. + delete match.name; + + consumable.consume( viewElement, match ); - return consumedMatches; + return match; } /** From 422b0aad2da6320bcc0ef84d1c7ad0b85905b89f Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 19 Feb 2024 14:13:17 +0100 Subject: [PATCH 04/13] Code refactoring. --- .../ckeditor5-html-support/src/datafilter.ts | 122 ++++++++++-------- 1 file changed, 65 insertions(+), 57 deletions(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index d81f12eb077..d7bcb2bf998 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -312,11 +312,16 @@ 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
converter triggers conversion for first and then for other elements, i.e. . - consumeAttributes( viewElement, conversionApi, this._disallowedAttributes ); + matchAndConsumeAttributes( viewElement, this._disallowedAttributes, consumable ); + + const matches = matchAndConsumeAttributes( viewElement, this._allowedAttributes, consumable ); + const mergedMatches = mergeMatchResults( matches ); - return consumeAttributes( viewElement, conversionApi, this._allowedAttributes ); + return normalizeGHSAttribute( viewElement, mergedMatches ); } /** @@ -806,52 +811,33 @@ export interface DataFilterRegisterEvent { } /** - * Matches and consumes the given view attributes. + * TODO */ -function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConversionApi, matcher: Matcher ) { - const stylesProcessor = viewElement.document.stylesProcessor; - 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 ); - } - } - } - - if ( attributes.size ) { - viewAttributes.attributes = iterableToObject( attributes, key => viewElement.getAttribute( key ) ); +function normalizeGHSAttribute( + viewElement: ViewElement, + { attributes, classes, styles }: { + attributes: Set; + classes: Set; + styles: Set; } - - if ( classes.size ) { - viewAttributes.classes = Array.from( classes ); +): GHSViewAttributes | null { + if ( !attributes.size && !classes.size && !styles.size ) { + return null; } - if ( styles.size ) { - // Use StyleMap to reduce style value to the minimal form (without shorthand and long-hand notation and duplication). - const stylesMap = new StylesMap( stylesProcessor ); - - for ( const key of styles ) { - const styleValue = viewElement.getStyle( key ); - - if ( styleValue !== undefined ) { - stylesMap.set( key, styleValue ); - } - } - - // Output styles in the reduced form. - viewAttributes.styles = Object.fromEntries( stylesMap.getStylesEntries() ); - } + return { + ...( attributes.size && { + attributes: normalizeAttributes( viewElement, attributes ) + } ), - if ( !Object.keys( viewAttributes ).length ) { - return null; - } + ...( styles.size && { + styles: normalizeStyles( viewElement, styles ) + } ), - return viewAttributes; + ...( classes.size && { + classes: Array.from( classes ) + } ) + }; } /** @@ -859,7 +845,11 @@ function consumeAttributes( viewElement: ViewElement, conversionApi: UpcastConve * * @returns Array with match information about found attributes. */ -function consumeAttributeMatches( viewElement: ViewElement, { consumable }: UpcastConversionApi, matcher: Matcher ): Array { +function matchAndConsumeAttributes( + viewElement: ViewElement, + matcher: Matcher, + consumable: ViewConsumable +): Array { const matches = matcher.matchAll( viewElement ) || []; const stylesProcessor = viewElement.document.stylesProcessor; @@ -922,12 +912,12 @@ function removeConsumedAttributes( consumable: ViewConsumable, viewElement: View * - styles Set with matched style names. * - classes Set with matched class names. */ -function mergeMatchResults( matches: Array ): -{ +function mergeMatchResults( matches: Array ): { attributes: Set; - styles: Set; classes: Set; + styles: Set; } { + const keys = [ 'attributes', 'classes', 'styles' ] as const; const matchResult = { attributes: new Set(), classes: new Set(), @@ -935,10 +925,10 @@ function mergeMatchResults( matches: Array ): }; for ( const match of matches ) { - for ( const key in matchResult ) { - const values: Array = match[ key as keyof typeof matchResult ] || []; - - values.forEach( value => ( matchResult[ key as keyof typeof matchResult ] ).add( value ) ); + for ( const key of keys ) { + for ( const value of match[ key ] || [] ) { + matchResult[ key ].add( value ); + } } } @@ -946,22 +936,40 @@ function mergeMatchResults( matches: Array ): } /** - * Converts the given iterable object into an object. + * Returns list of attributes as a normalized object. */ -function iterableToObject( iterable: Set, getValue: ( s: string ) => any ) { - const attributesObject: Record = {}; +function normalizeAttributes( viewElement: ViewElement, attributes: Iterable ): Record { + const attributesObject: Record = {}; - for ( const prop of iterable ) { - const value = getValue( prop ); + for ( const key of attributes ) { + const value = viewElement.getAttribute( key ); - if ( value !== undefined ) { - attributesObject[ prop ] = value; + if ( value !== undefined && isValidAttributeName( key ) ) { + attributesObject[ key ] = value; } } return attributesObject; } +/** + * Returns list of styles as a normalized object (without redundant entries). + */ +function normalizeStyles( viewElement: ViewElement, styles: Iterable ): Record { + // 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 ); + + if ( styleValue !== undefined ) { + stylesMap.set( key, styleValue ); + } + } + + return Object.fromEntries( stylesMap.getStylesEntries() ); +} + /** * Matcher by default has to match **all** patterns to count it as an actual match. Splitting the pattern * into separate patterns means that any matched pattern will be count as a match. From 8a05e086e09558587e1c958a802358bb925af465 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 19 Feb 2024 15:52:03 +0100 Subject: [PATCH 05/13] Code refactoring. --- .../src/conversion/viewconsumable.ts | 20 +-- .../ckeditor5-html-support/src/datafilter.ts | 168 +++++++----------- 2 files changed, 69 insertions(+), 119 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/viewconsumable.ts b/packages/ckeditor5-engine/src/conversion/viewconsumable.ts index b4aab91b8ec..91436e98d09 100644 --- a/packages/ckeditor5-engine/src/conversion/viewconsumable.ts +++ b/packages/ckeditor5-engine/src/conversion/viewconsumable.ts @@ -7,7 +7,7 @@ * @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'; @@ -15,8 +15,6 @@ 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 ) { - const items = isArray( item ) ? item : [ item ]; + private _add( type: ConsumableType, item: ArrayOrItem ) { + 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 ): boolean | null { - const items = isArray( item ) ? item : [ item ]; + private _test( type: ConsumableType, item: ArrayOrItem ): 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 ) { - const items = isArray( item ) ? item : [ item ]; + private _consume( type: ConsumableType, item: ArrayOrItem ) { + 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 ) { - const items = isArray( item ) ? item : [ item ]; + private _revert( type: ConsumableType, item: ArrayOrItem ) { + const items = toArray( item ); const consumables = this._consumables[ type ]; for ( const name of items ) { diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index d7bcb2bf998..f8aacbf25c7 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -13,7 +13,6 @@ import { Matcher, StylesMap, type MatcherPattern, - type Match, type UpcastConversionApi, type ViewElement, type ViewConsumable, @@ -318,10 +317,7 @@ export default class DataFilter extends Plugin { // For example, for block images the
converter triggers conversion for first and then for other elements, i.e. . matchAndConsumeAttributes( viewElement, this._disallowedAttributes, consumable ); - const matches = matchAndConsumeAttributes( viewElement, this._allowedAttributes, consumable ); - const mergedMatches = mergeMatchResults( matches ); - - return normalizeGHSAttribute( viewElement, mergedMatches ); + return normalizeGHSAttribute( viewElement, matchAndConsumeAttributes( viewElement, this._allowedAttributes, consumable ) ); } /** @@ -810,36 +806,6 @@ export interface DataFilterRegisterEvent { args: [ data: DataSchemaDefinition ]; } -/** - * TODO - */ -function normalizeGHSAttribute( - viewElement: ViewElement, - { attributes, classes, styles }: { - attributes: Set; - classes: Set; - styles: Set; - } -): GHSViewAttributes | null { - if ( !attributes.size && !classes.size && !styles.size ) { - return null; - } - - return { - ...( attributes.size && { - attributes: normalizeAttributes( viewElement, attributes ) - } ), - - ...( styles.size && { - styles: normalizeStyles( viewElement, styles ) - } ), - - ...( classes.size && { - classes: Array.from( classes ) - } ) - }; -} - /** * Consumes matched attributes. * @@ -849,90 +815,77 @@ function matchAndConsumeAttributes( viewElement: ViewElement, matcher: Matcher, consumable: ViewConsumable -): Array { +): { + attributes: Array; + classes: Array; + styles: Array; +} { const matches = matcher.matchAll( viewElement ) || []; const stylesProcessor = viewElement.document.stylesProcessor; - return matches.reduce( ( consumedMatches, { match } ) => { - // Inject other forms of the same style as those could be matched but not present in the element directly. - if ( match.styles ) { - for ( const style of match.styles ) { - for ( const relatedStyle of stylesProcessor.getRelatedStyles( style ) ) { - consumedMatches.push( consumeAttributeMatch( viewElement, consumable, { - styles: [ relatedStyle ] - } ) ); + return matches.reduce( ( result, { match } ) => { + for ( const style of match.styles || [] ) { + // Check other forms of the same style as those could be matched + // but not present in the element directly. + for ( const relatedStyle of stylesProcessor.getRelatedStyles( style ) ) { + if ( consumable.consume( viewElement, { styles: [ relatedStyle ] } ) ) { + result.styles.push( relatedStyle ); } } - } - - return consumedMatches.concat( consumeAttributeMatch( viewElement, consumable, match ) ); - }, [] as Array ); -} - -/** - * TODO - */ -function consumeAttributeMatch( viewElement: ViewElement, consumable: ViewConsumable, match: Match ): Match { - removeConsumedAttributes( consumable, viewElement, match ); - - // We only want to consume attributes, so element can be still processed by other converters. - delete match.name; - - consumable.consume( viewElement, match ); - return match; -} - -/** - * Removes attributes from the given match that were already consumed by other converters. - */ -function removeConsumedAttributes( consumable: ViewConsumable, viewElement: ViewElement, match: Match ) { - for ( const key of [ 'attributes', 'classes', 'styles' ] as const ) { - const attributes = match[ key ]; + if ( consumable.consume( viewElement, { styles: [ style ] } ) ) { + result.styles.push( style ); + } + } - if ( !attributes ) { - continue; + for ( const className of match.classes || [] ) { + if ( consumable.consume( viewElement, { classes: [ className ] } ) ) { + result.classes.push( className ); + } } - // 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 ); + for ( const attributeName of match.attributes || [] ) { + if ( consumable.consume( viewElement, { attributes: [ attributeName ] } ) ) { + result.attributes.push( attributeName ); } } - } + + return result; + }, { + attributes: [] as Array, + classes: [] as Array, + styles: [] as Array + } ); } /** - * 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. + * Prepares the GHS attribute value as an object with element attributes' values. */ -function mergeMatchResults( matches: Array ): { - attributes: Set; - classes: Set; - styles: Set; -} { - const keys = [ 'attributes', 'classes', 'styles' ] as const; - const matchResult = { - attributes: new Set(), - classes: new Set(), - styles: new Set() - }; - - for ( const match of matches ) { - for ( const key of keys ) { - for ( const value of match[ key ] || [] ) { - matchResult[ key ].add( value ); - } - } +function normalizeGHSAttribute( + viewElement: ViewElement, + { attributes, classes, styles }: { + attributes: Array; + classes: Array; + styles: Array; + } +): GHSViewAttributes | null { + if ( !attributes.length && !classes.length && !styles.length ) { + return null; } - return matchResult; + return { + ...( attributes.length && { + attributes: normalizeAttributes( viewElement, attributes ) + } ), + + ...( styles.length && { + styles: normalizeStyles( viewElement, styles ) + } ), + + ...( classes.length && { + classes + } ) + }; } /** @@ -982,8 +935,8 @@ function splitPattern( pattern: MatcherObjectPattern, attributeName: 'attributes const attributeValue = pattern[ attributeName ]; if ( isPlainObject( attributeValue ) ) { - return Object.entries( attributeValue as Record ).map( - ( [ key, value ] ) => ( { + return Object.entries( attributeValue as Record ) + .map( ( [ key, value ] ) => ( { name, [ attributeName ]: { [ key ]: value @@ -992,12 +945,11 @@ function splitPattern( pattern: MatcherObjectPattern, attributeName: 'attributes } if ( Array.isArray( attributeValue ) ) { - return attributeValue.map( - value => ( { + return attributeValue + .map( value => ( { name, [ attributeName ]: [ value ] - } ) - ); + } ) ); } return [ pattern ]; From aae2423c3d92f829bc6e63bc064a27deec1de2ed Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 19 Feb 2024 15:54:49 +0100 Subject: [PATCH 06/13] Removed unused import. --- packages/ckeditor5-html-support/src/datafilter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index f8aacbf25c7..4ecbc709bc8 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -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'; From 110c09f5714427c2071b635e83968319f8985da1 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 19 Feb 2024 20:11:41 +0100 Subject: [PATCH 07/13] Added support for filtering only partial style value. --- .../ckeditor5-html-support/src/datafilter.ts | 12 ++- .../tests/datafilter.js | 88 +++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index 4ecbc709bc8..0354ced5e97 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -824,26 +824,36 @@ function matchAndConsumeAttributes( const stylesProcessor = viewElement.document.stylesProcessor; return matches.reduce( ( result, { match } ) => { + // Verify and consume styles. for ( const style of match.styles || [] ) { - // Check other forms of the same style as those could be matched + // Check longer forms of the same style as those could be matched // but not present in the element directly. for ( const relatedStyle of stylesProcessor.getRelatedStyles( style ) ) { + // 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. + if ( relatedStyle.split( '-' ).length <= style.split( '-' ).length ) { + continue; + } + if ( consumable.consume( viewElement, { styles: [ relatedStyle ] } ) ) { result.styles.push( relatedStyle ); } } + // Verify and consume style as specified in the matcher. if ( consumable.consume( viewElement, { styles: [ style ] } ) ) { result.styles.push( style ); } } + // Verify and consume class names. for ( const className of match.classes || [] ) { if ( consumable.consume( viewElement, { classes: [ className ] } ) ) { result.classes.push( className ); } } + // Verify and consume other attributes. for ( const attributeName of match.attributes || [] ) { if ( consumable.consume( viewElement, { attributes: [ attributeName ] } ) ) { result.attributes.push( attributeName ); diff --git a/packages/ckeditor5-html-support/tests/datafilter.js b/packages/ckeditor5-html-support/tests/datafilter.js index 21e7653dbba..0b3b558478d 100644 --- a/packages/ckeditor5-html-support/tests/datafilter.js +++ b/packages/ckeditor5-html-support/tests/datafilter.js @@ -3970,6 +3970,28 @@ describe( 'DataFilter', () => { expect( editor.getData() ).to.equal( '

foobar

' ); } ); + it( 'should handle shorthand style for longhand filter (background vs background-color)', () => { + editor.data.addStyleProcessorRules( addBackgroundRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'background' } ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'background-color': 'red' + } + } + } + } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + it( 'should handle partial padding for generic padding filter (single box side)', () => { editor.data.addStyleProcessorRules( addPaddingRules ); @@ -3992,6 +4014,28 @@ describe( 'DataFilter', () => { expect( editor.getData() ).to.equal( '

foobar

' ); } ); + it( 'should handle partial padding for specific full padding filter (single box side)', () => { + editor.data.addStyleProcessorRules( addPaddingRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'padding-left' } ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'padding-left': '10px' + } + } + } + } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + it( 'should handle partial padding for generic padding filter (multiple sides)', () => { editor.data.addStyleProcessorRules( addPaddingRules ); @@ -4059,6 +4103,50 @@ describe( 'DataFilter', () => { expect( editor.getData() ).to.equal( '

foobar

' ); } ); + it( 'should handle partial border for partial border filter (box bottom side)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border-bottom' } ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-bottom': '3px dotted red' + } + } + } + } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle partial border for partial border filter (color only)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border-color' } ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-color': 'red' + } + } + } + } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + it( 'should handle partial border for generic border filter (mixed)', () => { editor.data.addStyleProcessorRules( addBorderRules ); From 5b75665370b76690508b93158b6b1846b6ca292b Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 19 Feb 2024 20:14:15 +0100 Subject: [PATCH 08/13] Updated comment. --- packages/ckeditor5-html-support/src/datafilter.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index 0354ced5e97..2c7485cd17a 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -807,9 +807,12 @@ export interface DataFilterRegisterEvent { } /** - * Consumes matched attributes. + * Matches and consumes matched attributes. * - * @returns Array with match information about found attributes. + * @returns Object with following properties: + * * - attributes Array with matched attribute names. + * * - styles Array with matched style names. + * * - classes Array with matched class names. */ function matchAndConsumeAttributes( viewElement: ViewElement, From 8da75278baebfe746518ad6e95ab2b5105e8bc72 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 19 Feb 2024 20:34:32 +0100 Subject: [PATCH 09/13] Added test. --- .../tests/datafilter.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/ckeditor5-html-support/tests/datafilter.js b/packages/ckeditor5-html-support/tests/datafilter.js index 0b3b558478d..934f3dd8a21 100644 --- a/packages/ckeditor5-html-support/tests/datafilter.js +++ b/packages/ckeditor5-html-support/tests/datafilter.js @@ -4103,6 +4103,29 @@ describe( 'DataFilter', () => { expect( editor.getData() ).to.equal( '

foobar

' ); } ); + it( 'should handle partial border for generic border filter (missing border color)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border-left' } ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-left-style': 'solid', + 'border-left-width': '1px' + } + } + } + } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + it( 'should handle partial border for partial border filter (box bottom side)', () => { editor.data.addStyleProcessorRules( addBorderRules ); From 93738178cd350a81422a13351cf168d3c075a41b Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 22 Feb 2024 13:59:08 +0100 Subject: [PATCH 10/13] Fixed case of matching `border` for `border-left: solid red` data. --- .../ckeditor5-html-support/src/datafilter.ts | 12 ++--- .../tests/datafilter.js | 45 +++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index 2c7485cd17a..b88cac076d2 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -831,13 +831,13 @@ function matchAndConsumeAttributes( 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. - for ( const relatedStyle of stylesProcessor.getRelatedStyles( style ) ) { - // 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. - if ( relatedStyle.split( '-' ).length <= style.split( '-' ).length ) { - continue; - } + // 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 ); } diff --git a/packages/ckeditor5-html-support/tests/datafilter.js b/packages/ckeditor5-html-support/tests/datafilter.js index 934f3dd8a21..60a11ff6e51 100644 --- a/packages/ckeditor5-html-support/tests/datafilter.js +++ b/packages/ckeditor5-html-support/tests/datafilter.js @@ -4103,6 +4103,51 @@ describe( 'DataFilter', () => { expect( editor.getData() ).to.equal( '

foobar

' ); } ); + it( 'should handle partial border for generic border filter (box bottom side style only)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border' } ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-bottom-style': 'dotted' + } + } + } + } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + + it( 'should handle partial border for generic border filter (box bottom side style and color)', () => { + editor.data.addStyleProcessorRules( addBorderRules ); + + dataFilter.allowElement( 'p' ); + dataFilter.allowAttributes( { name: 'p', styles: 'border' } ); + + editor.setData( '

foobar

' ); + + expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( { + data: 'foobar', + attributes: { + 1: { + styles: { + 'border-bottom-style': 'dotted', + 'border-bottom-color': 'red' + } + } + } + } ); + + expect( editor.getData() ).to.equal( '

foobar

' ); + } ); + it( 'should handle partial border for generic border filter (missing border color)', () => { editor.data.addStyleProcessorRules( addBorderRules ); From 90e477dc30a76c2b687cf7af5598c0ab0501ceeb Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 22 Feb 2024 14:00:23 +0100 Subject: [PATCH 11/13] Fixed js comment. --- packages/ckeditor5-html-support/src/datafilter.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index b88cac076d2..9110b4712e8 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -810,9 +810,9 @@ export interface DataFilterRegisterEvent { * Matches and consumes matched attributes. * * @returns Object with following properties: - * * - attributes Array with matched attribute names. - * * - styles Array with matched style names. - * * - classes Array with matched class names. + * - attributes Array with matched attribute names. + * - classes Array with matched class names. + * - styles Array with matched style names. */ function matchAndConsumeAttributes( viewElement: ViewElement, From fc6de1717eaef363e265f04abc9207b3046e5f09 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 22 Feb 2024 14:12:24 +0100 Subject: [PATCH 12/13] Removed meaningless comment. --- packages/ckeditor5-engine/src/view/stylesmap.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/stylesmap.ts b/packages/ckeditor5-engine/src/view/stylesmap.ts index 61ed4ab6203..7cb6859636e 100644 --- a/packages/ckeditor5-engine/src/view/stylesmap.ts +++ b/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 { /** From 1e98d25db908eef0cf1a3b62a8ff62a621d250b6 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 22 Feb 2024 14:31:47 +0100 Subject: [PATCH 13/13] Functions' naming update. --- .../ckeditor5-html-support/src/datafilter.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/ckeditor5-html-support/src/datafilter.ts b/packages/ckeditor5-html-support/src/datafilter.ts index 9110b4712e8..a5d0222ceb6 100644 --- a/packages/ckeditor5-html-support/src/datafilter.ts +++ b/packages/ckeditor5-html-support/src/datafilter.ts @@ -317,7 +317,7 @@ export default class DataFilter extends Plugin { // For example, for block images the
converter triggers conversion for first and then for other elements, i.e. . matchAndConsumeAttributes( viewElement, this._disallowedAttributes, consumable ); - return normalizeGHSAttribute( viewElement, matchAndConsumeAttributes( viewElement, this._allowedAttributes, consumable ) ); + return prepareGHSAttribute( viewElement, matchAndConsumeAttributes( viewElement, this._allowedAttributes, consumable ) ); } /** @@ -874,7 +874,7 @@ function matchAndConsumeAttributes( /** * Prepares the GHS attribute value as an object with element attributes' values. */ -function normalizeGHSAttribute( +function prepareGHSAttribute( viewElement: ViewElement, { attributes, classes, styles }: { attributes: Array; @@ -888,11 +888,11 @@ function normalizeGHSAttribute( return { ...( attributes.length && { - attributes: normalizeAttributes( viewElement, attributes ) + attributes: getAttributes( viewElement, attributes ) } ), ...( styles.length && { - styles: normalizeStyles( viewElement, styles ) + styles: getReducedStyles( viewElement, styles ) } ), ...( classes.length && { @@ -902,9 +902,9 @@ function normalizeGHSAttribute( } /** - * Returns list of attributes as a normalized object. + * Returns attributes as an object with names and values. */ -function normalizeAttributes( viewElement: ViewElement, attributes: Iterable ): Record { +function getAttributes( viewElement: ViewElement, attributes: Iterable ): Record { const attributesObject: Record = {}; for ( const key of attributes ) { @@ -919,9 +919,9 @@ function normalizeAttributes( viewElement: ViewElement, attributes: Iterable ): Record { +function getReducedStyles( viewElement: ViewElement, styles: Iterable ): Record { // 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 );