CKEditor 5 resizes images with % width instead of px width (the CKEditor 4 default): breaks image captions *and* is a regression

This is related to ✨ [drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing Fixed Pulled from git annotate.

To add to the rationale for this change. When does anybody want a photo to be a percent of the container? I could be wrong but I've never had this use-case. Might look good on desktop, but mobile it's a nightmare.

And with percentages, image_resize_filter doesn't know what to do. 💬 CKEditor 5 compatibility Active

The figcaption is not the only thing broken here. If you remove it and add figure around an image without a caption that is broken as well:

See fiddle
https://jsfiddle.net/fuaLqp5m/1/
Screenshot for posterity:

This is very strongly connected to 🐛 [upstream] CKEditor 5 breaks website CLS (no image width and height attributes) Postponed .

… and even more so to 🐛 EditorFileReference should compute a px if a % is specified, even though % is not allowed in HTML5 Needs work .

When does anybody want a photo to be a percent of the container?

IIRC CKEditor 5 intentionally chose this, I'll see if I can dig up their research/rationale.

Meanwhile, given @lauriii's expertise on Drupal's CKEditor 5 image integration, assigning to him 🤓

Confirmed MR3671 fixed the issue.

Not sure if this could get a test case or if it's possible

I think it would be good to have a reference to the ckeditor5 history (link if it's documented, or a summary from the ckeditor5 team if it's not) if we can, so it's easy for anyone wondering why we diverged later to see why we made the decision and not try to revert it back.

Note that it really was intentional of CKEditor 5 to use % instead of px by default. See https://ckeditor.com/docs/ckeditor5/latest/features/images/images-resizi....

Resizing to px matches CKEditor 4's behavior.

I'll see if I can dig up their research/rationale.

I've asked them.
AFAICT it was proposed by @Reinmar at https://github.com/ckeditor/ckeditor5/issues/5167#issuecomment-519087175 and decided at https://github.com/ckeditor/ckeditor5/issues/5167#issuecomment-522489212.

The rationale is that this results in responsively sized images by default. But … in Drupal, the CKEditor 5 instance runs in a different theme than the front-end, which I suspect is atypical for most CKEditor 5 users. So I think I agree with this change 😅

Hey everyone, happy to share some context from the CKEditor 5 side.
A lot of people may not know the exact size of the container in which the content will be used. Often, content is used in multiple destinations at once. With pixels, one gets far less flexibility with how the content will be used in the future.

As for the example presented above, the markup is not correct for the figure+figcaption. The width needs to be moved to the <figure>. The styling we use to make it work can be found here: https://ckeditor.com/docs/ckeditor5/latest/installation/advanced/content.... Unfortunately, it's quite complex as HTML/CSS poses challenges.

One thing from https://github.com/ckeditor/ckeditor5/issues/5167#issuecomment-519087175 I'm not sure I agree with:

All the RTEs use pixels, but I think this is wrong. This makes the images completely unresponsive. How do you know how wide the content that you're displaying will be? The column with the text may have 900px on desktop, but 500px on mobile.

Therefore, I think we should use percentage values. This will ensure that images scale well with the content. E.g. we use percentage values in the default image styles (side image has max-width 50%).

A lot of responsive designs will show an image half-width on wide screens, but then make it full width when the screen is narrow, with any captions or etc. moved underneath instead of to the side. That way the image stays close to the same size (or at least closer) while the layout adapts around it. But... I don't know how this relates to the other image placement options.

100% agreement with @catch in #13 as it embiggens my point :D Though I'd like to be open to the possibility that this was taken into consideration already in their discussions, and maybe they have a proposal that's better than "the way it was done for decades" perspective that I'm taking.

Thanks very much for posting here @witeksocha and welcome to the Drupal community!

I'll add point in #12 to the issue summary about moving the pixels to the figure element.

As for the example presented above, the markup is not correct for the figure+figcaption. The width needs to be moved to the <figure>. The styling we use to make it work can be found here: https://ckeditor.com/docs/ckeditor5/latest/installation/advanced/content...

— @witeksocha in comment #3348603-12: Image resize with precent makes figcaption expand to width of container →

Implemented the proposed solution from #12 for people to test.

The last submitted patch, 16: 3348603-16.patch, failed testing. View results →

@lauriii, thanks for the patch, I tried it and for some reason it didn't work on my captions at all (no width on figure element) and without caption it was on the image.

% version from #16

px version from MR 3671

I resized it in ckeditor and copied the source changes on one image to all the variations for this manual test.

Sorry @lauriii, I should have realized that was only on Olivero theme because of the template change. This will change will be tricky because of the template change that would be needed for Classy/Stable/Stable9/Starterkit/Bartik/Claro/Seven/Umami...

Here's the same test but with that theme, the images without caption suffer from the same problem as above.

@lauriii also We need to grant the style attribute to the filtered attributes for figure.

I'm more convinced my MR request is the way to go here...

Also we've deemed style attribute on a figure a security risk:

The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: .

#19: I'm not sure I understand what is the problem there? It looks to me that the % version from #16 renders as it was intended to 🤔

#20: Are you running the "Limit allowed HTML tags and correct faulty HTML" after "Caption images" filter?

#21 is a good point. There are certainly some challenges even with this use of the style attribute because it could enable XSS through the width and height attributes 🤔

Here's an attempt to add BC layer for the concerns raised in #19.

The last submitted patch, 23: 3348603-23.patch, failed testing. View results →

Discussed this with @joelpittet in detail.

The potential solutions we discussed:

1. Change to px units (in the MR by @joelpittet)
2. Keep using % but move width to <figure> (#23)
3. Combination of both; allow configuring whether user prefers to use % or px as the resize unit.

@joelpittet confirmed that approaches 1 and 2 solves the original problems. The discrepancies between the screenshots in #19 were due to an incorrect alignment being set on one of the images.

I'm not entirely convinced that using percentages is more intuitive for content editors. For instance, a 50% width setting may be small on mobile but quite large on desktop. Users are generally familiar with static width units like inches, even if px isn't a perfect measure.

Solution 2 has potential XSS concerns since it involves the style attribute. Although we can manage this, it requires conscious effort.

Percentage values aren't necessary for responsive design. The width attribute can be used for wider screens, while max-width ensures responsiveness.

From the user perspective it seems that px is the way to go. My main concern with px units is their potential impact on backwards compatibility. I think the ideal solution would be that existing width values remain unchanged until the image is resized again.

Thanks @lauriii,

#20: Are you running the "Limit allowed HTML tags and correct faulty HTML" after "Caption images" filter?

To confirm RE #22 That in #20 you are right the order of the filters were in the wrong order, and yes one of the alignments was incorrect. Take the screenshots with a grain of incompetence on my part;)

And a note if we continue down the % exploration that we need to remove height on the <figure> because with captions that is just wrong.

Yes, both solutions will solve my original issue around the Figure captions not respecting the image width when percents are used.

Re BC layer, currently we have the same BC regression that the px MR will produce. Content created/edited before the upgradig to ckeditor5 are using px, only modifying that content after will change them to % This is a note to @Wim Leers as I'm sure @lauriii will bring it up with you this week.

@catch in #13 and @joelpittet in #14: Yep, that's because they have fundamentally different assumptions for the default environment CKEditor 5 will run in. See the last paragraph I wrote in #11.

#16 – #23 are implementing and testing what @witeksocha described in #12 what Drupal would need to change to make image captions work correctly with resized images.

#25 summarized the whole issue and potential choices to make, he proposed a plan.

I just discussed #25 in detail with @lauriii. I'm convinced his proposal to switch to px (like @joelpittet's original MR does) is the way to go 👍

Also 100% agreed with @joelpittet in #26 that this is simply an oversight on our part when we brought CKEditor 5 to Drupal core: we didn't sufficiently question whether the shift from % to px was a problem or not. Sorry. 🫣

My only remaining concern: making sure that editing content that is using <img width="34%"> actually gets converted to a pixel value AND that it's not a jarring experience. A simple experiment proves that it works great, out of the box:

🤩

… but ideally we'd have functional JS test coverage that proves that that continues to work fine in future versions of CKEditor 5. If they turn out to be possible to write, because we know that anything drag-and-drop related is extremely difficult to simulate with chromedriver-powered tests 😬 @lauriii will attempt to write such a test next week 🥳

I tried to write tests for this and it kind of works (assertions pass) but for some reason CKEditor is throwing an error 😬

@laurii, what's the error CKEditor is throwing and where?

The error I'm getting when running \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest:testWidth is:

TypeError: this._native.setData is not a function
    at Eo.setData (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:210399)
    at listenTo.priority (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:623183)
    at Object.fire (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:550946)
    at We (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:157748)
    at He.fire (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:157139)
    at r.fire (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:200201)
    at r.onDomEvent (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:617840)
    at listenTo.useCapture (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:200071)
    at he.fire (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:550946)
    at HTMLDivElement.t (http://localhost/core/assets/vendor/ckeditor5/ckeditor5-dll/ckeditor5-dll.js?v=38.0.0:5:560955)
 /Users/lauri.eskola/Projects/drupal/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:133

However, this is specific to the test. For some reason it also looks like this is not happening on DrupalCI 🤯

Reasons it may not be happening on DrupalCI:

• somehow if the site is served not from the root but from a subdirectory of a domain name, the error does not occur
• DrupalCI runs Chrome headlessly.
• DrupalCI uses an older Chrome version!

Could you try eliminating those, @lauriii?

Discussed briefly with @lauriii in slack and we both think we should do #28 minus the test coverage, and open a follow-up to add it, so that this can definitely land in 10.1

I think we should try to get this bug fix in 10.1.x. More content editors will likely run into this and this isn't something we could fix in a patch release. Here's a patch without the test coverage. We still need to write a CR for the change.

And here's the patch 😇

Added a draft CR.

Tagging for follow-up to add the tests, although if it's hard to write a test that passes, we also might just want to skip that.

+1 for commit without test if test infra is getting in the way — this has been thoroughly manually tested!

Opened new issue for the tests: 📌 Add tests for resizing images in CKEditor 5 Active .

Committed 30a54c9 and pushed to 11.x. Also cherry-picked to 10.1.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.