Image jumps when resizing in container that has padding

[Mati365]

Suggested merge commit message (convention)

Fix (widget): Image no longer jumps when resizing in container that has padding. Closes #14698

[coderabbitai]

Walkthrough

The recent updates to CKEditor5 focus on enhancing image resizing functionalities. The changes include the addition of utility functions for resize calculations, optimization of test configurations to align with these updates, and a more efficient method for setting multiple styles simultaneously.

Changes

File Path	Change Summary
.../ckeditor5-widget/src/utils.ts	Added getElementComputedWidth utility function for resize calculations.
.../ckeditor5-image/src/imageresize/imageresizehandles.ts	Modified getResizeHost method to use imageModel directly.
.../ckeditor5-image/tests/imageresize/utils/...	Updated test expectations and style setting methods for precise image resizing.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

Out of diff range and nitpick comments (1)

packages/ckeditor5-widget/src/utils.ts (1)

Line range hint 456-517: Consider refactoring calculateResizeHostAncestorWidth to improve readability and maintainability.

- const getElementComputedWidth = ( element: HTMLElement ) => {
-   const { width, paddingLeft, paddingRight } = element.ownerDocument.defaultView!.getComputedStyle( element! );
-   return parseFloat( width ) - parseFloat( paddingLeft ) - parseFloat( paddingRight );
- };
- if ( isMaximumResizeHostAncestorElement( domResizeHost ) ) {
-   return getElementComputedWidth( domResizeHost );
- }
- const domResizeHostParent = domResizeHost.parentElement;
- if ( !domResizeHostParent ) {
-   return 0;
- }
- let parentWidth = getElementComputedWidth( domResizeHostParent! );
- if ( isMaximumResizeHostAncestorElement( domResizeHostParent ) ) {
-   return parentWidth;
- }
- const ancestorLevelLimit = 5;
- let currentLevel = 0;
- let checkedElement = domResizeHostParent!;
- while ( isNaN( parentWidth ) ) {
-   checkedElement = checkedElement.parentElement!;
-   if ( ++currentLevel > ancestorLevelLimit ) {
-     return 0;
-   }
-   parentWidth = getElementComputedWidth( checkedElement );
-   if ( isMaximumResizeHostAncestorElement( checkedElement ) ) {
-     break;
-   }
- }
- return parentWidth;
+ // Refactored code here

[coderabbitai]

Actionable comments posted: 2

[oleq]

• The part of the fix with calculating ancestor width excluding padding is fine 👍
• The part with isMaximumResizeHostAncestorElement() is, in my opinion, only masking an underlying bug in ImageResizeHandles. I think this issue has nothing to do with pagination, this popped up in pagination samples only because they use a specific combination of editing root/paddings/wrappers in DOM. For instance, currently on master this will also fail if you enable image resize in an inline editor:

Screen.Recording.2024-05-06.at.17.06.24.mov

---

Long story short, the entire solution that worked for me (relative to master) is listed below.

diff --git a/packages/ckeditor5-image/src/imageresize/imageresizehandles.ts b/packages/ckeditor5-image/src/imageresize/imageresizehandles.ts
index f311ddaa70..479ec976ef 100644
--- a/packages/ckeditor5-image/src/imageresize/imageresizehandles.ts
+++ b/packages/ckeditor5-image/src/imageresize/imageresizehandles.ts
@@ -101,8 +101,7 @@ export default class ImageResizeHandles extends Plugin {
                                                       return domWidgetElement.querySelector( 'img' )!;
                                              },
                                              getResizeHost() {
-                                                      // Return the model image element parent to avoid setting an inline element (<a>/<span>) as a resize host.
-                                                      return domConverter.mapViewToDom( mapper.toViewElement( imageModel.parent as Element )! ) as HTMLElement;
+                                                      return domConverter.mapViewToDom( mapper.toViewElement( imageModel as Element )! ) as HTMLElement;
                                              },
 
                                              isCentered() {
diff --git a/packages/ckeditor5-widget/src/utils.ts b/packages/ckeditor5-widget/src/utils.ts
index dc2eefaa8c..4b8a2e2b62 100644
--- a/packages/ckeditor5-widget/src/utils.ts
+++ b/packages/ckeditor5-widget/src/utils.ts
@@ -459,6 +459,12 @@ function addSelectionHandle( widgetElement: ViewContainerElement, writer: Downca
  * @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 getElementComputedWidthExcludingPadding = ( element: HTMLElement ) => {
+                  const { width, paddingLeft, paddingRight } = element.ownerDocument.defaultView!.getComputedStyle( element! );
+
+                  return parseFloat( width ) - ( parseFloat( paddingLeft ) || 0 ) - ( parseFloat( paddingRight ) || 0 );
+         };
+
          const domResizeHostParent = domResizeHost.parentElement;
 
          if ( !domResizeHostParent ) {
@@ -466,7 +472,7 @@ export function calculateResizeHostAncestorWidth( domResizeHost: HTMLElement ):
          }
 
          // 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 = getElementComputedWidthExcludingPadding( domResizeHostParent );
 
          // 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.
@@ -483,9 +489,7 @@ export function calculateResizeHostAncestorWidth( domResizeHost: HTMLElement ):
                            return 0;
                   }
 
-                  parentWidth = parseFloat(
-                                    domResizeHostParent!.ownerDocument.defaultView!.getComputedStyle( checkedElement ).width
-                  );
+                  parentWidth = getElementComputedWidthExcludingPadding( checkedElement );
          }
 
          return parentWidth;

I altered the behavior of getResizeHost() in ImageResizeHandles because I think it is incorrect. Please keep in mind that there are unit tests that would say otherwise (an aftermath of #9598)

• ckeditor5/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js

  Lines 1042 to 1058 in 67d2d60

  	it( 'should set the list item as the resize host if an image is inside a to-do list', async () => {
  	editor = await createEditor( {
  	plugins: [ Image, ImageResizeEditing, ImageResizeHandles, LegacyTodoList, Paragraph ]
  	} );
  	await setModelAndWaitForImages( editor,
  	'<listItem listType="todo" listIndent="0">' +
  	`[<imageInline linkHref="http://ckeditor.com" src="${ IMAGE_SRC_FIXTURE }" alt="alt text"></imageInline>]` +
  	'</listItem>'
  	);
  	const resizer = Array.from( editor.plugins.get( 'WidgetResize' )._resizers.values() )[ 0 ];
  	expect( resizer._getResizeHost().nodeName ).to.equal( 'LI' );
  	await editor.destroy();
  	} );

• ckeditor5/packages/ckeditor5-image/tests/imageresize/imageresizehandles.js

  Lines 998 to 1008 in 67d2d60

  	it( 'should set the paragraph as the resize host for an image wrapped with a link', async () => {
  	await setModelAndWaitForImages( editor,
  	'<paragraph>' +
  	`[<imageInline linkHref="http://ckeditor.com" src="${ IMAGE_SRC_FIXTURE }" alt="alt text"></imageInline>]` +
  	'</paragraph>'
  	);
  	const resizer = Array.from( editor.plugins.get( 'WidgetResize' )._resizers.values() )[ 0 ];
  	expect( resizer._getResizeHost().nodeName ).to.equal( 'P' );
  	} );

so what we need to do now is figure out how (if) this affects these cases, adjust it if necessary, and then document it (because API docs are missing

ckeditor5/packages/ckeditor5-widget/src/widgetresize.ts

Line 326 in 67d2d60

getResizeHost: ( widgetWrapper: HTMLElement ) => HTMLElement;

).

I briefly tested resizing images in to-do lists and linked images and it worked fine but maybe I didn't understand some use-cases. A QA session could also be useful.

[oleq]

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

ckeditor5/packages/ckeditor5-image/src/imageresize/imageresizehandles.ts

Line 103 in 67d2d60

getResizeHost() {

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.

ckeditor5/packages/ckeditor5-widget/src/widgetresize/resizer.ts

Lines 413 to 422 in 67d2d60

	/**
	* 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).

[Mati365]

@oleq I applied your code suggestion and moved the ticket to QA.

[oleq]

@oleq I applied your code suggestion and moved the ticket to QA.

Have you been able to verify these to-do list/linked image use cases?

[Mati365]

@oleq I briefly checked that and there were no issues.