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

Improve performance of placeholder and add ghs performance test. #17589

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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>'
);
Expand All @@ -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>'
);
Expand Down
45 changes: 28 additions & 17 deletions packages/ckeditor5-engine/src/view/placeholder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
scofalik marked this conversation as resolved.
Show resolved Hide resolved

// 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 ) {
Expand All @@ -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 ) );
}
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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;

Expand Down
2 changes: 2 additions & 0 deletions packages/ckeditor5-engine/tests/view/placeholder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
17 changes: 12 additions & 5 deletions packages/ckeditor5-image/src/imagesizeattributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -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 ) ) {
Expand Down Expand Up @@ -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 );
}
} );
}
Expand Down
12 changes: 8 additions & 4 deletions packages/ckeditor5-image/tests/imagesizeattributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>'
);

Expand All @@ -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>'
);

Expand Down Expand Up @@ -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>'
);

Expand All @@ -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>'
);

Expand Down
83 changes: 83 additions & 0 deletions tests/_data/data-sets/ghs.js
Original file line number Diff line number Diff line change
@@ -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 =
filipsobol marked this conversation as resolved.
Show resolved Hide resolved
`<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 );
}
16 changes: 12 additions & 4 deletions tests/_data/data-sets/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Loading