Improve performance of placeholder and add ghs performance test

ckeditor · Jan 8, 2025 · 1e65d3d · 1e65d3d
1 parent 0a7cf7f
commit 1e65d3d
Show file tree

Hide file tree

Showing 7 changed files with 150 additions and 32 deletions.
diff --git 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(
 					'<figure class="ck-widget ck-widget_selected image" contenteditable="false" data-ckbox-resource-id="example-id">' +
-						'<img alt="alt text" height="50" src="/assets/sample.png" style="aspect-ratio:50/50" width="50"></img>' +
+						'<img alt="alt text" height="50" loading="lazy" src="/assets/sample.png" style="aspect-ratio:50/50" width="50">' +
+						'</img>' +
 						'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
 					'</figure>'
 				);
@@ -961,7 +962,9 @@ describe( 'CKBoxImageEditCommand', () => {
 				expect( getViewData( editor.editing.view, { withoutSelection: true } ) ).to.equal(
 					'<figure class="ck-widget ck-widget_selected image image-processing" ' +
 						'contenteditable="false" data-ckbox-resource-id="example-id">' +
-						'<img alt="alt text" height="100" src="/assets/sample.png" style="height:100px;width:100px" width="100"></img>' +
+						'<img alt="alt text" height="100" loading="lazy" src="/assets/sample.png" ' +
+							'style="height:100px;width:100px" width="100">' +
+						'</img>' +
 						'<div class="ck ck-reset_all ck-widget__type-around"></div>' +
 					'</figure>'
 				);

diff --git 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<ObservableChangeEvent>( '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 ) );
 	}
 }
 
@@ -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,13 +208,28 @@ 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.
  *
  * @returns True if any changes were made to the view document.
  */
-function updateDocumentPlaceholders( doc: Document, writer: DowncastWriter ): boolean {
-	const placeholders = documentPlaceholders.get( doc )!;
+function updateDocumentPlaceholders(
+	placeholders: Iterable<[ Element, PlaceholderConfig ]>,
+	writer: DowncastWriter
+): boolean {
 	const directHostElements: Array<Element> = [];
 	let wasViewModified = false;
 

diff --git 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;

diff --git 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 <img> 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<DowncastAttributeEvent>( `attribute:${ modelAttributeName }:${ imageType }`, ( evt, data, conversionApi ) => {
 				if ( !conversionApi.consumable.consume( data.item, evt.name ) ) {
@@ -166,8 +167,14 @@ 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
@@ -290,7 +290,8 @@ describe( 'ImageSizeAttributes', () => {
 
 						expect( getViewData( view, { withoutSelection: true } ) ).to.equal(
 							'<p><span class="ck-widget image-inline image_resized" contenteditable="false" style="width:50px">' +
-								'<img height="200" src="/assets/sample.png" style="aspect-ratio:100/200" width="100"></img>' +
+								'<img height="200" loading="lazy" src="/assets/sample.png" style="aspect-ratio:100/200" width="100">' +
+								'</img>' +
 							'</span></p>'
 						);
 
@@ -307,7 +308,8 @@ describe( 'ImageSizeAttributes', () => {
 
 						expect( getViewData( view, { withoutSelection: true } ) ).to.equal(
 							'<p><span class="ck-widget image-inline" contenteditable="false">' +
-								'<img height="200" src="/assets/sample.png" style="aspect-ratio:100/200" width="100"></img>' +
+								'<img height="200" loading="lazy" src="/assets/sample.png" style="aspect-ratio:100/200" width="100">' +
+								'</img>' +
 							'</span></p>'
 						);
 
@@ -489,7 +491,8 @@ describe( 'ImageSizeAttributes', () => {
 
 						expect( getViewData( view, { withoutSelection: true } ) ).to.equal(
 							'<figure class="ck-widget image image_resized" contenteditable="false" style="width:50px">' +
-								'<img height="200" src="/assets/sample.png" style="aspect-ratio:100/200" width="100"></img>' +
+								'<img height="200" loading="lazy" src="/assets/sample.png" style="aspect-ratio:100/200" width="100">' +
+								'</img>' +
 							'</figure>'
 						);
 
@@ -507,7 +510,8 @@ describe( 'ImageSizeAttributes', () => {
 
 						expect( getViewData( view, { withoutSelection: true } ) ).to.equal(
 							'<figure class="ck-widget image" contenteditable="false">' +
-								'<img height="200" src="/assets/sample.png" style="aspect-ratio:100/200" width="100"></img>' +
+								'<img height="200" loading="lazy" src="/assets/sample.png" style="aspect-ratio:100/200" width="100">' +
+								'</img>' +
 							'</figure>'
 						);
 

diff --git a/tests/_data/data-sets/ghs.js b/tests/_data/data-sets/ghs.js
@@ -0,0 +1,83 @@
+/**
+ * @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-licensing-options
+ */
+
+// 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 =
+	`<p style="color:blue;">Feature paragraph</p>
+
+	<h2 style="color:green">Feature heading1</h2>
+	<h3 style="color:green">Feature heading2</h3>
+	<h4 style="color:green">Feature heading3</h4>
+
+	<p><strong style="color:blue;">Feature bold</strong></p>
+	<p><i style="color:blue;">Feature italic</i></p>
+	<p><s style="color:blue;">Feature strike</s></p>
+	<p><u style="color:blue;">Feature underline</u></p>
+	<p><code style="color:blue;">Feature code</code></p>
+	<p><sub style="color:blue;">Feature subscript</sub></p>
+	<p><sup style="color:blue;">Feature superscript</sup></p>
+
+	<p><a style="color:green;" href="https://example.com">Link feature</a></p>
+	<p><a name="anchor" id="anchor">Anchor (name, ID only)</a></p>
+
+	<p><mark class="marker-yellow" data-mark>Mark feature</mark></p>
+
+	<p><span class="text-big text-italic" style="background-color:hsl(60, 75%, 60%);color:hsl(240, 75%, 60%);font-family:'Courier New', Courier, monospace;border:1px solid black">Font feature</span></p>
+
+	<ul style="background:blue;">
+		<li style="color:yellow;">Bulleted List feature</li>
+		<li style="color:yellow;">Bulleted List feature</li>
+		<li style="color:yellow;">Bulleted List feature</li>
+	</ul>
+
+	<ol style="background:blue;">
+		<li style="color:yellow;">Numbered List feature</li>
+		<li style="color:yellow;">Numbered List feature</li>
+		<li style="color:yellow;">Numbered List feature</li>
+	</ol>
+
+	<ul class="todo-list" style="background:blue;">
+		<li style="color:yellow;">
+			<label class="todo-list__label">
+				<input type="checkbox" disabled="disabled">
+				<span class="todo-list__label__description">Todo List feature</span>
+			</label>
+		</li>
+		<li style="color:yellow;">
+			<label class="todo-list__label">
+				<input type="checkbox" disabled="disabled">
+				<span class="todo-list__label__description">Todo List feature</span>
+			</label>
+		</li>
+		<li style="color:yellow;">
+			<label class="todo-list__label">
+				<input type="checkbox" disabled="disabled">
+				<span class="todo-list__label__description">Todo List feature</span>
+			</label>
+		</li>
+	</ul>
+
+	<blockquote style="color:blue;"><p>Blockquote Feature</p></blockquote>
+
+	<figure style="border: 1px solid blue;" class="image">
+		<img src="/ckeditor5/tests/manual/sample.jpg" width="826" height="388"/>
+		<figcaption style="background:yellow;">Caption</figcaption>
+	</figure>
+
+	<pre style="background:blue;"><code style="color:yellow;" class="language-plaintext">Code Block</code></pre>
+
+	<div style="border: 1px solid blue" class="raw-html-embed">HTML snippet</div>
+	<div data-foo class="page-break" style="page-break-after:always;"><span style="display:none;">&nbsp;</span></div>
+	<p><i class="inline-icon"></i> empty inline at start</p>
+	<p>Text with <i class="inline-icon"></i> empty inline inside</p>
+	<p>Text with empty inline at the end <i class="inline-icon"></i></p>`;
+
+export default function makeData() {
+  return initialData.repeat( 250 );
+}
diff --git 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-licensing-options
  */
 
-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
 };