Skip to content

Commit

Permalink
Fix bugy color picker behaviour when used the first time on Chrome.
Browse files Browse the repository at this point in the history
  • Loading branch information
Mati365 committed Sep 19, 2024
1 parent d4209ca commit d5c9199
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 2 deletions.
13 changes: 13 additions & 0 deletions packages/ckeditor5-font/tests/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,16 @@ describe( 'Integration test Font', () => {
} );

describe( 'color picker feature', () => {
let clock;

beforeEach( () => {
clock = sinon.useFakeTimers();
} );

afterEach( () => {
clock.restore();
} );

it( 'should set colors in model in hsl format by default', () => {
setModelData( model,
'<paragraph>' +
Expand All @@ -231,6 +241,7 @@ describe( 'Integration test Font', () => {
} );

dropdown.colorSelectorView.colorPickerFragmentView.colorPickerView.picker.dispatchEvent( event );
clock.tick( 200 );

expect( getData( model ) ).to.equal( '<paragraph>[<$text fontColor="hsl( 150, 50%, 13% )">foo</$text>]</paragraph>' );
} );
Expand Down Expand Up @@ -265,6 +276,7 @@ describe( 'Integration test Font', () => {
} );

dropdown.colorSelectorView.colorPickerFragmentView.colorPickerView.picker.dispatchEvent( event );
clock.tick( 200 );

expect( getData( editor.model ) ).to.equal( '<paragraph>[<$text fontColor="lab( 18% -17 7 )">foo</$text>]</paragraph>' );

Expand All @@ -285,6 +297,7 @@ describe( 'Integration test Font', () => {
dropdown.colorSelectorView.colorPickerFragmentView.colorPickerView.color = 'hsl( 100, 30%, 43% )';

dropdown.colorSelectorView.colorPickerFragmentView.cancelButtonView.fire( 'execute' );
clock.tick( 200 );

expect( getData( model ) ).to.equal( '<paragraph>' +
'[<$text fontColor="hsl( 50, 10%, 23% )">foo</$text><$text fontColor="hsl( 150, 50%, 13% )">foo</$text>]' +
Expand Down
22 changes: 21 additions & 1 deletion packages/ckeditor5-ui/src/colorpicker/colorpickerview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,29 @@ export default class ColorPickerView extends View {
this.picker.shadowRoot!.appendChild( styleSheetForFocusedColorPicker );
}

// Workaround for unstable selection after firing 'color-changed' event in Webkit-based browsers.
// The selection is being collapsed after the event is fired, causing the font color plugin
// not to update the selected editor content. Setting the color immediately after the event
// is fired is a workaround for this issue. It's not a perfect solution, but it's the best we can do for now.
// It may be removed in the future if the issue is fixed in the browser.
// See more: https://github.com/ckeditor/ckeditor5/issues/17069
let colorSelectedEventDebounce: number | null = null;

this.picker.addEventListener( 'color-changed', event => {
const color = event.detail.value;
this._debounceColorPickerEvent( color );

// At first, set the color internally in the component. It's converted to the configured output format.
this.set( 'color', color );

if ( colorSelectedEventDebounce !== null ) {
clearTimeout( colorSelectedEventDebounce );
}

// Let's wait until selection is being moved from web component to the editor.
colorSelectedEventDebounce = setTimeout( () => {
// Then let the outside world know that the user changed the color.
this.fire<ColorPickerColorSelectedEvent>( 'colorSelected', { color: this.color } );
}, 150 );
} );
}

Expand Down
25 changes: 24 additions & 1 deletion packages/ckeditor5-ui/tests/colorpicker/colorpickerview.js
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,32 @@ describe( 'ColorPickerView', () => {

view.picker.dispatchEvent( event );

clock.tick( 200 );
expect( view.color ).to.equal( '#ff0000' );
} );

// See more: https://github.com/ckeditor/ckeditor5/issues/17069
it( 'should debounce third party events due to selection issues', () => {
const colorChangedSpy = sinon.spy( view, 'fire' );
const makeColorEvent = color => new CustomEvent( 'color-changed', {
detail: {
value: color
}
} );

view.picker.dispatchEvent( makeColorEvent( '#ff0000' ) );
expect( view.color ).to.equal( '#ff0000' );

view.picker.dispatchEvent( makeColorEvent( '#00ff00' ) );
expect( view.color ).to.equal( '#00ff00' );

view.picker.dispatchEvent( makeColorEvent( '#0000ff' ) );
expect( view.color ).to.equal( '#0000ff' );

clock.tick( 400 );

expect( colorChangedSpy ).not.calledWithMatch( 'colorSelected', { color: '#ff0000' } );
expect( colorChangedSpy ).not.calledWithMatch( 'colorSelected', { color: '#00ff00' } );
expect( colorChangedSpy ).calledWithMatch( 'colorSelected', { color: '#0000ff' } );
} );

it( 'should render without input if "hideInput" is set on true', () => {
Expand Down

0 comments on commit d5c9199

Please sign in to comment.