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

Various merge tags related improvements and adjustments. #16096

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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/ckeditor5-engine/src/view/domconverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ export default class DomConverter {
'td', 'tfoot', 'th', 'thead', 'tr', 'ul'
];
this.inlineObjectElements = [
'object', 'iframe', 'input', 'button', 'textarea', 'select', 'option', 'video', 'embed', 'audio', 'img', 'canvas'
'object', 'iframe', 'input', 'button', 'textarea', 'select', 'option', 'video', 'embed', 'audio', 'img', 'canvas', 'mergeTag'
Copy link
Contributor

@coderabbitai coderabbitai bot May 9, 2024

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Several files in the codebase still use the old function signature.

  • The search results indicate that 'mergeTag' is referenced in UI-related code but does not appear to be handled in a way that supports its behavior as an inline object element in domconverter.ts.
  • This could potentially lead to issues since the handling necessary to support its inclusion in the inlineObjectElements array does not seem to be implemented.
Analysis chain

Addition of 'mergeTag' to inlineObjectElements approved.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that 'mergeTag' is handled appropriately in other parts of the codebase.

# Test: Search for 'mergeTag' handling. Expect: Relevant handling logic in other parts of the code.
rg --type typescript 'mergeTag'

Length of output: 66



Script:

#!/bin/bash
# Description: Re-verify that 'mergeTag' is handled appropriately in other parts of the codebase by correcting the file type error in the previous script.

# Corrected Test: Search for 'mergeTag' handling in TypeScript files. Expect: Relevant handling logic in other parts of the code.
rg --type ts 'mergeTag'

Length of output: 511

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be injected from the feature code. It's public property. The property itself is read-only but the content is not.

See how we handle other similar props:

const unsafeElements = editor.editing.view.domConverter.unsafeElements;

// Make sure that this element will not render in the editing view.
if ( !unsafeElements.includes( viewElement.name ) ) {
unsafeElements.push( viewElement.name );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@niegowski, thank you for your input. The handling of 'mergeTag' as an inline object element in the DomConverter class is indeed crucial for ensuring it integrates smoothly with the rest of the system, especially given its role in the UI components.

From the examples you provided, it seems like the approach used in other parts of CKEditor 5 involves defining properties in a way that they are publicly accessible but their modification is controlled. This is typically achieved using getter/setter methods or similar patterns to encapsulate the property's value, ensuring it remains read-only externally while allowing its content to be modified internally.

Currently, 'mergeTag' is added directly to the inlineObjectElements array in the DomConverter class, which makes it part of the internal configuration for handling inline objects. This approach is straightforward but does not allow external manipulation or extension in the same way that a getter/setter pattern would.

If aligning the handling of 'mergeTag' with the approach you highlighted is desired, we could consider introducing a method to get and potentially set the contents of the inlineObjectElements array in a controlled manner. This would provide flexibility and maintain consistency with other parts of the codebase.

Would you like me to draft a potential implementation for this adjustment, or is there a specific aspect of the integration you would like to discuss further?

];
this.unsafeElements = [ 'script', 'style' ];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ describe( 'DomConverter – whitespace handling – integration', () => {
];

// Singletons don't have $text children.
const singletons = [ 'input', 'embed' ];
const singletons = [ 'input', 'embed', 'mergeTag' ];

for ( const name of elements ) {
it( `should work for the <${ name }> element`, () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* @license Copyright (c) 2003-2024, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module ui/highlightedtext/buttonlabelwithhighlightview
*/

import type ButtonLabel from '../button/buttonlabel.js';
import HighlightedTextView from './highlightedtextview.js';

/**
* A button label view that can highlight a text fragment.
*/
export default class ButtonLabelWithHighlightView extends HighlightedTextView implements ButtonLabel {
/**
* @inheritDoc
*/
declare public style: string | undefined;

/**
* @inheritDoc
*/
declare public text: string | undefined;

/**
* @inheritDoc
*/
declare public id: string | undefined;

/**
* @inheritDoc
*/
constructor() {
super();

this.set( {
style: undefined,
text: undefined,
id: undefined
} );

const bind = this.bindTemplate;

this.extendTemplate( {
attributes: {
class: [
'ck-button__label'
],
style: bind.to( 'style' ),
id: bind.to( 'id' )
}
} );
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* @license Copyright (c) 2003-2024, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module ui/highlightedtext/labelwithhighlightview
*/

import HighlightedTextView from './highlightedtextview.js';
import type LabelView from '../label/labelview.js';
import { uid } from '@ckeditor/ckeditor5-utils';

/**
* A label view that can highlight a text fragment.
*/
export default class LabelWithHighlightView extends HighlightedTextView implements LabelView {
/**
* @inheritDoc
*/
public readonly id: string;

/**
* @inheritDoc
*/
declare public for: string | undefined;

/**
* @inheritDoc
*/
constructor() {
super();

this.set( 'for', undefined );

const bind = this.bindTemplate;

this.id = `ck-editor__label_${ uid() }`;

this.extendTemplate( {
attributes: {
class: [
'ck',
'ck-label'
],
id: this.id,
for: bind.to( 'for' )
}
} );
}
}
3 changes: 3 additions & 0 deletions packages/ckeditor5-ui/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export { default as ListItemGroupView } from './list/listitemgroupview.js';
export { default as ListItemView } from './list/listitemview.js';
export { default as ListSeparatorView } from './list/listseparatorview.js';
export { default as ListView } from './list/listview.js';
export { default as filterGroupAndItemNames } from './search/filtergroupanditemnames.js';

export { default as Notification } from './notification/notification.js';

Expand All @@ -100,6 +101,8 @@ export { default as SearchTextView, type SearchTextViewSearchEvent, type SearchT
export { default as SearchInfoView } from './search/searchinfoview.js';
export type { default as FilteredView, FilteredViewExecuteEvent } from './search/filteredview.js';
export { default as HighlightedTextView } from './highlightedtext/highlightedtextview.js';
export { default as ButtonLabelWithHighlightView } from './highlightedtext/buttonlabelwithhighlightview.js';
export { default as LabelWithHighlightView } from './highlightedtext/labelwithhighlightview.js';

export { default as TooltipManager } from './tooltipmanager.js';
export { default as Template, type TemplateDefinition } from './template.js';
Expand Down
18 changes: 16 additions & 2 deletions packages/ckeditor5-ui/src/menubar/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,12 @@ export const MenuBarMenuViewPanelPositioningFunctions: Record<string, Positionin
* ]
* },
* {
* groupId: 'previewMergeTags',
* items: [
* 'menuBar:previewMergeTags'
* ]
* },
* {
* groupId: 'restrictedEditingException',
* items: [
* 'menuBar:restrictedEditingException'
Expand All @@ -514,7 +520,8 @@ export const MenuBarMenuViewPanelPositioningFunctions: Record<string, Positionin
* groupId: 'insertInline',
* items: [
* 'menuBar:link',
* 'menuBar:comment'
* 'menuBar:comment',
* 'menuBar:insertMergeTag'
* ]
* },
* {
Expand Down Expand Up @@ -737,6 +744,12 @@ export const DefaultMenuBarItems: DeepReadonly<MenuBarConfigObject[ 'items' ]> =
'menuBar:showBlocks'
]
},
{
groupId: 'previewMergeTags',
items: [
'menuBar:previewMergeTags'
]
},
{
groupId: 'restrictedEditingException',
items: [
Expand All @@ -762,7 +775,8 @@ export const DefaultMenuBarItems: DeepReadonly<MenuBarConfigObject[ 'items' ]> =
groupId: 'insertInline',
items: [
'menuBar:link',
'menuBar:comment'
'menuBar:comment',
'menuBar:insertMergeTag'
]
},
{
Expand Down
65 changes: 65 additions & 0 deletions packages/ckeditor5-ui/src/search/filtergroupanditemnames.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* @license Copyright (c) 2003-2024, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/**
* @module ui/search/filtergroupanditemnames
*/

import type ButtonView from '../button/buttonview.js';
import type ButtonLabelWithHighlightView from '../highlightedtext/buttonlabelwithhighlightview.js';
import type LabelWithHighlightView from '../highlightedtext/labelwithhighlightview.js';
import type ViewCollection from '../viewcollection.js';
import type ListItemGroupView from '../list/listitemgroupview.js';
import type ListItemView from '../list/listitemview.js';
import type ListSeparatorView from '../list/listseparatorview.js';

/**
* A filter function that returns matching item and group names in the list view.
*/
export default function filterGroupAndItemNames(
regExp: RegExp | null,
items: ViewCollection<ListItemGroupView | ListItemView | ListSeparatorView>
): {
resultsCount: number;
totalItemsCount: number;
} {
let totalItemsCount = 0;
let resultsCount = 0;

for ( const groupView of items ) {
const group = groupView as ListItemGroupView;
const groupItems = group.items as ViewCollection<ListItemView>;
const isGroupLabelMatching = regExp && !!group.label.match( regExp );

( group.labelView as LabelWithHighlightView ).highlightText( isGroupLabelMatching ? regExp : null );

for ( const listItemView of groupItems ) {
const buttonView = listItemView.children.first as ButtonView;
const labelView = buttonView.labelView as ButtonLabelWithHighlightView;

if ( !regExp ) {
listItemView.isVisible = true;
labelView.highlightText( null );
} else {
const isItemLabelMatching = !!buttonView.label!.match( regExp );

labelView.highlightText( isItemLabelMatching ? regExp : null );

listItemView.isVisible = isGroupLabelMatching || isItemLabelMatching;
}
}

const visibleInGroupCount = groupItems.filter( listItemView => listItemView.isVisible ).length;

totalItemsCount += group.items.length;
resultsCount += isGroupLabelMatching ? group.items.length : visibleInGroupCount;
group.isVisible = isGroupLabelMatching || !!visibleInGroupCount;
}

return {
resultsCount,
totalItemsCount
};
}
1 change: 1 addition & 0 deletions packages/ckeditor5-ui/src/search/text/searchtextview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ export default class SearchTextView<
public reset(): void {
this.queryView.reset();
this.search( '' );
this.filteredView.element!.scrollTo( 0, 0 );
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/**
* @license Copyright (c) 2003-2024, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

import { global } from '@ckeditor/ckeditor5-utils';
import HighlightedTextView from '../../src/highlightedtext/highlightedtextview.js';
import ButtonLabelWithHighlightView from '../../src/highlightedtext/buttonlabelwithhighlightview.js';

describe( 'ButtonLabelWithHighlightView', () => {
let view;

beforeEach( () => {
view = new ButtonLabelWithHighlightView( );
view.render();

global.document.body.appendChild( view.element );
} );

afterEach( () => {
view.element.remove();
view.destroy();
} );

describe( 'constructor()', () => {
it( 'should be HighlightedTextView instance', () => {
expect( view ).to.be.instanceof( HighlightedTextView );
} );

it( 'should set initial properties as undefined', () => {
expect( view.style ).to.equal( undefined );
expect( view.text ).to.equal( undefined );
expect( view.id ).to.equal( undefined );
} );
} );

describe( 'default template', () => {
it( 'should bind view#style to template style', () => {
view.style = 'color: red';

expect( view.element.getAttribute( 'style' ) ).to.equal( 'color: red' );

view.style = 'color: blue';

expect( view.element.getAttribute( 'style' ) ).to.equal( 'color: blue' );
} );

it( 'should bind view#text to template text', () => {
view.text = 'Test';

expect( view.element.textContent ).to.equal( 'Test' );

view.text = undefined;

expect( view.element.textContent ).to.equal( '' );
} );

it( 'should bind view#id to template id', () => {
view.id = 'test-id';

expect( view.element.getAttribute( 'id' ) ).to.equal( 'test-id' );

view.id = 'new-id';

expect( view.element.getAttribute( 'id' ) ).to.equal( 'new-id' );
} );
} );

describe( 'Highlighting template text', () => {
beforeEach( () => {
view.text = 'Example text';
} );

it( 'should not highlight anything when no query is specified', () => {
view.highlightText( null );

expect( view.element.innerHTML ).to.equal( 'Example text' );
} );

it( 'should highlight the query', () => {
view.highlightText( new RegExp( /text/, 'ig' ) );

expect( view.element.innerHTML ).to.equal( 'Example <mark>text</mark>' );
} );

it( 'should highlight multiple occurences of the query', () => {
view.highlightText( new RegExp( /e/, 'ig' ) );

expect( view.element.innerHTML ).to.equal(
'<mark>E</mark>xampl<mark>e</mark> t<mark>e</mark>xt'
);
} );
} );
} );