From d5c9199ffca1473e1b528acbe614758eec0cdb9b Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Tue, 17 Sep 2024 14:31:05 +0200 Subject: [PATCH] Fix bugy color picker behaviour when used the first time on Chrome. --- packages/ckeditor5-font/tests/integration.js | 13 ++++++++++ .../src/colorpicker/colorpickerview.ts | 22 +++++++++++++++- .../tests/colorpicker/colorpickerview.js | 25 ++++++++++++++++++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-font/tests/integration.js b/packages/ckeditor5-font/tests/integration.js index 915eb9499de..437fa8c2749 100644 --- a/packages/ckeditor5-font/tests/integration.js +++ b/packages/ckeditor5-font/tests/integration.js @@ -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, '' + @@ -231,6 +241,7 @@ describe( 'Integration test Font', () => { } ); dropdown.colorSelectorView.colorPickerFragmentView.colorPickerView.picker.dispatchEvent( event ); + clock.tick( 200 ); expect( getData( model ) ).to.equal( '[<$text fontColor="hsl( 150, 50%, 13% )">foo]' ); } ); @@ -265,6 +276,7 @@ describe( 'Integration test Font', () => { } ); dropdown.colorSelectorView.colorPickerFragmentView.colorPickerView.picker.dispatchEvent( event ); + clock.tick( 200 ); expect( getData( editor.model ) ).to.equal( '[<$text fontColor="lab( 18% -17 7 )">foo]' ); @@ -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( '' + '[<$text fontColor="hsl( 50, 10%, 23% )">foo<$text fontColor="hsl( 150, 50%, 13% )">foo]' + diff --git a/packages/ckeditor5-ui/src/colorpicker/colorpickerview.ts b/packages/ckeditor5-ui/src/colorpicker/colorpickerview.ts index 9dfa07226c6..41c207381e8 100644 --- a/packages/ckeditor5-ui/src/colorpicker/colorpickerview.ts +++ b/packages/ckeditor5-ui/src/colorpicker/colorpickerview.ts @@ -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( 'colorSelected', { color: this.color } ); + }, 150 ); } ); } diff --git a/packages/ckeditor5-ui/tests/colorpicker/colorpickerview.js b/packages/ckeditor5-ui/tests/colorpicker/colorpickerview.js index f65dc01c92f..0b4beab6b7b 100644 --- a/packages/ckeditor5-ui/tests/colorpicker/colorpickerview.js +++ b/packages/ckeditor5-ui/tests/colorpicker/colorpickerview.js @@ -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', () => {