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

Image jumps when resizing in container that has padding #16304

Merged
merged 9 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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 @@ -59,17 +59,16 @@ describe( 'getSelectedImageWidthInUnits', () => {
const { unit, value } = getSelectedImageWidthInUnits( editor, 'px' );

expect( unit ).to.be.equal( 'px' );
expect( value ).to.be.greaterThan( 400 );
expect( value ).to.be.equal( 310 );
Mati365 marked this conversation as resolved.
Show resolved Hide resolved
} );

it( 'should return casted pixels value to percentage', () => {
setData( model, `[<imageBlock src="${ IMAGE_SRC_FIXTURE }" resizedWidth="380px"></imageBlock>]` );
setData( model, `[<imageBlock src="${ IMAGE_SRC_FIXTURE }" resizedWidth="250px"></imageBlock>]` );

const { unit, value } = getSelectedImageWidthInUnits( editor, '%' );

expect( unit ).to.be.equal( '%' );
expect( value ).to.be.greaterThan( 30 );
expect( value ).to.be.lessThan( 40 );
expect( value ).to.be.equal( 50 );
Mati365 marked this conversation as resolved.
Show resolved Hide resolved
} );

async function createEditor( config ) {
Expand All @@ -80,6 +79,13 @@ describe( 'getSelectedImageWidthInUnits', () => {
}
} );

editor.editing.view.change( writer => {
writer.setStyle( {
width: '500px',
padding: '0px'
}, editor.editing.view.document.getRoot() );
} );

model = editor.model;
return editor;
}
Expand Down
35 changes: 30 additions & 5 deletions packages/ckeditor5-widget/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -453,20 +453,36 @@ function addSelectionHandle( widgetElement: ViewContainerElement, writer: Downca
* Starting from a DOM resize host element (an element that receives dimensions as a result of resizing),
* this helper returns the width of the found ancestor element.
*
* **Note**: This helper searches up to 5 levels of ancestors only.
* * It searches up to 5 levels of ancestors only.
* * It will skip search if passed element (or it's parent) has `contenteditable` attribute or is root document editable.
* It's crucial for features such like pagination because editable root element might have different width than whole editor itself.
*
* @param domResizeHost Resize host DOM element that receives dimensions as a result of resizing.
* @returns Width of ancestor element in pixels or 0 if no ancestor with a computed width has been found.
*/
export function calculateResizeHostAncestorWidth( domResizeHost: HTMLElement ): number {
const getElementComputedWidth = ( element: HTMLElement ) => {
const { width, paddingLeft, paddingRight } = element.ownerDocument.defaultView!.getComputedStyle( element! );

return parseFloat( width ) - ( parseFloat( paddingLeft ) || 0 ) - ( parseFloat( paddingRight ) || 0 );
};

if ( isMaximumResizeHostAncestorElement( domResizeHost ) ) {
return getElementComputedWidth( domResizeHost );
}

const domResizeHostParent = domResizeHost.parentElement;

if ( !domResizeHostParent ) {
return 0;
}

// Need to use computed style as it properly excludes parent's paddings from the returned value.
let parentWidth = parseFloat( domResizeHostParent!.ownerDocument.defaultView!.getComputedStyle( domResizeHostParent! ).width );
let parentWidth = getElementComputedWidth( domResizeHostParent! );

if ( isMaximumResizeHostAncestorElement( domResizeHostParent ) ) {
Copy link
Member

@oleq oleq May 6, 2024

Choose a reason for hiding this comment

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

This, in my opinion, is only masking the actual root of the issue in ImageResizeHandles. Check out the current implementation at

and see what the output is for various combinations of editors (decoupled, classic) and images (block, inline).

Long story short, it returns paragraphs for inline images and editing roots for block images (that exist directly in a root). I think this is wrong. According to _getResizeHost() in Resizer class

Resize host is an object that receives dimensions which are the result of resizing.

/**
* Obtains the resize host.
*
* Resize host is an object that receives dimensions which are the result of resizing.
*/
private _getResizeHost(): HTMLElement {
const widgetWrapper = this._domResizerWrapper!.parentElement;
return this._options.getResizeHost( widgetWrapper! );
}

An editing root does not receive dimensions. <figure> does (block image) or <span> (inline image).

return parentWidth;
}

// Sometimes parent width cannot be accessed. If that happens we should go up in the elements tree
// and try to get width from next ancestor.
Expand All @@ -483,14 +499,23 @@ export function calculateResizeHostAncestorWidth( domResizeHost: HTMLElement ):
return 0;
}

parentWidth = parseFloat(
domResizeHostParent!.ownerDocument.defaultView!.getComputedStyle( checkedElement ).width
);
parentWidth = getElementComputedWidth( checkedElement );

if ( isMaximumResizeHostAncestorElement( checkedElement ) ) {
break;
}
}

return parentWidth;
}

/**
* Checks if passed HTML has `contenteditable` attribute.
*/
function isMaximumResizeHostAncestorElement( element: HTMLElement ) {
return element.getAttribute( 'contenteditable' ) === 'true' || element.classList.contains( 'ck-editor__editable' );
}

/**
* Calculates a relative width of a `domResizeHost` compared to its ancestor in percents.
*
Expand Down
56 changes: 56 additions & 0 deletions packages/ckeditor5-widget/tests/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,49 @@ describe( 'widget utils', () => {

describe( 'Calculate resize host ancestor width utils', () => {
describe( 'calculateResizeHostAncestorWidth()', () => {
it( 'should return passed element width with excluded padding if it\'s resize root element', () => {
const editable = editorEditableTag();
document.body.appendChild( editable );

const width = calculateResizeHostAncestorWidth( editable );
expect( width ).to.be.equal( 560 );

editable.remove();
} );

it( 'should pick editable grandparent width if current element is not editable', () => {
const editable = editorEditableTag( {}, [
tag( 'div' )
] );

document.body.appendChild( editable );

const width = calculateResizeHostAncestorWidth( editable.firstChild );
expect( width ).to.be.equal( 560 );

editable.remove();
} );

it( 'should break lookup on nearest editable element (excluding grandparent)', () => {
const child = document.createElement( 'div' );
const editable = editorEditableTag( { width: '100px' }, [
editorEditableTag( { width: '800px' }, [
tag( 'span', {}, [
tag( 'span', {}, [
child
] )
] )
] )
] );

document.body.appendChild( editable );

const width = calculateResizeHostAncestorWidth( child );
expect( width ).to.be.equal( 760 );

editable.remove();
} );

it( 'should get size from parent of passed element', () => {
const domResizeHost = tag( 'div' );
const tree = tag( 'div', sizeAttributes( 200 ), [
Expand Down Expand Up @@ -862,6 +905,19 @@ describe( 'widget utils', () => {
} );
} );

function editorEditableTag( style = {}, children = [] ) {
const element = tag( 'div', { contenteditable: 'true' }, children );

element.classList.add( 'ck-editor__editable' );
Object.assign( element.style, {
padding: '0 20px',
width: '600px',
...style
} );

return element;
}

function tag( name, attributes = {}, children = [] ) {
const element = document.createElement( name );

Expand Down