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

🖼️ Supporting image height attribute #14146

Closed
10 tasks done
Witoso opened this issue May 15, 2023 · 8 comments · Fixed by #14763
Closed
10 tasks done

🖼️ Supporting image height attribute #14146

Witoso opened this issue May 15, 2023 · 8 comments · Fixed by #14763
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. Epic package:image squad:core Issue to be handled by the Core team.

Comments

@Witoso
Copy link
Member

Witoso commented May 15, 2023

Image feature compatibility with CKEditor 4 (height)

[UPDATE]

Please test things out if you can, the more feedback and eyeballs we get, the more confident we will be that the implementation is correct.

This feature hit the master branch and is available on nightly releases and nightly docs to test.

The details of the changes are described in the comment below.

Archive of discovery

Main problem statements

When implementing images it looks like there are still some areas in which CKEditor 5 lags behind the CKE4. This applies only to inline images because CKEditor 4’s image plugin could only generate inline images. We don’t have to care about the behavior of image2 (low popularity).

After initial research, it looks like the main missing attribute is height, and the following problems need to be solved.

  1. How to preserve the image’s height and width with the content that is already out there and is loaded to the editor.
  2. Should we eagerly set image height and width as it will produce better rendering? What about:
    1. the resizing implementation?
    2. should we set those values proactively when uploading of the image happens?

Feedback and Community

FYI

The srcset attribute specifies the image variants dedicated for the various screen sizes for the web browser to choose from (360px, 720px, 1080px, 1440px, etc.). For instance, the image.jpg file uploaded by the user will have the following markup:

sizesOne or more strings separated by commas, indicating a set of source sizes. Each source size consists of:

  1. media condition. This must be omitted for the last item in the list.
  2. A source size value.
    Media Conditions describe properties of the viewport, not of the image. For example, (max-height: 500px) 1000px proposes to use a source of 1000px width, if the viewport is not higher than 500px.

If the srcset attribute uses width descriptors, the sizes attribute must also be present, or the srcset itself will be ignored.

CKEditor5 when using responsive image (EasyImage) the width attribute is set but not the height. CKBox is not doing it.

Good explanation of the impact to width and height on CLS: https://www.aleksandrhovhannisyan.com/blog/setting-width-and-height-on-images/

Essentially, the width and height attributes are meant for initializing an image’s aspect ratio, not for dictating the width and height at which it renders—that’s the job of CSS. Once an image has loaded in, if you want to apply some custom CSS to size it differently, you can do so without changing the image’s intrinsic aspect ratio sizing.

Use cases

Problem 1: I have HTML with width and height set and it doesn't load to the editor.
Outcome: Load the width and height attributes of the image, e.g. setData content to the model (no GHS).


Problem 2: When I resized an image (with width and height), and I have the resized image placed on the page there should not be a layout shift. Also, I still want to resize with the aspect ratio locked.
Outcome: width and height stay but the image is resized with the aspect ratio intact.


Problem 3: When I insert images, I want width and height set automatically.
Outcome: After upload those attributes appear on the image element.

Scope

Out of scope

Problem 4: As a user, I want to change the width and height attributes.
Outcome: width and height attributes are configurable on the UI.

Decided to keep it out of scope for now. width and height should set the intrinsic size of the image to lock the aspect ratio for the browser, and restyling of images should be done via the CSS.

@Witoso Witoso added squad:core Issue to be handled by the Core team. Epic labels May 15, 2023
@Witoso Witoso changed the title Image height Supporting image height attribute May 15, 2023
@Witoso Witoso added package:image domain:ui/ux This issue reports a problem related to UI or UX. labels May 15, 2023
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label May 16, 2023
@Witoso
Copy link
Member Author

Witoso commented May 31, 2023

Questions:

  • non intuitive when setting only height.
  • height: auto collides with a set height in the content.
  • Should there be a config option that turns it off?
  • Where to put it in the docs?

@Witoso
Copy link
Member Author

Witoso commented Jun 2, 2023

We decided that we will set those attributes on upload by default as the most content-friendly option.

However, research needs to be conducted on how the proposed solution will behave in a variety of content sources. This initiative is a hit or miss as we cannot roll back easily. CKEditor 4 output data, and Drupal's content, as well as users' intent preservation, need to be taken into account. One of the challenges is resizing in CKE4 by tweaking the aspect ratio of the original image.

Tracking in: #14313

@Witoso
Copy link
Member Author

Witoso commented Jul 12, 2023

We want to share what's our approach to this topic so far and what are the changes that will. We will not share yet detailed rational why some decisions were made, as we plan to write everything down after we complete all tasks and cover the edge cases.

The brief summary of the context and decisions:

  • We intend to support height and width attributes as it produces good quality content:
    • Those attributes will be set automatically on the upload step.
    • We will not change already existing content, loading HTML (i.e., setData) with images will not set up those attributes.
    • Changes to an image (e.g., resize) will trigger creation of those attributes. We decided that those attributes are crucial, and actions on a current image that doesn't have them, should improve this image's markup.
  • We need to support as much as possible the user intents, especially from the content that was produced in the CKEditor 4.
    • In CKE4, it was possible to tweak the aspect-ratio of an image. We decided to support this in the editor, and if the aspect-ratio was changed, we should show it as changed.
  • We don't introduce now the options to set width and height via the editor's UI or a resize with unlocked aspect-ratio. Mostly due to the complexities of the changes so far, we try not to introduce a scope creep.

This is a very complex, and multidimensional change as the scenarios and different configs create a very challenging matrix of behaviors and effects. We spent a lot of time on this, and we are close to the finish line.

The changes can be tested on the ck/epic/14146-support-image-height-attribute branch. A special manual test was created to reflect different scenarios, `imagesizeattributesallcases.html (launched by the yarn manual -sw -f 'image').

Please test things out if you can, the more feedback and eyeballs we get, the more confident we will be that the implementation is correct.

What follows below is a detailed explanation of what changes in the editor's model, and pipelines. If you're not integrating deeply with our images, you can skip this section.

Image styles/attributes vs. model attributes

  • Style width → model resizedWidth (changed from width).
  • Style height → model resizedHeight (new).
  • Attribute width → model width (new).
  • Attribute height → model height (new).

Upcast:

  • Style width & height set (in px) → upcast to width & height (because styles override width and height attributes).
  • Else if attributes width & height set → upcast to width & height
  • Only style width set (% or px) → upcast to resizedWidth
  • Style width & height set (in %)
    • set resizedHeight, but remove when resizing.
  • Style height only (in %)
    • set resizedHeight, but remove when resizing.
  • Style height only (in px)
    • set resizedHeight, but remove when resizing.

Editing downcast:

  • Resizing in % → set resizedWidth.
  • Resizing in px → set resizedWidth.
  • If resizedHeight was set in data → remove it while resizing.
    • So, we don’t break how resizing in CKE5 works right now (keeping the aspect ratio).
  • Height: auto (in content styles) (for <img>):
    • Inline
      • set for all.
    • Block
      • set for all.
  • Aspect-ratio inline style (for <img>):
    • Inline
      • set for all.
    • Block
      • set for all.

Data downcast:

  • Height: auto (for <img>):
    • Inline
      • needed for resized images only (class image_resized on <img>)
      • we set it for resized only
    • block
      • set for all
  • Aspect-ratio style (for <img>):
    • Inline
      • needed for resized images with broken aspect ratio only (otherwise height:auto style resets aspect ratio to proportional sizes).
      • we set it for resized only.
    • Block
      • set for all.

@wimleers
Copy link

wimleers commented Jul 13, 2023

In CKE4, it was possible to tweak the aspect-ratio of an image. We decided to support this in the editor, and if the aspect-ratio was changed, we should show it as changed.

IIRC we had previously concluded that this was very rarely used? Am I now misremembering that? 🤔 Asking because this this complicates the implementation enormously.
Checking … found it: the image2 demo clearly shows that there's no ability to customize the aspect ratio?

Pinged @lauriii to analyze/respond in detail, because I'm not sure yet how all of the above applies to Drupal's DrupalImage plugin which extends CKEditor 5's own ImageInsertUI and ImageUpload — to never generate style attributes but width instead.

@Witoso
Copy link
Member Author

Witoso commented Jul 13, 2023

Unfortunately, it may be a rare case the case in Drupal which, I think, didn't implement the modal. In the plain CKE4, double-click on an image showed the dimensions, with a pretty obvious mechanism to unlock the aspect ratio.

@Witoso Witoso changed the title Supporting image height attribute [Feedback needed] Supporting image height attribute Jul 14, 2023
@Witoso Witoso pinned this issue Jul 14, 2023
@Witoso Witoso changed the title [Feedback needed] Supporting image height attribute 🖼️ [Feedback needed] Supporting image height attribute Jul 24, 2023
@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Aug 7, 2023
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Aug 26, 2023
niegowski added a commit that referenced this issue Sep 14, 2023
…eight-attribute

Feature (image): The image `width` and `height` attributes are preserved while loading editor content. Closes #14146.

Feature (image): Images without size specified will automatically gain natural image size on any interaction with the image. See #14146.

MAJOR BREAKING CHANGE (image): The model attribute name of the resized image is now changed to `resizedWidth`. The `width` and `height` attributes are now used to preserve the image's natural width and height.

MAJOR BREAKING CHANGE (image): The `srcset` model attribute has been simplified. It is no longer an object `{ data: "...", width: "..." }`, but a value previously stored in the `data` part.
@CKEditorBot CKEditorBot added this to the iteration 67 milestone Sep 14, 2023
@Witoso
Copy link
Member Author

Witoso commented Sep 20, 2023

This feature hit the master and is available on nightly releases and nightly docs to test.

@KlemenDEV
Copy link

Nice, thank you!

@Witoso
Copy link
Member Author

Witoso commented Sep 21, 2023

Setting the width/height via the UI: #15044.

@Witoso Witoso changed the title 🖼️ [Feedback needed] Supporting image height attribute 🖼️ Supporting image height attribute Oct 2, 2023
@Witoso Witoso unpinned this issue Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. Epic package:image squad:core Issue to be handled by the Core team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants