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

Fix font color picker not working properly on first change on Chrome #17109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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