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

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

… and even more so to #3336446: EditorFileReference should compute a px <img height> if a % <img width> is specified, even though % <img width> is not allowed in HTML5.

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.

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

#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.

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.

@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 😬

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: #3362462: Add tests for resizing images in CKEditor 5.

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