From fc7f17c667fd22ab07da08bc43d5cd81ee70fc92 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Tue, 21 Apr 2020 09:07:43 +0200 Subject: [PATCH 1/4] fontSize.supportAllValues will not work with named presets. --- packages/ckeditor5-font/docs/features/font.md | 7 ++-- packages/ckeditor5-font/src/fontsize.js | 5 ++- .../src/fontsize/fontsizeediting.js | 23 +++++++++++-- .../tests/fontsize/fontsizeediting.js | 33 ++++++++++++++++++- packages/ckeditor5-font/tests/integration.js | 2 ++ 5 files changed, 64 insertions(+), 6 deletions(-) diff --git a/packages/ckeditor5-font/docs/features/font.md b/packages/ckeditor5-font/docs/features/font.md index 4df339d1662..4b47b788fd5 100644 --- a/packages/ckeditor5-font/docs/features/font.md +++ b/packages/ckeditor5-font/docs/features/font.md @@ -173,13 +173,12 @@ ClassicEditor By default, all `font-size` values that are not specified in the `config.fontSize.options` are stripped. You can enable support for all font sizes by using the {@link module:font/fontfamily~FontFamilyConfig#supportAllValues `config.fontSize.supportAllValues`} option. - ```js ClassicEditor .create( document.querySelector( '#editor' ), { fontSize: { options: [ - // ... + // Numerical values. ], supportAllValues: true }, @@ -189,6 +188,10 @@ ClassicEditor .catch( ... ); ``` + + When this options is enabled, available options for the {@link module:font/fontsize~FontSize} plugin should be numerical values. + + ## Configuring the font color and font background color features Both font color and font background color features are configurable and share the same configuration format. diff --git a/packages/ckeditor5-font/src/fontsize.js b/packages/ckeditor5-font/src/fontsize.js index 5047d1163b1..bfa60683665 100644 --- a/packages/ckeditor5-font/src/fontsize.js +++ b/packages/ckeditor5-font/src/fontsize.js @@ -139,9 +139,12 @@ export default class FontSize extends Plugin { * You can preserve pasted font size values by switching the option: * * const fontSizeConfig = { - * supportAllValues: true + * options: [ 9, 10, 11, 12, 13, 14, 15 ], + * supportAllValues: true, * }; * + * You need to also define numerical options for the plugin. + * * Now, the font sizes, not specified in the editor's configuration, won't be removed when pasting the content. * * @member {Boolean} module:font/fontsize~FontSizeConfig#supportAllValues diff --git a/packages/ckeditor5-font/src/fontsize/fontsizeediting.js b/packages/ckeditor5-font/src/fontsize/fontsizeediting.js index b44b10b81f6..9180ee6f96b 100644 --- a/packages/ckeditor5-font/src/fontsize/fontsizeediting.js +++ b/packages/ckeditor5-font/src/fontsize/fontsizeediting.js @@ -12,6 +12,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import FontSizeCommand from './fontsizecommand'; import { normalizeOptions } from './utils'; import { buildDefinition, FONT_SIZE } from '../utils'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** * The font size editing feature. @@ -75,7 +76,7 @@ export default class FontSizeEditing extends Plugin { // Set-up the two-way conversion. if ( supportAllValues ) { - this._prepareAnyValueConverters(); + this._prepareAnyValueConverters( definition ); } else { editor.conversion.attributeToElement( definition ); } @@ -88,11 +89,29 @@ export default class FontSizeEditing extends Plugin { * Those converters enable keeping any value found as `style="font-size: *"` as a value of an attribute on a text even * if it isn't defined in the plugin configuration. * + * @param {Object} definition {@link module:engine/conversion/conversion~ConverterDefinition Converter definition} out of input data. * @private */ - _prepareAnyValueConverters() { + _prepareAnyValueConverters( definition ) { const editor = this.editor; + // If `fontSize.supportAllValues=true`, we do not allow to use named presets in the plugin's configuration. + const presets = definition.model.values.filter( value => !String( value ).match( /[\d.]+[\w%]+/ ) ); + + if ( presets.length ) { + /** + * If {@link module:font/fontsize~FontSizeConfig#supportAllValues} is `true`, you need to use numerical values as + * font size options. + * + * See valid examples described in the {@link module:font/fontsize~FontSizeConfig#options plugin configuration}. + * + * @error font-size-named-presets + */ + const message = 'font-size-named-presets: ' + + 'If set `fontSize.supportAllValues` on `true`, you cannot use named presets as plugin\'s configuration.'; + throw new CKEditorError( message, null, { presets } ); + } + editor.conversion.for( 'downcast' ).attributeToElement( { model: FONT_SIZE, view: ( attributeValue, writer ) => { diff --git a/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js b/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js index 469b32b882e..b2843967f40 100644 --- a/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js +++ b/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js @@ -9,6 +9,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; +import { assertCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; describe( 'FontSizeEditing', () => { let editor, doc; @@ -68,7 +69,17 @@ describe( 'FontSizeEditing', () => { .create( { plugins: [ FontSizeEditing, Paragraph ], fontSize: { - supportAllValues: true + supportAllValues: true, + options: [ + '6.25%', + '8em', + '10px', + 12, + { + model: 14, + title: '14px' + } + ] } } ) .then( newEditor => { @@ -101,6 +112,26 @@ describe( 'FontSizeEditing', () => { expect( editor.getData() ).to.equal( '

foo

' ); } ); } ); + + it( 'should throw an error if used with default configuration of the plugin', () => { + return VirtualTestEditor + .create( { + plugins: [ FontSizeEditing ], + fontSize: { + supportAllValues: true + } + } ) + .then( + () => { + throw new Error( 'Supposed to be rejected' ); + }, + error => { + assertCKEditorError( error, /font-size-named-presets/, null, { + presets: [ 'tiny', 'small', 'big', 'huge' ] + } ); + } + ); + } ); } ); } ); diff --git a/packages/ckeditor5-font/tests/integration.js b/packages/ckeditor5-font/tests/integration.js index 99828e43370..2c4f80230c4 100644 --- a/packages/ckeditor5-font/tests/integration.js +++ b/packages/ckeditor5-font/tests/integration.js @@ -63,6 +63,7 @@ describe( 'Integration test Font', () => { supportAllValues: true }, fontSize: { + options: [ 10, 12, 14 ], supportAllValues: true } } ) @@ -125,6 +126,7 @@ describe( 'Integration test Font', () => { supportAllValues: true }, fontSize: { + options: [ 10, 12, 14 ], supportAllValues: true } } ) From 7a5fe3d029f645fbf4165969704a3d1768e48d03 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Tue, 21 Apr 2020 09:08:02 +0200 Subject: [PATCH 2/4] Removed unused part of the code. --- .../src/fontsize/fontsizeediting.js | 2 +- packages/ckeditor5-font/src/fontsize/utils.js | 24 +++---------------- .../ckeditor5-font/tests/fontsize/utils.js | 20 +--------------- 3 files changed, 5 insertions(+), 41 deletions(-) diff --git a/packages/ckeditor5-font/src/fontsize/fontsizeediting.js b/packages/ckeditor5-font/src/fontsize/fontsizeediting.js index 9180ee6f96b..9fe271bc278 100644 --- a/packages/ckeditor5-font/src/fontsize/fontsizeediting.js +++ b/packages/ckeditor5-font/src/fontsize/fontsizeediting.js @@ -70,7 +70,7 @@ export default class FontSizeEditing extends Plugin { const supportAllValues = editor.config.get( 'fontSize.supportAllValues' ); // Define view to model conversion. - const options = normalizeOptions( this.editor.config.get( 'fontSize.options' ), { supportAllValues } ) + const options = normalizeOptions( this.editor.config.get( 'fontSize.options' ) ) .filter( item => item.model ); const definition = buildDefinition( FONT_SIZE, options ); diff --git a/packages/ckeditor5-font/src/fontsize/utils.js b/packages/ckeditor5-font/src/fontsize/utils.js index 86635d8957d..9472da7a6ab 100644 --- a/packages/ckeditor5-font/src/fontsize/utils.js +++ b/packages/ckeditor5-font/src/fontsize/utils.js @@ -14,28 +14,16 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; * to the {@link module:font/fontsize~FontSizeOption} format. * * @param {Array.} configuredOptions An array of options taken from the configuration. - * @param {Object} [options={}] - * @param {Boolean} [options.supportAllValues=false] * @returns {Array.} */ -export function normalizeOptions( configuredOptions, options = {} ) { - const supportAllValues = options.supportAllValues || false; - +export function normalizeOptions( configuredOptions ) { // Convert options to objects. return configuredOptions - .map( item => getOptionDefinition( item, supportAllValues ) ) + .map( item => getOptionDefinition( item ) ) // Filter out undefined values that `getOptionDefinition` might return. .filter( option => !!option ); } -// The values should be synchronized with values specified in the "/theme/fontsize.css" file. -export const FONT_SIZE_PRESET_UNITS = { - tiny: '0.7em', - small: '0.85em', - big: '1.4em', - huge: '1.8em' -}; - // Default named presets map. Always create a new instance of the preset object in order to avoid modifying references. const namedPresets = { get tiny() { @@ -86,12 +74,10 @@ const namedPresets = { // Returns an option definition either from preset or creates one from number shortcut. // If object is passed then this method will return it without alternating it. Returns undefined for item than cannot be parsed. -// If supportAllValues=true, model will be set to a specified unit value instead of text. // // @param {String|Number|Object} item -// @param {Boolean} supportAllValues // @returns {undefined|module:font/fontsize~FontSizeOption} -function getOptionDefinition( option, supportAllValues ) { +function getOptionDefinition( option ) { // Check whether passed option is a full item definition provided by user in configuration. if ( isFullItemDefinition( option ) ) { return attachPriority( option ); @@ -101,10 +87,6 @@ function getOptionDefinition( option, supportAllValues ) { // Item is a named preset. if ( preset ) { - if ( supportAllValues ) { - preset.model = FONT_SIZE_PRESET_UNITS[ option ]; - } - return attachPriority( preset ); } diff --git a/packages/ckeditor5-font/tests/fontsize/utils.js b/packages/ckeditor5-font/tests/fontsize/utils.js index cf6064378d8..c3989a038ef 100644 --- a/packages/ckeditor5-font/tests/fontsize/utils.js +++ b/packages/ckeditor5-font/tests/fontsize/utils.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import { normalizeOptions, FONT_SIZE_PRESET_UNITS } from '../../src/fontsize/utils'; +import { normalizeOptions } from '../../src/fontsize/utils'; import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; describe( 'FontSizeEditing Utils', () => { @@ -37,18 +37,6 @@ describe( 'FontSizeEditing Utils', () => { ] ); } ); - it( 'should return defined presets with units in model values if supportAllValues=true', () => { - const options = normalizeOptions( [ 'tiny', 'small', 'default', 'big', 'huge' ], { supportAllValues: true } ); - - expect( options ).to.deep.equal( [ - { title: 'Tiny', model: '0.7em', view: { name: 'span', classes: 'text-tiny', priority: 7 } }, - { title: 'Small', model: '0.85em', view: { name: 'span', classes: 'text-small', priority: 7 } }, - { title: 'Default', model: undefined }, - { title: 'Big', model: '1.4em', view: { name: 'span', classes: 'text-big', priority: 7 } }, - { title: 'Huge', model: '1.8em', view: { name: 'span', classes: 'text-huge', priority: 7 } } - ] ); - } ); - it( 'should add "view" definition if missing', () => { const tinyOption = { title: 'Tiny', @@ -165,10 +153,4 @@ describe( 'FontSizeEditing Utils', () => { } ); } ); } ); - - describe( 'FONT_SIZE_PRESET_UNITS', () => { - it( 'provides default values', () => { - expect( Object.keys( FONT_SIZE_PRESET_UNITS ).length ).to.equal( 4 ); - } ); - } ); } ); From 8377b3801c15d6a03df15bf511fecb7a5f83e8d4 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Tue, 21 Apr 2020 09:59:35 +0200 Subject: [PATCH 3/4] Removed an obsolete comment. --- packages/ckeditor5-font/theme/fontsize.css | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ckeditor5-font/theme/fontsize.css b/packages/ckeditor5-font/theme/fontsize.css index 85c93131182..677b6efc8ed 100644 --- a/packages/ckeditor5-font/theme/fontsize.css +++ b/packages/ckeditor5-font/theme/fontsize.css @@ -3,7 +3,6 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -/* The values should be synchronized with the "FONT_SIZE_PRESET_UNITS" object in the "/src/fontsize/utils.js" file. */ .text-tiny { font-size: .7em; } From fa6b3a56f78b8b8ec606740e5c883242b6277583 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 21 Apr 2020 15:46:10 +0200 Subject: [PATCH 4/4] Mainly wording. --- packages/ckeditor5-font/docs/features/font.md | 2 +- packages/ckeditor5-font/src/fontsize.js | 6 +++--- .../src/fontsize/fontsizeediting.js | 15 +++++++++------ .../tests/fontsize/fontsizeediting.js | 2 +- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-font/docs/features/font.md b/packages/ckeditor5-font/docs/features/font.md index 4b47b788fd5..78d1eccf199 100644 --- a/packages/ckeditor5-font/docs/features/font.md +++ b/packages/ckeditor5-font/docs/features/font.md @@ -189,7 +189,7 @@ ClassicEditor ``` - When this options is enabled, available options for the {@link module:font/fontsize~FontSize} plugin should be numerical values. + This option can be used only in combination with [numerical values](#using-numerical-values). ## Configuring the font color and font background color features diff --git a/packages/ckeditor5-font/src/fontsize.js b/packages/ckeditor5-font/src/fontsize.js index bfa60683665..6ad1eca91d0 100644 --- a/packages/ckeditor5-font/src/fontsize.js +++ b/packages/ckeditor5-font/src/fontsize.js @@ -139,11 +139,11 @@ export default class FontSize extends Plugin { * You can preserve pasted font size values by switching the option: * * const fontSizeConfig = { - * options: [ 9, 10, 11, 12, 13, 14, 15 ], - * supportAllValues: true, + * options: [ 9, 10, 11, 12, 'default', 14, 15 ], + * supportAllValues: true * }; * - * You need to also define numerical options for the plugin. + * **Note:** This option can only be used with numerical values as font size options. * * Now, the font sizes, not specified in the editor's configuration, won't be removed when pasting the content. * diff --git a/packages/ckeditor5-font/src/fontsize/fontsizeediting.js b/packages/ckeditor5-font/src/fontsize/fontsizeediting.js index 9fe271bc278..428b945d1bd 100644 --- a/packages/ckeditor5-font/src/fontsize/fontsizeediting.js +++ b/packages/ckeditor5-font/src/fontsize/fontsizeediting.js @@ -100,16 +100,19 @@ export default class FontSizeEditing extends Plugin { if ( presets.length ) { /** - * If {@link module:font/fontsize~FontSizeConfig#supportAllValues} is `true`, you need to use numerical values as - * font size options. + * If {@link module:font/fontsize~FontSizeConfig#supportAllValues `config.fontSize.supportAllValues`} is `true`, + * you need to use numerical values as font size options. * * See valid examples described in the {@link module:font/fontsize~FontSizeConfig#options plugin configuration}. * - * @error font-size-named-presets + * @error font-size-invalid-use-of-named-presets + * @param {Array.} presets Invalid values. */ - const message = 'font-size-named-presets: ' + - 'If set `fontSize.supportAllValues` on `true`, you cannot use named presets as plugin\'s configuration.'; - throw new CKEditorError( message, null, { presets } ); + throw new CKEditorError( + 'font-size-invalid-use-of-named-presets: ' + + 'If config.fontSize.supportAllValues is set to true, you need to use numerical values as font size options.', + null, { presets } + ); } editor.conversion.for( 'downcast' ).attributeToElement( { diff --git a/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js b/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js index b2843967f40..a358825435c 100644 --- a/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js +++ b/packages/ckeditor5-font/tests/fontsize/fontsizeediting.js @@ -126,7 +126,7 @@ describe( 'FontSizeEditing', () => { throw new Error( 'Supposed to be rejected' ); }, error => { - assertCKEditorError( error, /font-size-named-presets/, null, { + assertCKEditorError( error, /font-size-invalid-use-of-named-presets/, null, { presets: [ 'tiny', 'small', 'big', 'huge' ] } ); }