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

Allow and preserve image width and height #14233

Conversation

mmotyczynska
Copy link
Contributor

@mmotyczynska mmotyczynska commented May 24, 2023

Suggested merge commit message (convention)

Feature (image): It should be possible to load and preserve image width and height attributes. Closes #14201.

MAJOR BREAKING CHANGE (image): The name of the image resize width attribute has been changed to resizedWidth. The width and height attributes are now used to preserve image width and height.

MAJOR BREAKING CHANGE (image): The srcset attribute has been simplified. It is no longer an object { data: "...", width: "..." }, but a value previously stored in the data part.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@mmotyczynska mmotyczynska marked this pull request as ready for review May 25, 2023 14:43
@arkflpc arkflpc self-requested a review May 26, 2023 09:41
packages/ckeditor5-image/tests/image/imageblockediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/image/imageediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/image/imageediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/image/imageediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/image/imageediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/image/imageblockediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/image/imageinlineediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/tests/image/imageinlineediting.js Outdated Show resolved Hide resolved
@mmotyczynska mmotyczynska requested a review from arkflpc May 26, 2023 11:56
@arkflpc
Copy link
Contributor

arkflpc commented May 29, 2023

@ckeditor/qa-team , can you look at it?
@mmotyczynska , will you provide more info if neede?

@dufipl
Copy link
Contributor

dufipl commented May 29, 2023

@arkflpc Sure, we will take a look at this 🕵️

@mmotyczynska
Copy link
Contributor Author

The first  change:

  • We now enable width and height attributes for imageInline and imageBlock elements.
  • Unfortunately the width attribute in model element was already in use for image resize. So we had to change its name from width to resizedWidth.

The second change:

  • We simplified the srcset attribute which previosusly was an object: { data: "...", width: "..." }, but now it's a value previously stored in the data part. This is because we handle the width in the first change and it's no longer needed here in srcset.

Manual tests:

  • There is one new manual test  that already loads new attributes for block/inline images with and without resize value.
  • But these changes may affect also other tests that handle images, e.g. paste from office.

What to check:

  • The image should load and preserve new attributes (use dedicated manual test or setData() for example).
  • Images should preserve aspect ratio.
  • This change should not break the image resize feature, but its value should now be stored in resizedWidth instead of width attribute.
  • Other than that use your creativity and try to break the editor or break importing images etc. 😄

@Acrophost
Copy link

height attribute is completely ignored even when width is not specified

Steps

  1. Open the manual test
  2. Call editor.setData('<img src="sample.jpg" alt="Foo" height="100">'); in the console

Result

Image gets the default size of 800x376 regardless of given height.
The height attribute is kept but size itself is overwritten by .ck-content .image-inline picture, .ck-content .image-inline img { height: auto; } .
If the width is specified, image resizes accordingly to it.

@Acrophost

This comment was marked as off-topic.

@Acrophost
Copy link

We have finished testing ✨
The main thing we noticed is that height is getting overwritten by height: auto style and thus, has no effect even when there is no width defined (when this declaration is disabled, all works as it should).
Aside from that, everything looks good to us 🎉

@charlttsie @dufipl thank you for the jolly cooperation 🍻

@mmotyczynska mmotyczynska changed the base branch from master to ck/epic/14146-support-image-height-attribute June 5, 2023 15:56
@mmotyczynska mmotyczynska merged commit 7f84ec2 into ck/epic/14146-support-image-height-attribute Jun 7, 2023
@mmotyczynska mmotyczynska deleted the ck/14201-allow-and-preserve-image-width-and-height branch June 7, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow and preserve image width and height attributes
4 participants