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.

Sign up for GitHub

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

Jump to bottom

Group changes into one batch #14060

Merged
merged 10 commits into from
May 8, 2023
Merged

Group changes into one batch #14060

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a98e4bf
implement grouping changes into one batch
illia-stv May 5, 2023
1b5d7d4
debug commit
illia-stv May 8, 2023
475b782
back to 100% coverage
illia-stv May 8, 2023
dd4a44a
Merge branch 'ck/epic/2356-color-picker' into ck/3162-group-changes-i…
mlewand May 8, 2023
a21131a
Adjust manual tests.
Dumluregn May 8, 2023
b7121b6
code review fixes
illia-stv May 8, 2023
c16294b
code review fixes
illia-stv May 8, 2023
e33f0c7
fix typo
illia-stv May 8, 2023
d2d07a5
Small amendments.
Dumluregn May 8, 2023
c899f4b
Merge branch 'ck/epic/2356-color-picker' into ck/3162-group-changes-i…
Dumluregn May 8, 2023
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
1 change: 1 addition & 0 deletions packages/ckeditor5-font/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"@ckeditor/ckeditor5-ui": "^37.1.0"
},
"devDependencies": {
"@ckeditor/ckeditor5-undo": "^37.1.0",
"@ckeditor/ckeditor5-core": "^37.1.0",
"@ckeditor/ckeditor5-dev-utils": "^37.0.0",
"@ckeditor/ckeditor5-editor-classic": "^37.1.0",
Expand Down
6 changes: 4 additions & 2 deletions packages/ckeditor5-font/src/fontcommand.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import { Command, type Editor } from 'ckeditor5/src/core';
import { type Batch } from 'ckeditor5/src/engine';

/**
* The base font command.
Expand Down Expand Up @@ -57,14 +58,15 @@ export default abstract class FontCommand extends Command {
* @param options.value The value to apply.
* @fires execute
*/
public override execute( options: { value?: string } = {} ): void {
public override execute( options: { value?: string; batch?: Batch } = {} ): void {
const model = this.editor.model;
const document = model.document;
const selection = document.selection;

const value = options.value;
const batch = options.batch;

model.change( writer => {
model.enqueueChange( batch, writer => {
if ( selection.isCollapsed ) {
if ( value ) {
writer.setSelectionAttribute( this.attributeKey, value );
Expand Down
2 changes: 2 additions & 0 deletions packages/ckeditor5-font/src/ui/colortableview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ export default class ColorTableView extends View {
* Show "Color picker" and hide "Color grids".
*/
public showColorPicker(): void {
this.fire( 'showColorPicker' );
illia-stv marked this conversation as resolved.
Show resolved Hide resolved

if ( !this.colorPickerPageView.colorPickerView ) {
return;
}
Expand Down
27 changes: 17 additions & 10 deletions packages/ckeditor5-font/src/ui/colorui.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import { Plugin, type Editor } from 'ckeditor5/src/core';
import type { Batch } from 'ckeditor5/src/engine';
import { createDropdown, normalizeColorOptions, getLocalizedColorOptions, focusChildOnDropdownOpen } from 'ckeditor5/src/ui';

import {
Expand Down Expand Up @@ -63,6 +64,11 @@ export default class ColorUI extends Plugin {
*/
public colorTableView: ColorTableView | undefined;

/**
* Keeps all changes of color picker in one batch.
illia-stv marked this conversation as resolved.
Show resolved Hide resolved
*/
declare private _undoStepBatch: Batch;

/**
* Creates a plugin which introduces a dropdown with a pre–configured {@link module:font/ui/colortableview~ColorTableView}.
*
Expand Down Expand Up @@ -111,7 +117,6 @@ export default class ColorUI extends Plugin {
const dropdownView: ColorTableDropdownView = createDropdown( locale );
// Font color dropdown rendering is deferred once it gets open to improve performance (#6192).
let dropdownContentRendered = false;
let colorSavedUponDropdownOpen: string;

this.colorTableView = addColorTableToDropdown( {
dropdownView,
Expand Down Expand Up @@ -148,19 +153,25 @@ export default class ColorUI extends Plugin {

this.colorTableView.on<ColorTableExecuteEvent>( 'execute', ( evt, data ) => {
if ( dropdownView.isOpen ) {
editor.execute( this.commandName, data );
editor.execute( this.commandName, {
value: data.value,
batch: this._undoStepBatch
} );
}

if ( data.source !== 'colorPicker' ) {
editor.editing.view.focus();
}
} );

this.colorTableView.on( 'showColorPicker', () => {
this._undoStepBatch = editor.model.createBatch();
} );

this.colorTableView.on<ColorTableCancelEvent>( 'cancel', () => {
editor.execute( this.commandName, {
value: colorSavedUponDropdownOpen
} );
this.colorTableView!.selectedColor = colorSavedUponDropdownOpen;
if ( this._undoStepBatch!.operations.length ) {
editor.execute( 'undo', this._undoStepBatch );
}

editor.editing.view.focus();
} );
Expand All @@ -173,10 +184,6 @@ export default class ColorUI extends Plugin {
}

if ( isVisible ) {
if ( hasColorPicker ) {
colorSavedUponDropdownOpen = dropdownView.colorTableView!.selectedColor!;
}

if ( documentColorsCount !== 0 ) {
this.colorTableView!.updateDocumentColors( editor.model, this.componentName );
}
Expand Down
13 changes: 5 additions & 8 deletions packages/ckeditor5-font/tests/fontcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,16 +224,13 @@ describe( 'FontCommand', () => {
);
} );

it( 'should use parent batch for storing undo steps', () => {
it( 'should use provided batch', () => {
setData( model, '<paragraph>a[bc<$text font="foo">fo]obar</$text>xyz</paragraph>' );
const batch = model.createBatch();
const spy = sinon.spy( model, 'enqueueChange' );

model.change( writer => {
expect( writer.batch.operations.length ).to.equal( 0 );
command.execute( { value: 'foo' } );
expect( writer.batch.operations.length ).to.equal( 1 );
} );

expect( getData( model ) ).to.equal( '<paragraph>a[<$text font="foo">bcfo]obar</$text>xyz</paragraph>' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having both tests (the old one and new one) makes sense - depending on whether you've passed the batch as a parameter.

command.execute( { value: '#f00', batch } );
sinon.assert.calledWith( spy, batch );
} );

describe( 'should cause firing model change event', () => {
Expand Down
57 changes: 57 additions & 0 deletions packages/ckeditor5-font/tests/manual/font-color-picker.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<h2>Editor with default color picker</h2>

<div id="editor-with-default-picker">
<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>

<h2>Editor with color picker output set to "rgb" for font color "hex" for background</h2>

<div id="editor-with-custom-picker">
<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>

<h2>Editor without color picker</h2>

<div id="editor-without-picker">
<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>
134 changes: 134 additions & 0 deletions packages/ckeditor5-font/tests/manual/font-color-picker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/**
* @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';
import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset';
import FontColor from '../../src/fontcolor';
import FontBackgroundColor from '../../src/fontbackgroundcolor';

ClassicEditor
.create( document.querySelector( '#editor-with-default-picker' ), {
image: { toolbar: [ 'toggleImageCaption', 'imageTextAlternative' ] },
plugins: [
ArticlePluginSet,
FontColor,
FontBackgroundColor
],
toolbar: [
'heading',
'|',
'fontColor',
'fontBackgroundColor',
'bold',
'italic',
'link',
'bulletedList',
'numberedList',
'blockQuote',
'undo',
'redo'
],
fontColor: {
columns: 3
}
} )
.then( editor => {
window.editor = editor;
} )
.catch( err => {
console.error( err.stack );
} );

ClassicEditor
.create( document.querySelector( '#editor-with-custom-picker' ), {
image: { toolbar: [ 'toggleImageCaption', 'imageTextAlternative' ] },
plugins: [
ArticlePluginSet,
FontColor,
FontBackgroundColor
],
toolbar: [
'heading',
'|',
'fontColor',
'fontBackgroundColor',
'bold',
'italic',
'link',
'bulletedList',
'numberedList',
'blockQuote',
'undo',
'redo'
],
fontColor: {
columns: 3,
colorPicker: {
format: 'rgb'
}
},
fontBackgroundColor: {
colorPicker: {
format: 'hex'
}
}
} )
.then( editor => {
window.editor = editor;
} )
.catch( err => {
console.error( err.stack );
} );

ClassicEditor
.create( document.querySelector( '#editor-without-picker' ), {
image: { toolbar: [ 'toggleImageCaption', 'imageTextAlternative' ] },
plugins: [
ArticlePluginSet,
FontColor,
FontBackgroundColor
],
toolbar: [
'heading',
'|',
'fontColor',
'fontBackgroundColor',
'bold',
'italic',
'link',
'bulletedList',
'numberedList',
'blockQuote',
'undo',
'redo'
],
fontColor: {
columns: 3,
colorPicker: false
},
fontBackgroundColor: {
colorPicker: false
}
} )
.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' ) );
23 changes: 23 additions & 0 deletions packages/ckeditor5-font/tests/manual/font-color-picker.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## Color picker manual test

### Description

This manual test contains 3 editors:
* the first one has the default color picker config;
* the second one has the output colors set to `rgb` for font color and `hex` for background color;
* the last one has color picker disabled.

### Testing

To test the feature, ensure that:
* The "Color picker" button is visible by default.
* Clicking the "Color picker" button toggles the dropdown view to color picker.
* Upon color picker opening, the current selection color is set in the palette and color input (converted to the `hex` format).
* Colors set through the color picker are applied in the model and data in the right formats.
* Font and font background color configs are independent.
* "Save" button applies changes and hides the dropdown.
* "Cancel" button cancels the changes done through the color picker.
* Color input only accepts colors in `hex` format (both with or without `#` at the beginning).
* Multiple color changes through the color picker are batched in one undo step.
* Keyboard navigation (with `tab` and arrows) works (all items in color picker view are accessible).
* Focus is clearly indicated on all focusable items.
19 changes: 1 addition & 18 deletions packages/ckeditor5-font/tests/manual/font-color.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,4 @@
<h2>Editor with color picker</h2>

<div id="editor-with-picker">
<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>

<h2>Editor without color picker</h2>

<div id="editor-without-picker">
<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>
Expand Down
Loading