Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font color and font background color throw error while having buttons in the main toolbar and the balloon toolbar #15658

Merged
23 changes: 8 additions & 15 deletions packages/ckeditor5-font/src/ui/colorui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
normalizeColorOptions,
getLocalizedColorOptions,
focusChildOnDropdownOpen,
type ColorSelectorView,
type ColorSelectorExecuteEvent,
type ColorSelectorColorPickerCancelEvent,
type ColorSelectorColorPickerShowEvent
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -156,7 +149,7 @@ export default class ColorUI extends Plugin {

dropdownView.bind( 'isEnabled' ).to( command );

this.colorSelectorView.on<ColorSelectorExecuteEvent>( 'execute', ( evt, data ) => {
colorSelectorView.on<ColorSelectorExecuteEvent>( 'execute', ( evt, data ) => {
if ( dropdownView.isOpen ) {
editor.execute( this.commandName, {
value: data.value,
Expand All @@ -173,11 +166,11 @@ export default class ColorUI extends Plugin {
}
} );

this.colorSelectorView.on<ColorSelectorColorPickerShowEvent>( 'colorPicker:show', () => {
colorSelectorView.on<ColorSelectorColorPickerShowEvent>( 'colorPicker:show', () => {
this._undoStepBatch = editor.model.createBatch();
} );

this.colorSelectorView.on<ColorSelectorColorPickerCancelEvent>( 'colorPicker:cancel', () => {
colorSelectorView.on<ColorSelectorColorPickerCancelEvent>( '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,
Expand All @@ -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();
}
} );

Expand Down
25 changes: 25 additions & 0 deletions packages/ckeditor5-font/tests/manual/tickets/15580/1.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<div id="editor">
<p><span>01. no-color; no-color</span></p>
<p><span style="color:hsl(0, 0%, 100%);background-color:hsl(0, 0%, 0%);">02. color:hsl(0, 0%, 100%);background-color:hsl(0, 0%, 0%);</span></p>
<p><span style="color:hsl(0, 75%, 60%);">03. color:hsl(0, 75%, 60%); no-color;</span></p>
<p><span style="background-color:hsl(90, 75%, 60%);">04. no-color; background-color:hsl(90, 75%, 60%);</span></p>
<p><span style="color:hsl(30, 75%, 60%);background-color:hsl(0, 0%, 30%);">05. color:hsl(30, 75%, 60%); background-color:hsl(0, 0%, 30%);</span></p>
<p><span style="color:#00FFFF;background-color:rgb(255, 0, 0);">06. color:#00FFFF;background-color:rgb(255, 0, 0);</span></p>
<p><span style="color:hsla( 0, 0%, 0%, .7);background-color:gold;">07. color:hsla( 0, 0%, 0%, .7); background-color:gold;</span></p>
<p><span style="color:rgba( 0, 120, 250, 0.8);background-color:hsla(270, 100%, 50%, 0.3);">08. color:;background-color:hsla(270, 100%, 50%, 0.3);</span></p>
<p><span style="color:#ddd;background-color:hsl(150, 75%, 60%);">09. color:#ddd;background-color:hsl(150, 75%, 60%);</span></p>
<p><span style="color:hsl(270, 75%, 60%);background-color:#d82;">10. color:hsl(270, 75%, 60%);background-color:#d82;</span></p>
</div>

<div id="color-box" style="margin-top: 10px; border: 1px solid black; padding: 5px">
<p>You can use this helper to generate text with a particular color / background applied. Change inputs and accept it with enter key.</p>
<label>
Color:
<input type="text" name="color" id="color" placeholder="#3d3d3d">
</label>
<label>
Background Color:
<input type="text" name="bgcolor" id="bgcolor" placeholder="pink">
</label>
<p contenteditable="true"><span>Text for copying.</span></p>
</div>
59 changes: 59 additions & 0 deletions packages/ckeditor5-font/tests/manual/tickets/15580/1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
* @license Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved.
illia-stv marked this conversation as resolved.
Show resolved Hide resolved
* 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' ) );
5 changes: 5 additions & 0 deletions packages/ckeditor5-font/tests/manual/tickets/15580/1.md
Original file line number Diff line number Diff line change
@@ -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.
illia-stv marked this conversation as resolved.
Show resolved Hide resolved
63 changes: 60 additions & 3 deletions packages/ckeditor5-font/tests/ui/colorui.js
Original file line number Diff line number Diff line change
Expand Up @@ -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% )';

Expand All @@ -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';

Expand All @@ -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 );
Expand Down Expand Up @@ -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'
illia-stv marked this conversation as resolved.
Show resolved Hide resolved
]
} )
.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;
illia-stv marked this conversation as resolved.
Show resolved Hide resolved

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' } ) );
} );
} );
} );
} );