From 07a1c1c6c3a7a9af71d820b103ee8c812e3ae4ad Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Wed, 4 Dec 2024 09:31:19 +0100 Subject: [PATCH 1/9] Improve performance of placeholder and add `ghs` performance test. --- .../ckeditor5-engine/src/view/placeholder.ts | 19 ++++-- tests/_data/data-sets/ghs.js | 62 +++++++++++++++++++ tests/_data/data-sets/index.js | 16 +++-- 3 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 tests/_data/data-sets/ghs.js diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 9ab7ad6d0e4..34026e323b0 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -182,11 +182,7 @@ export function needsPlaceholder( element: Element, keepOnFocus: boolean ): bool return false; } - // Anything but uiElement(s) counts as content. - const hasContent = Array.from( element.getChildren() ) - .some( element => !element.is( 'uiElement' ) ); - - if ( hasContent ) { + if ( hasContent( element ) ) { return false; } @@ -212,6 +208,19 @@ export function needsPlaceholder( element: Element, keepOnFocus: boolean ): bool return !!selectionAnchor && selectionAnchor.parent !== element; } +/** + * Anything but uiElement(s) counts as content. + */ +function hasContent( element: Element ): boolean { + for ( const child of element.getChildren() ) { + if ( !child.is( 'uiElement' ) ) { + return true; + } + } + + return false; +} + /** * Updates all placeholders associated with a document in a post–fixer callback. * diff --git a/tests/_data/data-sets/ghs.js b/tests/_data/data-sets/ghs.js new file mode 100644 index 00000000000..85f5c9b358f --- /dev/null +++ b/tests/_data/data-sets/ghs.js @@ -0,0 +1,62 @@ +/** + * @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 + */ + +// This is main Wikipedia page source copied four times. This is to test content with a lot of messy / unsupported markup. +// After it is loaded in the editor, it is ~150 A4 pages (however there are a lot of very short paragraphs and list items). + +/* eslint-disable */ + +const initialData = + `

HTML playground

+
+
+

+ A sample paragraph with a data attribute. +

+
+
+

Responsive column layout

+
+
+
+ +
HTML
+
+
+
+
+ +
CSS
+
+
+
+
+ +
JS
+
+
+
+
+
+ +
+
`; + +export default function makeData() { + return initialData.repeat( 1000 ); +} diff --git a/tests/_data/data-sets/index.js b/tests/_data/data-sets/index.js index 32df0c204c6..79cae3fa7a3 100644 --- a/tests/_data/data-sets/index.js +++ b/tests/_data/data-sets/index.js @@ -3,14 +3,22 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import paragraphs from './paragraphs.js'; -import lists from './lists.js'; -import tableHuge from './table-huge.js'; import formattingLongP from './formatting-long-paragraphs.js'; +import ghs from './ghs.js'; import inlineStyles from './inline-styles.js'; +import lists from './lists.js'; import mixed from './mixed.js'; +import paragraphs from './paragraphs.js'; +import tableHuge from './table-huge.js'; import wiki from './wiki.js'; export default { - paragraphs, lists, tableHuge, formattingLongP, inlineStyles, mixed, wiki + formattingLongP, + ghs, + inlineStyles, + lists, + mixed, + paragraphs, + tableHuge, + wiki }; From 8099d921fe91369d9a9a79af13dbc5f5183dce98 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Tue, 10 Dec 2024 14:43:08 +0100 Subject: [PATCH 2/9] Improve performance of `enablePlaceholder` --- .../ckeditor5-engine/src/view/placeholder.ts | 65 ++++++++----------- packages/ckeditor5-image/src/image/utils.ts | 4 +- tests/_data/data-sets/ghs.js | 6 +- 3 files changed, 32 insertions(+), 43 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 34026e323b0..e5741ccbde0 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -50,24 +50,22 @@ export function enablePlaceholder( { view, element, text, isDirectHost = true, k } ): void { const doc = view.document; - // Use a single a single post fixer per—document to update all placeholders. + // Use a single post fixer per—document to update all placeholders. if ( !documentPlaceholders.has( doc ) ) { documentPlaceholders.set( doc, new Map() ); // If a post-fixer callback makes a change, it should return `true` so other post–fixers // can re–evaluate the document again. - doc.registerPostFixer( writer => updateDocumentPlaceholders( doc, writer ) ); + doc.registerPostFixer( writer => updateDocumentPlaceholders( documentPlaceholders.get( doc )!, writer ) ); // Update placeholders on isComposing state change since rendering is disabled while in composition mode. doc.on( 'change:isComposing', () => { - view.change( writer => updateDocumentPlaceholders( doc, writer ) ); + view.change( writer => updateDocumentPlaceholders( documentPlaceholders.get( doc )!, writer ) ); }, { priority: 'high' } ); } if ( element.is( 'editableElement' ) ) { - element.on( 'change:placeholder', ( evtInfo, evt, text ) => { - setPlaceholder( text ); - } ); + element.on( 'change:placeholder', ( evtInfo, evt, text ) => setPlaceholder( text ) ); } if ( element.placeholder ) { @@ -81,16 +79,18 @@ export function enablePlaceholder( { view, element, text, isDirectHost = true, k } function setPlaceholder( text: string ) { - // Store information about the element placeholder under its document. - documentPlaceholders.get( doc )!.set( element, { + const config = { text, isDirectHost, keepOnFocus, hostElement: isDirectHost ? element : null - } ); + }; + + // Store information about the element placeholder under its document. + documentPlaceholders.get( doc )!.set( element, config ); // Update the placeholders right away. - view.change( writer => updateDocumentPlaceholders( doc, writer ) ); + view.change( writer => updateDocumentPlaceholders( [ [ element, config ] ], writer ) ); } } @@ -226,44 +226,33 @@ function hasContent( element: Element ): boolean { * * @returns True if any changes were made to the view document. */ -function updateDocumentPlaceholders( doc: Document, writer: DowncastWriter ): boolean { - const placeholders = documentPlaceholders.get( doc )!; - const directHostElements: Array = []; +function updateDocumentPlaceholders( + placeholders: Iterable<[ Element, PlaceholderConfig ]>, + writer: DowncastWriter +): boolean { + const processedHostElements: Array = []; let wasViewModified = false; - // First set placeholders on the direct hosts. for ( const [ element, config ] of placeholders ) { if ( config.isDirectHost ) { - directHostElements.push( element ); + processedHostElements.push( element ); + } else { + const hostElement = getChildPlaceholderHostSubstitute( element ); - if ( updatePlaceholder( writer, element, config ) ) { - wasViewModified = true; + // When not a direct host, it could happen that there is no child element capable of displaying a placeholder. + if ( !hostElement ) { + continue; } - } - } - // Then set placeholders on the indirect hosts but only on those that does not already have an direct host placeholder. - for ( const [ element, config ] of placeholders ) { - if ( config.isDirectHost ) { - continue; - } - - const hostElement = getChildPlaceholderHostSubstitute( element ); - - // When not a direct host, it could happen that there is no child element - // capable of displaying a placeholder. - if ( !hostElement ) { - continue; - } + // Don't override placeholder if the host element already has some direct placeholder. + if ( processedHostElements.includes( hostElement ) ) { + continue; + } - // Don't override placeholder if the host element already has some direct placeholder. - if ( directHostElements.includes( hostElement ) ) { - continue; + // Update the host element (used for setting and removing the placeholder). + config.hostElement = hostElement; } - // Update the host element (used for setting and removing the placeholder). - config.hostElement = hostElement; - if ( updatePlaceholder( writer, element, config ) ) { wasViewModified = true; } diff --git a/packages/ckeditor5-image/src/image/utils.ts b/packages/ckeditor5-image/src/image/utils.ts index 9e97b5cd59c..7c10a0158ee 100644 --- a/packages/ckeditor5-image/src/image/utils.ts +++ b/packages/ckeditor5-image/src/image/utils.ts @@ -34,7 +34,7 @@ import type ImageUtils from '../imageutils.js'; */ export function createInlineImageViewElement( writer: DowncastWriter ): ViewContainerElement { return writer.createContainerElement( 'span', { class: 'image-inline' }, - writer.createEmptyElement( 'img' ) + writer.createEmptyElement( 'img', { loading: 'lazy' } ) ); } @@ -51,7 +51,7 @@ export function createInlineImageViewElement( writer: DowncastWriter ): ViewCont */ export function createBlockImageViewElement( writer: DowncastWriter ): ViewContainerElement { return writer.createContainerElement( 'figure', { class: 'image' }, [ - writer.createEmptyElement( 'img' ), + writer.createEmptyElement( 'img', { loading: 'lazy' } ), writer.createSlot( 'children' ) ] ); } diff --git a/tests/_data/data-sets/ghs.js b/tests/_data/data-sets/ghs.js index 85f5c9b358f..1d1af9e204c 100644 --- a/tests/_data/data-sets/ghs.js +++ b/tests/_data/data-sets/ghs.js @@ -21,19 +21,19 @@ const initialData =
- +
HTML
- +
CSS
- +
JS
From 35192d01c478f9c79cf58392c9b402e49605221b Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Thu, 12 Dec 2024 13:13:57 +0100 Subject: [PATCH 3/9] Add `loading="lazy"` only to images with `width` and `height` attributes --- packages/ckeditor5-image/src/image/utils.ts | 4 ++-- packages/ckeditor5-image/src/imagesizeattributes.ts | 1 + tests/_data/data-sets/ghs.js | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/ckeditor5-image/src/image/utils.ts b/packages/ckeditor5-image/src/image/utils.ts index 7c10a0158ee..9e97b5cd59c 100644 --- a/packages/ckeditor5-image/src/image/utils.ts +++ b/packages/ckeditor5-image/src/image/utils.ts @@ -34,7 +34,7 @@ import type ImageUtils from '../imageutils.js'; */ export function createInlineImageViewElement( writer: DowncastWriter ): ViewContainerElement { return writer.createContainerElement( 'span', { class: 'image-inline' }, - writer.createEmptyElement( 'img', { loading: 'lazy' } ) + writer.createEmptyElement( 'img' ) ); } @@ -51,7 +51,7 @@ export function createInlineImageViewElement( writer: DowncastWriter ): ViewCont */ export function createBlockImageViewElement( writer: DowncastWriter ): ViewContainerElement { return writer.createContainerElement( 'figure', { class: 'image' }, [ - writer.createEmptyElement( 'img', { loading: 'lazy' } ), + writer.createEmptyElement( 'img' ), writer.createSlot( 'children' ) ] ); } diff --git a/packages/ckeditor5-image/src/imagesizeattributes.ts b/packages/ckeditor5-image/src/imagesizeattributes.ts index 4c53f9e30d2..6ffc1315483 100644 --- a/packages/ckeditor5-image/src/imagesizeattributes.ts +++ b/packages/ckeditor5-image/src/imagesizeattributes.ts @@ -168,6 +168,7 @@ export default class ImageSizeAttributes extends Plugin { if ( width && height ) { viewWriter.setStyle( 'aspect-ratio', `${ width }/${ height }`, img ); + viewWriter.setAttribute( 'loading', 'lazy', img ); } } ); } diff --git a/tests/_data/data-sets/ghs.js b/tests/_data/data-sets/ghs.js index 1d1af9e204c..c6204401149 100644 --- a/tests/_data/data-sets/ghs.js +++ b/tests/_data/data-sets/ghs.js @@ -21,19 +21,19 @@ const initialData =
- +
HTML
- +
CSS
- +
JS
From 7057c399304c31ec80e634a61dafa4a42cfda092 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Thu, 12 Dec 2024 16:07:05 +0100 Subject: [PATCH 4/9] Make tests pass --- .../ckboximageedit/ckboximageeditcommand.js | 7 ++++-- .../ckeditor5-engine/src/view/placeholder.ts | 24 ++----------------- .../src/imagesizeattributes.ts | 16 +++++++++---- .../tests/imagesizeattributes.js | 12 ++++++---- 4 files changed, 26 insertions(+), 33 deletions(-) diff --git a/packages/ckeditor5-ckbox/tests/ckboximageedit/ckboximageeditcommand.js b/packages/ckeditor5-ckbox/tests/ckboximageedit/ckboximageeditcommand.js index 39b94370bb0..25fd5f9d405 100644 --- a/packages/ckeditor5-ckbox/tests/ckboximageedit/ckboximageeditcommand.js +++ b/packages/ckeditor5-ckbox/tests/ckboximageedit/ckboximageeditcommand.js @@ -951,7 +951,8 @@ describe( 'CKBoxImageEditCommand', () => { 'while waiting for the processed image', async () => { expect( getViewData( editor.editing.view, { withoutSelection: true } ) ).to.equal( '
' + - 'alt text' + + 'alt text' + + '' + '
' + '
' ); @@ -961,7 +962,9 @@ describe( 'CKBoxImageEditCommand', () => { expect( getViewData( editor.editing.view, { withoutSelection: true } ) ).to.equal( '
' + - 'alt text' + + 'alt text' + + '' + '
' + '
' ); diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index e5741ccbde0..5538492e204 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -83,7 +83,7 @@ export function enablePlaceholder( { view, element, text, isDirectHost = true, k text, isDirectHost, keepOnFocus, - hostElement: isDirectHost ? element : null + hostElement: isDirectHost ? element : getChildPlaceholderHostSubstitute( element ) }; // Store information about the element placeholder under its document. @@ -230,30 +230,10 @@ function updateDocumentPlaceholders( placeholders: Iterable<[ Element, PlaceholderConfig ]>, writer: DowncastWriter ): boolean { - const processedHostElements: Array = []; let wasViewModified = false; for ( const [ element, config ] of placeholders ) { - if ( config.isDirectHost ) { - processedHostElements.push( element ); - } else { - const hostElement = getChildPlaceholderHostSubstitute( element ); - - // When not a direct host, it could happen that there is no child element capable of displaying a placeholder. - if ( !hostElement ) { - continue; - } - - // Don't override placeholder if the host element already has some direct placeholder. - if ( processedHostElements.includes( hostElement ) ) { - continue; - } - - // Update the host element (used for setting and removing the placeholder). - config.hostElement = hostElement; - } - - if ( updatePlaceholder( writer, element, config ) ) { + if ( config.hostElement && updatePlaceholder( writer, element, config ) ) { wasViewModified = true; } } diff --git a/packages/ckeditor5-image/src/imagesizeattributes.ts b/packages/ckeditor5-image/src/imagesizeattributes.ts index 6ffc1315483..91ed625a48d 100644 --- a/packages/ckeditor5-image/src/imagesizeattributes.ts +++ b/packages/ckeditor5-image/src/imagesizeattributes.ts @@ -121,8 +121,8 @@ export default class ImageSizeAttributes extends Plugin { // Dedicated converters to propagate attributes to the element. editor.conversion.for( 'editingDowncast' ).add( dispatcher => { - attachDowncastConverter( dispatcher, 'width', 'width', true ); - attachDowncastConverter( dispatcher, 'height', 'height', true ); + attachDowncastConverter( dispatcher, 'width', 'width', true, true ); + attachDowncastConverter( dispatcher, 'height', 'height', true, true ); } ); editor.conversion.for( 'dataDowncast' ).add( dispatcher => { @@ -134,7 +134,8 @@ export default class ImageSizeAttributes extends Plugin { dispatcher: DowncastDispatcher, modelAttributeName: string, viewAttributeName: string, - setRatioForInlineImage: boolean + setRatioForInlineImage: boolean, + isEditingDowncast: boolean = false ) { dispatcher.on( `attribute:${ modelAttributeName }:${ imageType }`, ( evt, data, conversionApi ) => { if ( !conversionApi.consumable.consume( data.item, evt.name ) ) { @@ -166,8 +167,13 @@ export default class ImageSizeAttributes extends Plugin { const width = data.item.getAttribute( 'width' ); const height = data.item.getAttribute( 'height' ); - if ( width && height ) { - viewWriter.setStyle( 'aspect-ratio', `${ width }/${ height }`, img ); + if ( !width || !height ) { + return; + } + + viewWriter.setStyle( 'aspect-ratio', `${ width }/${ height }`, img ); + + if ( isEditingDowncast ) { viewWriter.setAttribute( 'loading', 'lazy', img ); } } ); diff --git a/packages/ckeditor5-image/tests/imagesizeattributes.js b/packages/ckeditor5-image/tests/imagesizeattributes.js index 3e282442531..6e980517ac3 100644 --- a/packages/ckeditor5-image/tests/imagesizeattributes.js +++ b/packages/ckeditor5-image/tests/imagesizeattributes.js @@ -290,7 +290,8 @@ describe( 'ImageSizeAttributes', () => { expect( getViewData( view, { withoutSelection: true } ) ).to.equal( '

' + - '' + + '' + + '' + '

' ); @@ -307,7 +308,8 @@ describe( 'ImageSizeAttributes', () => { expect( getViewData( view, { withoutSelection: true } ) ).to.equal( '

' + - '' + + '' + + '' + '

' ); @@ -489,7 +491,8 @@ describe( 'ImageSizeAttributes', () => { expect( getViewData( view, { withoutSelection: true } ) ).to.equal( '
' + - '' + + '' + + '' + '
' ); @@ -507,7 +510,8 @@ describe( 'ImageSizeAttributes', () => { expect( getViewData( view, { withoutSelection: true } ) ).to.equal( '
' + - '' + + '' + + '' + '
' ); From 52f47d3d2e3564e1e4db4783ffe4b73eda0c7384 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Thu, 12 Dec 2024 16:38:51 +0100 Subject: [PATCH 5/9] Fix tests --- packages/ckeditor5-engine/src/view/placeholder.ts | 4 ++++ tests/_data/data-sets/ghs.js | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index a7f8f607228..a32e425166c 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -233,6 +233,10 @@ function updateDocumentPlaceholders( let wasViewModified = false; for ( const [ element, config ] of placeholders ) { + if ( !config.isDirectHost && !config.hostElement ) { + config.hostElement = getChildPlaceholderHostSubstitute( element ); + } + if ( config.hostElement && updatePlaceholder( writer, element, config ) ) { wasViewModified = true; } diff --git a/tests/_data/data-sets/ghs.js b/tests/_data/data-sets/ghs.js index c6204401149..4fbb86ef217 100644 --- a/tests/_data/data-sets/ghs.js +++ b/tests/_data/data-sets/ghs.js @@ -1,6 +1,6 @@ /** * @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 + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-licensing-options */ // This is main Wikipedia page source copied four times. This is to test content with a lot of messy / unsupported markup. From 88e899fa611bd0ce360ab4916080cc40869fabb8 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Thu, 12 Dec 2024 17:14:47 +0100 Subject: [PATCH 6/9] Update --- .../ckeditor5-engine/src/view/placeholder.ts | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index a32e425166c..93daa62c57c 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -230,14 +230,43 @@ function updateDocumentPlaceholders( placeholders: Iterable<[ Element, PlaceholderConfig ]>, writer: DowncastWriter ): boolean { + const directHostElements: Array = []; let wasViewModified = false; + // First set placeholders on the direct hosts. for ( const [ element, config ] of placeholders ) { - if ( !config.isDirectHost && !config.hostElement ) { - config.hostElement = getChildPlaceholderHostSubstitute( element ); + if ( config.isDirectHost ) { + directHostElements.push( element ); + + if ( updatePlaceholder( writer, element, config ) ) { + wasViewModified = true; + } + } + } + + // Then set placeholders on the indirect hosts but only on those that does not already have an direct host placeholder. + for ( const [ element, config ] of placeholders ) { + if ( config.isDirectHost ) { + continue; } - if ( config.hostElement && updatePlaceholder( writer, element, config ) ) { + const hostElement = getChildPlaceholderHostSubstitute( element ); + + // When not a direct host, it could happen that there is no child element + // capable of displaying a placeholder. + if ( !hostElement ) { + continue; + } + + // Don't override placeholder if the host element already has some direct placeholder. + if ( directHostElements.includes( hostElement ) ) { + continue; + } + + // Update the host element (used for setting and removing the placeholder). + config.hostElement = hostElement; + + if ( updatePlaceholder( writer, element, config ) ) { wasViewModified = true; } } From 37ebad5964e4b0b637aa044b1a1ccec179a63133 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Fri, 13 Dec 2024 09:52:47 +0100 Subject: [PATCH 7/9] Fix tests --- .../ckeditor5-engine/src/view/placeholder.ts | 38 +++++-------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 93daa62c57c..18578f9b87e 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -83,7 +83,7 @@ export function enablePlaceholder( { view, element, text, isDirectHost = true, k text, isDirectHost, keepOnFocus, - hostElement: isDirectHost ? element : getChildPlaceholderHostSubstitute( element ) + hostElement: isDirectHost ? element : null }; // Store information about the element placeholder under its document. @@ -230,42 +230,22 @@ function updateDocumentPlaceholders( placeholders: Iterable<[ Element, PlaceholderConfig ]>, writer: DowncastWriter ): boolean { - const directHostElements: Array = []; let wasViewModified = false; - // First set placeholders on the direct hosts. for ( const [ element, config ] of placeholders ) { - if ( config.isDirectHost ) { - directHostElements.push( element ); + if ( !config.isDirectHost ) { + const hostElement = getChildPlaceholderHostSubstitute( element ); - if ( updatePlaceholder( writer, element, config ) ) { - wasViewModified = true; + // When not a direct host, it could happen that there is no child element + // capable of displaying a placeholder. + if ( !hostElement ) { + continue; } - } - } - // Then set placeholders on the indirect hosts but only on those that does not already have an direct host placeholder. - for ( const [ element, config ] of placeholders ) { - if ( config.isDirectHost ) { - continue; + // Update the host element (used for setting and removing the placeholder). + config.hostElement = hostElement; } - const hostElement = getChildPlaceholderHostSubstitute( element ); - - // When not a direct host, it could happen that there is no child element - // capable of displaying a placeholder. - if ( !hostElement ) { - continue; - } - - // Don't override placeholder if the host element already has some direct placeholder. - if ( directHostElements.includes( hostElement ) ) { - continue; - } - - // Update the host element (used for setting and removing the placeholder). - config.hostElement = hostElement; - if ( updatePlaceholder( writer, element, config ) ) { wasViewModified = true; } From 627206e47e3d95dd7b0eb29dd9ec8efce4dd20c4 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Fri, 13 Dec 2024 10:40:22 +0100 Subject: [PATCH 8/9] Fix tests --- .../ckeditor5-engine/src/view/placeholder.ts | 36 ++++++++++++++----- .../tests/view/placeholder.js | 2 ++ 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 18578f9b87e..38472a0a81b 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -230,22 +230,42 @@ function updateDocumentPlaceholders( placeholders: Iterable<[ Element, PlaceholderConfig ]>, writer: DowncastWriter ): boolean { + const directHostElements: Array = []; let wasViewModified = false; + // First set placeholders on the direct hosts. for ( const [ element, config ] of placeholders ) { - if ( !config.isDirectHost ) { - const hostElement = getChildPlaceholderHostSubstitute( element ); + if ( config.isDirectHost ) { + directHostElements.push( element ); - // When not a direct host, it could happen that there is no child element - // capable of displaying a placeholder. - if ( !hostElement ) { - continue; + if ( updatePlaceholder( writer, element, config ) ) { + wasViewModified = true; } + } + } - // Update the host element (used for setting and removing the placeholder). - config.hostElement = hostElement; + // Then set placeholders on the indirect hosts but only on those that does not already have an direct host placeholder. + for ( const [ element, config ] of placeholders ) { + if ( config.isDirectHost ) { + continue; } + const hostElement = getChildPlaceholderHostSubstitute( element ); + + // When not a direct host, it could happen that there is no child element + // capable of displaying a placeholder. + if ( !hostElement ) { + continue; + } + + // Don't override placeholder if the host element already has some direct placeholder. + if ( directHostElements.includes( hostElement ) ) { + continue; + } + + // Update the host element (used for setting and removing the placeholder). + config.hostElement = hostElement; + if ( updatePlaceholder( writer, element, config ) ) { wasViewModified = true; } diff --git a/packages/ckeditor5-engine/tests/view/placeholder.js b/packages/ckeditor5-engine/tests/view/placeholder.js index 9c3f09df116..09967c718ce 100644 --- a/packages/ckeditor5-engine/tests/view/placeholder.js +++ b/packages/ckeditor5-engine/tests/view/placeholder.js @@ -288,6 +288,8 @@ describe( 'placeholder', () => { isDirectHost: true } ); + view.forceRender(); + expect( viewRoot.getChild( 0 ).getAttribute( 'data-placeholder' ) ).to.equal( 'bar' ); expect( viewRoot.getChild( 0 ).isEmpty ).to.be.true; expect( viewRoot.getChild( 0 ).hasClass( 'ck-placeholder' ) ).to.be.true; From d80f6970602d353c29a92e6160e4cf9af2d6a4c5 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Fri, 13 Dec 2024 11:28:39 +0100 Subject: [PATCH 9/9] Change content of the `ghs` performance test --- tests/_data/data-sets/ghs.js | 117 +++++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 48 deletions(-) diff --git a/tests/_data/data-sets/ghs.js b/tests/_data/data-sets/ghs.js index 4fbb86ef217..3c495d5ed5f 100644 --- a/tests/_data/data-sets/ghs.js +++ b/tests/_data/data-sets/ghs.js @@ -9,54 +9,75 @@ /* eslint-disable */ const initialData = - `

HTML playground

-
-
-

- A sample paragraph with a data attribute. -

-
-
-

Responsive column layout

-
-
-
- -
HTML
-
-
-
-
- -
CSS
-
-
-
-
- -
JS
-
-
-
-
-
- -
-
`; + `

Feature paragraph

+ +

Feature heading1

+

Feature heading2

+

Feature heading3

+ +

Feature bold

+

Feature italic

+

Feature strike

+

Feature underline

+

Feature code

+

Feature subscript

+

Feature superscript

+ +

Link feature

+

Anchor (name, ID only)

+ +

Mark feature

+ +

Font feature

+ +
    +
  • Bulleted List feature
    • +
  • Bulleted List feature
    • +
  • Bulleted List feature
    • +
+ +
    +
  1. Numbered List feature
    2. +
  2. Numbered List feature
    3. +
  3. Numbered List feature
    4. +
+ +
    +
  • + +
    • +
  • + +
    • +
  • + +
    • +
+ +

Blockquote Feature

+ +
+ +
Caption
+
+ + 
Code Block
+ +
HTML snippet
+
 
+

empty inline at start

+

Text with empty inline inside

+

Text with empty inline at the end

`; export default function makeData() { - return initialData.repeat( 1000 ); + return initialData.repeat( 250 ); }