From 5567ac153c80d1e0cb02b3a58026d1932634c9f8 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 8 Jan 2024 16:42:00 +0100 Subject: [PATCH 1/8] Fix implementation. --- .../src/colorselector/colorgridsfragmentview.ts | 6 ++++-- .../tests/colorselector/colorselectorview.js | 9 +++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts b/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts index 963a0ffa0fc..7373c294e6b 100644 --- a/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts +++ b/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts @@ -237,10 +237,12 @@ export default class ColorGridsFragmentView extends View { */ public updateSelectedColors(): void { const documentColorsGrid = this.documentColorsGrid; - const staticColorsGrid = this.staticColorsGrid!; + const staticColorsGrid = this.staticColorsGrid; const selectedColor = this.selectedColor; - staticColorsGrid.selectedColor = selectedColor; + if ( staticColorsGrid ) { + staticColorsGrid.selectedColor = selectedColor; + } if ( documentColorsGrid ) { documentColorsGrid.selectedColor = selectedColor; diff --git a/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js b/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js index 958735ade01..f51cbad17c0 100644 --- a/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js +++ b/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js @@ -301,6 +301,15 @@ describe( 'ColorSelectorView', () => { sinon.assert.calledOnce( spy ); } ); + it( 'should call updateSelectedColors when color grid is not defined without errors', () => { + const spy = sinon.spy( colorSelectorView, 'updateSelectedColors' ); + + colorSelectorView.colorGridsFragmentView.staticColorsGrid = null; + colorSelectorView.updateSelectedColors(); + + sinon.assert.calledOnce( spy ); + } ); + it( 'should unset selected color', () => { setModelData( model, '<$text testColor="gold">Bar' + From 286ffdb8bc27b6f62ca214f3b2618b8640b47aaa Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Tue, 9 Jan 2024 11:51:33 +0100 Subject: [PATCH 2/8] Add manual test. --- .../tests/manual/tickets/15580/1.html | 25 ++++++++ .../tests/manual/tickets/15580/1.js | 59 +++++++++++++++++++ .../tests/manual/tickets/15580/1.md | 5 ++ 3 files changed, 89 insertions(+) create mode 100644 packages/ckeditor5-font/tests/manual/tickets/15580/1.html create mode 100644 packages/ckeditor5-font/tests/manual/tickets/15580/1.js create mode 100644 packages/ckeditor5-font/tests/manual/tickets/15580/1.md diff --git a/packages/ckeditor5-font/tests/manual/tickets/15580/1.html b/packages/ckeditor5-font/tests/manual/tickets/15580/1.html new file mode 100644 index 00000000000..cea9cedf2aa --- /dev/null +++ b/packages/ckeditor5-font/tests/manual/tickets/15580/1.html @@ -0,0 +1,25 @@ +
+

01. no-color; no-color

+

02. color:hsl(0, 0%, 100%);background-color:hsl(0, 0%, 0%);

+

03. color:hsl(0, 75%, 60%); no-color;

+

04. no-color; background-color:hsl(90, 75%, 60%);

+

05. color:hsl(30, 75%, 60%); background-color:hsl(0, 0%, 30%);

+

06. color:#00FFFF;background-color:rgb(255, 0, 0);

+

07. color:hsla( 0, 0%, 0%, .7); background-color:gold;

+

08. color:;background-color:hsla(270, 100%, 50%, 0.3);

+

09. color:#ddd;background-color:hsl(150, 75%, 60%);

+

10. color:hsl(270, 75%, 60%);background-color:#d82;

+
+ +
+

You can use this helper to generate text with a particular color / background applied. Change inputs and accept it with enter key.

+ + +

Text for copying.

+
diff --git a/packages/ckeditor5-font/tests/manual/tickets/15580/1.js b/packages/ckeditor5-font/tests/manual/tickets/15580/1.js new file mode 100644 index 00000000000..289343c951e --- /dev/null +++ b/packages/ckeditor5-font/tests/manual/tickets/15580/1.js @@ -0,0 +1,59 @@ +/** + * @license Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/* globals console, window, document */ + +import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor.js'; +import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset.js'; +import FontColor from '../../../../src/fontcolor.js'; +import FontBackgroundColor from '../../../../src/fontbackgroundcolor.js'; +import { BalloonToolbar } from '@ckeditor/ckeditor5-ui'; + +ClassicEditor + .create( document.querySelector( '#editor' ), { + image: { toolbar: [ 'toggleImageCaption', 'imageTextAlternative' ] }, + plugins: [ + ArticlePluginSet, + FontColor, + FontBackgroundColor, + BalloonToolbar + ], + toolbar: [ + 'heading', + '|', + 'fontColor', + 'fontBackgroundColor', + 'bold', + 'italic', + 'link', + 'bulletedList', + 'numberedList', + 'blockQuote', + 'undo', + 'redo' + ], + fontColor: { + columns: 3 + }, + balloonToolbar: [ 'bold', 'italic', 'fontColor' ] + } ) + .then( editor => { + window.editor = editor; + } ) + .catch( err => { + console.error( err.stack ); + } ); + +function updateText( styleName ) { + return evt => { + const el = document.querySelector( '#color-box > p > span' ); + if ( el ) { + el.style[ styleName ] = evt.target.value; + } + }; +} + +document.getElementById( 'color' ).addEventListener( 'change', updateText( 'color' ) ); +document.getElementById( 'bgcolor' ).addEventListener( 'change', updateText( 'backgroundColor' ) ); diff --git a/packages/ckeditor5-font/tests/manual/tickets/15580/1.md b/packages/ckeditor5-font/tests/manual/tickets/15580/1.md new file mode 100644 index 00000000000..39c7e757e96 --- /dev/null +++ b/packages/ckeditor5-font/tests/manual/tickets/15580/1.md @@ -0,0 +1,5 @@ +## Font color and font background color throw error while having buttons in the main toolbar and the balloon toolbar [#8233](https://github.com/ckeditor/ckeditor5/pull/15658) + +1. Select text, should appear balloon with font color button. +2. Click in the font color button and change color. +3. In console should no be any errors. From 8e06929f84083f0f77a0d6aad690a08d747ed4d5 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Fri, 12 Jan 2024 14:31:28 +0100 Subject: [PATCH 3/8] Code review fixes. --- packages/ckeditor5-font/src/ui/colorui.ts | 23 +++---- packages/ckeditor5-font/tests/ui/colorui.js | 63 ++++++++++++++++++- .../colorselector/colorgridsfragmentview.ts | 6 +- .../tests/colorselector/colorselectorview.js | 9 --- 4 files changed, 70 insertions(+), 31 deletions(-) diff --git a/packages/ckeditor5-font/src/ui/colorui.ts b/packages/ckeditor5-font/src/ui/colorui.ts index b700c515cf5..124f4840964 100644 --- a/packages/ckeditor5-font/src/ui/colorui.ts +++ b/packages/ckeditor5-font/src/ui/colorui.ts @@ -14,7 +14,6 @@ import { normalizeColorOptions, getLocalizedColorOptions, focusChildOnDropdownOpen, - type ColorSelectorView, type ColorSelectorExecuteEvent, type ColorSelectorColorPickerCancelEvent, type ColorSelectorColorPickerShowEvent @@ -63,11 +62,6 @@ export default class ColorUI extends Plugin { */ public columns: number; - /** - * Keeps a reference to {@link module:ui/colorselector/colorselectorview~ColorSelectorView}. - */ - public colorSelectorView: ColorSelectorView | undefined; - /** * Keeps all changes in color picker in one batch while dropdown is open. */ @@ -100,7 +94,6 @@ export default class ColorUI extends Plugin { this.icon = icon; this.dropdownLabel = dropdownLabel; this.columns = editor.config.get( `${ this.componentName }.columns` )!; - this.colorSelectorView = undefined; } /** @@ -123,7 +116,7 @@ export default class ColorUI extends Plugin { // Font color dropdown rendering is deferred once it gets open to improve performance (#6192). let dropdownContentRendered = false; - this.colorSelectorView = addColorSelectorToDropdown( { + const colorSelectorView = addColorSelectorToDropdown( { dropdownView, colors: localizedColors.map( option => ( { label: option.label, @@ -140,7 +133,7 @@ export default class ColorUI extends Plugin { colorPickerViewConfig: hasColorPicker ? ( componentConfig.colorPicker || {} ) : false } ); - this.colorSelectorView.bind( 'selectedColor' ).to( command, 'value' ); + colorSelectorView.bind( 'selectedColor' ).to( command, 'value' ); dropdownView.buttonView.set( { label: this.dropdownLabel, @@ -156,7 +149,7 @@ export default class ColorUI extends Plugin { dropdownView.bind( 'isEnabled' ).to( command ); - this.colorSelectorView.on( 'execute', ( evt, data ) => { + colorSelectorView.on( 'execute', ( evt, data ) => { if ( dropdownView.isOpen ) { editor.execute( this.commandName, { value: data.value, @@ -173,11 +166,11 @@ export default class ColorUI extends Plugin { } } ); - this.colorSelectorView.on( 'colorPicker:show', () => { + colorSelectorView.on( 'colorPicker:show', () => { this._undoStepBatch = editor.model.createBatch(); } ); - this.colorSelectorView.on( 'colorPicker:cancel', () => { + colorSelectorView.on( 'colorPicker:cancel', () => { if ( this._undoStepBatch!.operations.length ) { // We need to close the dropdown before the undo batch. // Otherwise, ColorUI treats undo as a selected color change, @@ -199,11 +192,11 @@ export default class ColorUI extends Plugin { if ( isVisible ) { if ( documentColorsCount !== 0 ) { - this.colorSelectorView!.updateDocumentColors( editor.model, this.componentName ); + colorSelectorView!.updateDocumentColors( editor.model, this.componentName ); } - this.colorSelectorView!.updateSelectedColors(); - this.colorSelectorView!.showColorGridsFragment(); + colorSelectorView!.updateSelectedColors(); + colorSelectorView!.showColorGridsFragment(); } } ); diff --git a/packages/ckeditor5-font/tests/ui/colorui.js b/packages/ckeditor5-font/tests/ui/colorui.js index 9b03b36c62b..4d9789ba42f 100644 --- a/packages/ckeditor5-font/tests/ui/colorui.js +++ b/packages/ckeditor5-font/tests/ui/colorui.js @@ -232,7 +232,7 @@ describe( 'ColorUI', () => { const spyUndo = sinon.spy( editor.commands.get( 'undo' ), 'execute' ); dropdown.isOpen = true; - testColorPlugin.colorSelectorView.fire( 'colorPicker:show' ); + dropdown.colorSelectorView.fire( 'colorPicker:show' ); dropdown.colorSelectorView.selectedColor = 'hsl( 0, 0%, 100% )'; @@ -250,7 +250,7 @@ describe( 'ColorUI', () => { it( 'should create new batch when color picker is showed', () => { dropdown.isOpen = true; - testColorPlugin.colorSelectorView.colorGridsFragmentView.colorPickerButtonView.fire( 'execute' ); + dropdown.colorSelectorView.colorGridsFragmentView.colorPickerButtonView.fire( 'execute' ); dropdown.colorSelectorView.selectedColor = '#000000'; @@ -270,7 +270,7 @@ describe( 'ColorUI', () => { } ); dropdown.isOpen = true; - testColorPlugin.colorSelectorView.colorGridsFragmentView.colorPickerButtonView.fire( 'execute' ); + dropdown.colorSelectorView.colorGridsFragmentView.colorPickerButtonView.fire( 'execute' ); expect( testColorPlugin._undoStepBatch.operations.length, 'should have 0 changes in batch' ).to.equal( 0 ); @@ -535,4 +535,61 @@ describe( 'ColorUI', () => { expect( dropdown.colorSelectorView.colorPickerView ).to.be.undefined; } ); } ); + + // Issue: https://github.com/ckeditor/ckeditor5/issues/15580 + // For simplicity we create editor with two same buttons in toolbar + // instead overcomplicating stuff with ballon toolbar. + describe( 'toolabar with two same instance of testColor', () => { + let editor, element; + + beforeEach( () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + return ClassicTestEditor + .create( element, { + plugins: [ Paragraph, TestColorPlugin, Undo ], + testColor: testColorConfig, + toolbar: [ + 'testColor', + 'testColor' + ] + } ) + .then( newEditor => { + editor = newEditor; + model = editor.model; + testColorPlugin = newEditor.plugins.get( 'TestColorPlugin' ); + } ); + } ); + + afterEach( () => { + element.remove(); + + return editor.destroy(); + } ); + + describe( 'testColor Dropdown', () => { + let dropdown; + + beforeEach( () => { + command = editor.commands.get( 'testColorCommand' ); + dropdown = editor.ui.componentFactory.create( 'testColor' ); + dropdown.isOpen = true; + + dropdown.render(); + } ); + + afterEach( () => { + dropdown.destroy(); + } ); + + it( 'should execute command if the color gets changed when dropdown is open', () => { + const spy = sinon.spy( editor, 'execute' ); + + dropdown.colorSelectorView.colorPickerFragmentView.colorPickerView.fire( 'colorSelected', { color: '#a37474' } ); + + sinon.assert.calledWithExactly( spy, 'testColorCommand', sinon.match( { value: '#a37474' } ) ); + } ); + } ); + } ); } ); diff --git a/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts b/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts index 7373c294e6b..963a0ffa0fc 100644 --- a/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts +++ b/packages/ckeditor5-ui/src/colorselector/colorgridsfragmentview.ts @@ -237,12 +237,10 @@ export default class ColorGridsFragmentView extends View { */ public updateSelectedColors(): void { const documentColorsGrid = this.documentColorsGrid; - const staticColorsGrid = this.staticColorsGrid; + const staticColorsGrid = this.staticColorsGrid!; const selectedColor = this.selectedColor; - if ( staticColorsGrid ) { - staticColorsGrid.selectedColor = selectedColor; - } + staticColorsGrid.selectedColor = selectedColor; if ( documentColorsGrid ) { documentColorsGrid.selectedColor = selectedColor; diff --git a/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js b/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js index f51cbad17c0..958735ade01 100644 --- a/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js +++ b/packages/ckeditor5-ui/tests/colorselector/colorselectorview.js @@ -301,15 +301,6 @@ describe( 'ColorSelectorView', () => { sinon.assert.calledOnce( spy ); } ); - it( 'should call updateSelectedColors when color grid is not defined without errors', () => { - const spy = sinon.spy( colorSelectorView, 'updateSelectedColors' ); - - colorSelectorView.colorGridsFragmentView.staticColorsGrid = null; - colorSelectorView.updateSelectedColors(); - - sinon.assert.calledOnce( spy ); - } ); - it( 'should unset selected color', () => { setModelData( model, '<$text testColor="gold">Bar' + From 4c41dd5d3d76b1a4f005a85844ed263c701a89a2 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 15 Jan 2024 12:44:44 +0100 Subject: [PATCH 4/8] Code reivew fixes. --- .../ckeditor5-font/tests/manual/tickets/15580/1.md | 2 +- packages/ckeditor5-font/tests/ui/colorui.js | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-font/tests/manual/tickets/15580/1.md b/packages/ckeditor5-font/tests/manual/tickets/15580/1.md index 39c7e757e96..d1e79b08ba5 100644 --- a/packages/ckeditor5-font/tests/manual/tickets/15580/1.md +++ b/packages/ckeditor5-font/tests/manual/tickets/15580/1.md @@ -2,4 +2,4 @@ 1. Select text, should appear balloon with font color button. 2. Click in the font color button and change color. -3. In console should no be any errors. +3. There should be no errors in the console. diff --git a/packages/ckeditor5-font/tests/ui/colorui.js b/packages/ckeditor5-font/tests/ui/colorui.js index 2c6244b0f74..e90f22b89b8 100644 --- a/packages/ckeditor5-font/tests/ui/colorui.js +++ b/packages/ckeditor5-font/tests/ui/colorui.js @@ -569,11 +569,12 @@ describe( 'ColorUI', () => { } ); describe( 'testColor Dropdown', () => { - let dropdown; + let dropdown, dropdown2; beforeEach( () => { command = editor.commands.get( 'testColorCommand' ); dropdown = editor.ui.componentFactory.create( 'testColor' ); + dropdown2 = editor.ui.componentFactory.create( 'testColor' ); dropdown.isOpen = true; dropdown.render(); @@ -586,9 +587,17 @@ describe( 'ColorUI', () => { it( 'should execute command if the color gets changed when dropdown is open', () => { const spy = sinon.spy( editor, 'execute' ); + dropdown.isOpen = true; + dropdown.colorSelectorView.colorPickerFragmentView.colorPickerView.fire( 'colorSelected', { color: '#a37474' } ); sinon.assert.calledWithExactly( spy, 'testColorCommand', sinon.match( { value: '#a37474' } ) ); + + dropdown2.isOpen = true; + + dropdown2.colorSelectorView.colorPickerFragmentView.colorPickerView.fire( 'colorSelected', { color: '#ffffff' } ); + + sinon.assert.calledWithExactly( spy, 'testColorCommand', sinon.match( { value: '#ffffff' } ) ); } ); } ); } ); From 6dfb77b40803f1448a5c0efd646651b494f23098 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 15 Jan 2024 12:48:36 +0100 Subject: [PATCH 5/8] Rename test. --- packages/ckeditor5-font/tests/ui/colorui.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/ckeditor5-font/tests/ui/colorui.js b/packages/ckeditor5-font/tests/ui/colorui.js index e90f22b89b8..eb1ad846e0b 100644 --- a/packages/ckeditor5-font/tests/ui/colorui.js +++ b/packages/ckeditor5-font/tests/ui/colorui.js @@ -575,7 +575,6 @@ describe( 'ColorUI', () => { command = editor.commands.get( 'testColorCommand' ); dropdown = editor.ui.componentFactory.create( 'testColor' ); dropdown2 = editor.ui.componentFactory.create( 'testColor' ); - dropdown.isOpen = true; dropdown.render(); } ); @@ -584,7 +583,7 @@ describe( 'ColorUI', () => { dropdown.destroy(); } ); - it( 'should execute command if the color gets changed when dropdown is open', () => { + it( 'should execute command if in toolbar there are more than one dropdowns', () => { const spy = sinon.spy( editor, 'execute' ); dropdown.isOpen = true; From 85cfa7662b4c3f02df63ad225c13ec48abf9256a Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 15 Jan 2024 12:59:46 +0100 Subject: [PATCH 6/8] Fix license header. --- packages/ckeditor5-font/tests/manual/tickets/15580/1.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-font/tests/manual/tickets/15580/1.js b/packages/ckeditor5-font/tests/manual/tickets/15580/1.js index 289343c951e..43ba4cf7c0a 100644 --- a/packages/ckeditor5-font/tests/manual/tickets/15580/1.js +++ b/packages/ckeditor5-font/tests/manual/tickets/15580/1.js @@ -1,5 +1,5 @@ /** - * @license Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved. + * @license Copyright (c) 2003-2024, CKSource Holding sp. z o.o. All rights reserved. * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ From 2a7ded547025da56c28dc099862c1642f92d9686 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 15 Jan 2024 14:39:59 +0100 Subject: [PATCH 7/8] Remove redundant testColor from toolbar in test. --- packages/ckeditor5-font/tests/ui/colorui.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/ckeditor5-font/tests/ui/colorui.js b/packages/ckeditor5-font/tests/ui/colorui.js index eb1ad846e0b..82512a77d9d 100644 --- a/packages/ckeditor5-font/tests/ui/colorui.js +++ b/packages/ckeditor5-font/tests/ui/colorui.js @@ -550,10 +550,7 @@ describe( 'ColorUI', () => { .create( element, { plugins: [ Paragraph, TestColorPlugin, Undo ], testColor: testColorConfig, - toolbar: [ - 'testColor', - 'testColor' - ] + toolbar: [ 'testColor' ] } ) .then( newEditor => { editor = newEditor; From 78293a4e74265937c25dc1fbd8e709ef7fa32e21 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 15 Jan 2024 15:54:11 +0100 Subject: [PATCH 8/8] Code review fixes. --- packages/ckeditor5-font/tests/ui/colorui.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/ckeditor5-font/tests/ui/colorui.js b/packages/ckeditor5-font/tests/ui/colorui.js index 82512a77d9d..12124790da7 100644 --- a/packages/ckeditor5-font/tests/ui/colorui.js +++ b/packages/ckeditor5-font/tests/ui/colorui.js @@ -572,12 +572,11 @@ describe( 'ColorUI', () => { command = editor.commands.get( 'testColorCommand' ); dropdown = editor.ui.componentFactory.create( 'testColor' ); dropdown2 = editor.ui.componentFactory.create( 'testColor' ); - - dropdown.render(); } ); afterEach( () => { dropdown.destroy(); + dropdown2.destroy(); } ); it( 'should execute command if in toolbar there are more than one dropdowns', () => {