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

Improvements in insertImage command, tooltip manager and mention #17177

Open
wants to merge 45 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
cac71a4
Add possibility to break the current block by InsertImageCommand.
Dumluregn Sep 27, 2024
41387cb
Add necessary export.
Dumluregn Oct 11, 2024
739511d
Add unit test and docs.
Dumluregn Oct 11, 2024
f9f0850
Update packages/ckeditor5-ui/src/tooltipmanager.ts
Dumluregn Oct 18, 2024
edea934
Change the matcher for instant tooltips from class to attribute.
Dumluregn Oct 18, 2024
f9c7020
Hide tooltip without delay for instant tooltips.
Dumluregn Oct 18, 2024
eb0337e
Updated sections related to translating the project.
psmyrek Oct 21, 2024
3a53827
Docs: minor fixes. [short flow]
godai78 Oct 22, 2024
09cf884
Introduced `translations:synchronize` script to synchronize translati…
psmyrek Oct 7, 2024
ab645f4
Introduced `translations:validate` script to validate translations ag…
psmyrek Oct 11, 2024
db85afe
Added support for skipping adding the license header to the newly cre…
psmyrek Oct 11, 2024
e78b6bc
Aligned to `ckeditor5-dev` changes in translations.
psmyrek Oct 11, 2024
762e890
Temporarily use `ci/3804` branch that includes changes from `ckeditor…
psmyrek Oct 14, 2024
1a02c54
Use `"Image"` text id from another package.
psmyrek Oct 14, 2024
2002b8d
Added missing language files with empty translations.
psmyrek Oct 14, 2024
6d788cf
Revert back the `@ckeditor/ckeditor5-dev-translations` to v44.0.0.
psmyrek Oct 14, 2024
9a3e49c
Fixed language contexts and moved related translations between packages.
psmyrek Oct 15, 2024
c85ae1c
Introduced `translations:move` script to move requested translations …
psmyrek Oct 16, 2024
ef06e6d
Align translations to changes in `ckeditor5-dev-*`.
psmyrek Oct 18, 2024
b5d9aae
Added support for `--packages` flag in `translations:synchronize` scr…
psmyrek Oct 21, 2024
cd29502
Extracted `replaceKebabCaseWithCamelCase()` to a separate util.
psmyrek Oct 21, 2024
41611d2
Use the latest `ckeditor5-dev-*` packages.
pomek Oct 22, 2024
769339f
Aligned plural forms.
psmyrek Oct 22, 2024
e0db2a6
Made the page unscrollable while the modal is visible.
oleq Oct 9, 2024
a157d00
Tests: Updated a test description.
oleq Oct 9, 2024
888ad22
Tests: Addressed an issue in the dialog scroll lock test.
oleq Oct 17, 2024
1d953eb
Tests: Added unit tests.
oleq Oct 17, 2024
12fc4cd
Disabled viewportOffset positioning restrictions for modal windows.
oleq Oct 21, 2024
4916e17
Disabled dragging support for modal windows.
oleq Oct 21, 2024
ecd32a8
Allowed for linking FocusTracker instances together and propagate the…
oleq Oct 2, 2024
7b69965
Addressed an issue with one FocusTracking regarding another as focus …
oleq Oct 3, 2024
71b7396
Docs.
oleq Oct 3, 2024
0688973
Made sure that grouped items toolbar can host a dropdown menu compone…
oleq Oct 11, 2024
0ada588
Docs: Applied review suggestions.
oleq Oct 17, 2024
449ff93
Code refactoring.
oleq Oct 17, 2024
c3a6cd0
Docs.
oleq Oct 17, 2024
d743bd8
Removed debug log properties.
oleq Oct 22, 2024
5725a66
Docs: Improved API docs in the FocusTracker class.
oleq Oct 22, 2024
6e0cdb7
Introduce automated manual test sample for performance testing (#17205)
scofalik Oct 22, 2024
01affe0
Internal (ui): Moved the document scroll lock for modal windows from …
oleq Oct 22, 2024
605d6f5
Merge branch 'master' into cc/image-merge-fields
Dumluregn Oct 23, 2024
cdc0e79
Allow for mentions markers to be longer than 1 character.
Dumluregn Oct 28, 2024
4b249d2
Adjusted a comment. [skip ci]
Dumluregn Oct 28, 2024
11e0556
Move a condition to the else block.
Dumluregn Oct 28, 2024
e427db6
Merge pull request #17335 from ckeditor/cc/6443-matcher-does-not-matc…
Dumluregn Nov 4, 2024
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
4 changes: 4 additions & 0 deletions packages/ckeditor5-image/src/image/insertimagecommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,14 @@ export default class InsertImageCommand extends Command {
* @param options Options for the executed command.
* @param options.imageType The type of the image to insert. If not specified, the type will be determined automatically.
* @param options.source The image source or an array of image sources to insert.
* @param options.breakBlock If set to `true`, the block at the selection start will be broken before inserting the image.
* See the documentation of the command to learn more about accepted formats.
*/
public override execute(
options: {
source: ArrayOrItem<string | Record<string, unknown>>;
imageType?: 'imageBlock' | 'imageInline' | null;
breakBlock?: boolean;
}
): void {
const sourceDefinitions = toArray<string | Record<string, unknown>>( options.source );
Expand Down Expand Up @@ -132,6 +134,8 @@ export default class InsertImageCommand extends Command {
const position = this.editor.model.createPositionAfter( selectedElement );

imageUtils.insertImage( { ...sourceDefinition, ...selectionAttributes }, position, options.imageType );
} else if ( options.breakBlock ) {
imageUtils.insertImage( { ...sourceDefinition, ...selectionAttributes }, selection.getFirstPosition(), options.imageType );
} else {
imageUtils.insertImage( { ...sourceDefinition, ...selectionAttributes }, null, options.imageType );
}
Expand Down
1 change: 1 addition & 0 deletions packages/ckeditor5-image/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export { default as ImageCaptionUI } from './imagecaption/imagecaptionui.js';
export { createImageTypeRegExp } from './imageupload/utils.js';

export type { ImageConfig } from './imageconfig.js';
export type { ImageLoadedEvent } from './image/imageloadobserver.js';
export type { default as ImageTypeCommand } from './image/imagetypecommand.js';
export type { default as InsertImageCommand } from './image/insertimagecommand.js';
export type { default as ReplaceImageSourceCommand } from './image/replaceimagesourcecommand.js';
Expand Down
16 changes: 16 additions & 0 deletions packages/ckeditor5-image/tests/image/insertimagecommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,22 @@ describe( 'InsertImageCommand', () => {
);
} );

it( 'should be possible to break the block with an inserted image', () => {
const imgSrc = 'foo/bar.jpg';

setModelData( model, '<paragraph>f[]oo</paragraph>' );

command.execute( {
imageType: 'imageBlock',
source: imgSrc,
breakBlock: true
} );

expect( getModelData( model ) ).to.equal(
`<paragraph>f</paragraph>[<imageBlock src="${ imgSrc }"></imageBlock>]<paragraph>oo</paragraph>`
);
} );

it( 'should insert multiple images at selection position as other widgets for inline type images', () => {
const imgSrc1 = 'foo/bar.jpg';
const imgSrc2 = 'foo/baz.jpg';
Expand Down
22 changes: 2 additions & 20 deletions packages/ckeditor5-mention/src/mentioncommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,27 +113,9 @@ export default class MentionCommand extends Command {

const mention = _addMentionAttributes( { _text: mentionText, id: mentionID }, mentionData );

if ( options.marker.length != 1 ) {
if ( !mentionID.startsWith( options.marker ) ) {
/**
* The marker must be a single character.
*
* Correct markers: `'@'`, `'#'`.
*
* Incorrect markers: `'@@'`, `'[@'`.
*
* See {@link module:mention/mentionconfig~MentionConfig}.
*
* @error mentioncommand-incorrect-marker
*/
throw new CKEditorError(
'mentioncommand-incorrect-marker',
this
);
}

if ( mentionID.charAt( 0 ) != options.marker ) {
/**
* The feed item ID must start with the marker character.
* The feed item ID must start with the marker character(s).
*
* Correct mention feed setting:
*
Expand Down
10 changes: 5 additions & 5 deletions packages/ckeditor5-mention/src/mentionui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -722,12 +722,12 @@ export function createRegExp( marker: string, minimumCharacters: number ): RegEx
// The pattern consists of 3 groups:
//
// - 0 (non-capturing): Opening sequence - start of the line, space or an opening punctuation character like "(" or "\"",
// - 1: The marker character,
// - 1: The marker character(s),
// - 2: Mention input (taking the minimal length into consideration to trigger the UI),
//
// The pattern matches up to the caret (end of string switch - $).
// (0: opening sequence )(1: marker )(2: typed mention )$
const pattern = `(?:^|[ ${ openAfterCharacters }])([${ marker }])(${ mentionCharacters }${ numberOfCharacters })$`;
// (0: opening sequence )(1: marker )(2: typed mention )$
const pattern = `(?:^|[ ${ openAfterCharacters }])(${ marker })(${ mentionCharacters }${ numberOfCharacters })$`;

return new RegExp( pattern, 'u' );
}
Expand Down Expand Up @@ -822,8 +822,8 @@ function isMarkerInExistingMention( markerPosition: Position ): boolean | null {
/**
* Checks if string is a valid mention marker.
*/
function isValidMentionMarker( marker: string ): boolean | string {
return marker && marker.length == 1;
function isValidMentionMarker( marker: string ): boolean {
return !!marker;
}

/**
Expand Down
13 changes: 0 additions & 13 deletions packages/ckeditor5-mention/tests/mentioncommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,19 +151,6 @@ describe( 'MentionCommand', () => {
expect( textNode.hasAttribute( 'bold' ) ).to.be.true;
} );

it( 'should throw if marker is not one character', () => {
setData( model, '<paragraph>foo @Jo[]bar</paragraph>' );

const testCases = [
{ marker: '##', mention: '##foo' },
{ marker: '', mention: '@foo' }
];

for ( const options of testCases ) {
expectToThrowCKEditorError( () => command.execute( options ), /mentioncommand-incorrect-marker/, editor );
}
} );

it( 'should throw if marker does not match mention id', () => {
setData( model, '<paragraph>foo @Jo[]bar</paragraph>' );

Expand Down
60 changes: 52 additions & 8 deletions packages/ckeditor5-mention/tests/mentionui.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,8 @@ describe( 'MentionUI', () => {
} );
} );

it( 'should throw if marker is longer then 1 character', () => {
return createClassicTestEditor( { feeds: [ { marker: '$$', feed: [ 'a' ] } ] } ).catch( error => {
assertCKEditorError( error, /mentionconfig-incorrect-marker/, null, { marker: '$$' } );
} );
it( 'should not throw if marker is longer then 1 character', () => {
expect( () => createClassicTestEditor( { feeds: [ { marker: '$$', feed: [ 'a' ] } ] } ) ).to.not.throw();
} );
} );

Expand Down Expand Up @@ -401,28 +399,35 @@ describe( 'MentionUI', () => {
env.features.isRegExpUnicodePropertySupported = false;
createRegExp( '@', 2 );
sinon.assert.calledOnce( regExpStub );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\(\\[{"\'])([@])(.{2,})$', 'u' );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\(\\[{"\'])(@)(.{2,})$', 'u' );
} );

it( 'returns a ES2018 RegExp for browsers supporting Unicode punctuation groups', () => {
env.features.isRegExpUnicodePropertySupported = true;
createRegExp( '@', 2 );
sinon.assert.calledOnce( regExpStub );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\p{Ps}\\p{Pi}"\'])([@])(.{2,})$', 'u' );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\p{Ps}\\p{Pi}"\'])(@)(.{2,})$', 'u' );
} );

it( 'returns a proper regexp for markers longer than 1 character', () => {
env.features.isRegExpUnicodePropertySupported = true;
createRegExp( '@@', 2 );
sinon.assert.calledOnce( regExpStub );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\p{Ps}\\p{Pi}"\'])(@@)(.{2,})$', 'u' );
} );

it( 'correctly escapes passed marker #1', () => {
env.features.isRegExpUnicodePropertySupported = true;
createRegExp( ']', 2 );
sinon.assert.calledOnce( regExpStub );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\p{Ps}\\p{Pi}"\'])([\\]])(.{2,})$', 'u' );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\p{Ps}\\p{Pi}"\'])(\\])(.{2,})$', 'u' );
} );

it( 'correctly escapes passed marker #2', () => {
env.features.isRegExpUnicodePropertySupported = true;
createRegExp( '\\', 2 );
sinon.assert.calledOnce( regExpStub );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\p{Ps}\\p{Pi}"\'])([\\\\])(.{2,})$', 'u' );
sinon.assert.calledWithExactly( regExpStub, '(?:^|[ \\p{Ps}\\p{Pi}"\'])(\\\\)(.{2,})$', 'u' );
} );
} );

Expand Down Expand Up @@ -470,6 +475,45 @@ describe( 'MentionUI', () => {
} );
} );

it( 'should show panel after the whole marker is matched', () => {
return createClassicTestEditor( {
feeds: [ { marker: '@@', feed: [ '@Barney', '@Lily', '@Marshall', '@Robin', '@Ted' ] } ]
} )
.then( () => {
setData( editor.model, '<paragraph>foo []</paragraph>' );

model.change( writer => {
writer.insertText( '@', doc.selection.getFirstPosition() );
} );
} )
.then( waitForDebounce )
.then( () => {
expect( panelView.isVisible ).to.be.false;
expect( editor.model.markers.has( 'mention' ) ).to.be.false;
} )
.then( () => {
model.change( writer => {
writer.insertText( '@', doc.selection.getFirstPosition() );
} );
} )
.then( waitForDebounce )
.then( () => {
expect( panelView.isVisible ).to.be.true;
expect( editor.model.markers.has( 'mention' ) ).to.be.true;
expect( mentionsView.items ).to.have.length( 5 );

model.change( writer => {
writer.insertText( 't', doc.selection.getFirstPosition() );
} );
} )
.then( waitForDebounce )
.then( () => {
expect( panelView.isVisible ).to.be.true;
expect( editor.model.markers.has( 'mention' ) ).to.be.true;
expect( mentionsView.items ).to.have.length( 1 );
} );
} );

it( 'should update the marker if the selection was moved from one valid position to another', () => {
const spy = sinon.spy();

Expand Down
34 changes: 31 additions & 3 deletions packages/ckeditor5-ui/src/tooltipmanager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,21 @@ const BALLOON_CLASS = 'ck-tooltip';
*
* # Disabling tooltips
*
* In order to disable the tooltip temporarily, use the `data-cke-tooltip-disabled` attribute:
* In order to disable the tooltip temporarily, use the `data-cke-tooltip-disabled` attribute:
*
* ```ts
* domElement.dataset.ckeTooltipText = 'Disabled. For now.';
* domElement.dataset.ckeTooltipDisabled = 'true';
* ```
*
* # Instant tooltips
*
* To remove the delay before showing or hiding the tooltip, use the `data-cke-tooltip-instant` attribute:
*
* ```ts
* domElement.dataset.ckeTooltipInstant = 'true';
* ```
*
* # Styling tooltips
*
* By default, the tooltip has `.ck-tooltip` class and its text inner `.ck-tooltip__text`.
Expand Down Expand Up @@ -294,6 +302,8 @@ export default class TooltipManager extends /* #__PURE__ */ DomEmitterMixin() {
// * a tooltip is displayed for a focused element, then the same element gets mouseentered,
// * a tooltip is displayed for an element via mouseenter, then the focus moves to the same element.
if ( elementWithTooltipAttribute === this._currentElementWithTooltip ) {
this._unpinTooltipDebounced.cancel();

return;
}

Expand All @@ -302,7 +312,13 @@ export default class TooltipManager extends /* #__PURE__ */ DomEmitterMixin() {
// The tooltip should be pinned immediately when the element gets focused using keyboard.
// If it is focused using the mouse, the tooltip should be pinned after a delay to prevent flashing.
// See https://github.com/ckeditor/ckeditor5/issues/16383
if ( evt.name === 'focus' && !elementWithTooltipAttribute.matches( ':hover' ) ) {
// Also, if the element has an attribute `data-cke-tooltip-instant`, the tooltip should be pinned immediately.
// This is useful for elements that have their content partially hidden (e.g. a long text in a small container)
// and should show a tooltip on hover, like merge field.
if (
evt.name === 'focus' && !elementWithTooltipAttribute.matches( ':hover' ) ||
elementWithTooltipAttribute.matches( '[data-cke-tooltip-instant]' )
) {
this._pinTooltip( elementWithTooltipAttribute, getTooltipData( elementWithTooltipAttribute ) );
} else {
this._pinTooltipDebounced( elementWithTooltipAttribute, getTooltipData( elementWithTooltipAttribute ) );
Expand All @@ -329,6 +345,7 @@ export default class TooltipManager extends /* #__PURE__ */ DomEmitterMixin() {
// Do not hide the tooltip when the user moves the cursor over it.
if ( isEnteringBalloon ) {
this._unpinTooltipDebounced.cancel();

return;
}

Expand All @@ -347,7 +364,17 @@ export default class TooltipManager extends /* #__PURE__ */ DomEmitterMixin() {
// Note that this should happen whether the tooltip is already visible or not, for instance,
// it could be invisible but queued (debounced): it should get canceled.
if ( isLeavingBalloon || ( descendantWithTooltip && descendantWithTooltip !== relatedDescendantWithTooltip ) ) {
this._unpinTooltipDebounced();
this._pinTooltipDebounced.cancel();

// If the currently visible tooltip is instant, unpin it immediately.
if (
this._currentElementWithTooltip && this._currentElementWithTooltip.matches( '[data-cke-tooltip-instant]' ) ||
descendantWithTooltip && descendantWithTooltip.matches( '[data-cke-tooltip-instant]' )
) {
this._unpinTooltip();
} else {
this._unpinTooltipDebounced();
}
}
} else {
// If a tooltip is currently visible, don't act for a targets other than the one it is attached to.
Expand All @@ -358,6 +385,7 @@ export default class TooltipManager extends /* #__PURE__ */ DomEmitterMixin() {

// Note that unpinning should happen whether the tooltip is already visible or not, for instance, it could be invisible but
// queued (debounced): it should get canceled (e.g. quick focus then quick blur using the keyboard).
this._pinTooltipDebounced.cancel();
this._unpinTooltipDebounced();
}
}
Expand Down
25 changes: 25 additions & 0 deletions packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,18 @@ describe( 'TooltipManager', () => {
} );
} );

it( 'should pin a tooltip instantly if element has a `data-cke-tooltip-instant` attribute', () => {
elements.a.dataset.ckeTooltipInstant = true;

utils.dispatchMouseEnter( elements.a );

sinon.assert.calledOnce( pinSpy );
sinon.assert.calledWith( pinSpy, {
target: elements.a,
positions: sinon.match.array
} );
} );

it( 'should pin just a single tooltip (singleton)', async () => {
const secondEditor = await ClassicTestEditor.create( element, {
plugins: [ Paragraph, Bold, Italic ],
Expand Down Expand Up @@ -700,6 +712,19 @@ describe( 'TooltipManager', () => {
sinon.assert.calledOnce( unpinSpy );
} );

it( 'should remove the tooltip immediately if the element has `data-cke-tooltip-instant` attribute', () => {
elements.a.dataset.ckeTooltipInstant = true;

utils.dispatchMouseEnter( elements.a );

sinon.assert.calledOnce( pinSpy );

unpinSpy = sinon.spy( tooltipManager.balloonPanelView, 'unpin' );
utils.dispatchMouseLeave( tooltipManager.balloonPanelView.element, elements.b );

sinon.assert.calledOnce( unpinSpy );
} );

it( 'should not work if the tooltip is currently pinned and the event target is different than the current element', () => {
utils.dispatchMouseEnter( elements.a );
utils.waitForTheTooltipToShow( clock );
Expand Down