When image is resized in CKEditor 4, both height and width attributes are added. The attributes persist if the text format is upgraded to use CKEditor 5. Now if user resizes the image, the width attribute is converted to use percentages. In this process, the height is left untouched which may lead into the image rendering with incorrect proportions in some browsers.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3336446_17.patch | 2.72 KB | wmcmillian |
| #8 | 3336446-7-test-only-FAIL-rerolled.patch | 1.62 KB | frega |
| #4 | 3336446-4.patch | 2.93 KB | wim leers |
| #4 | 3336446-4-test-only-FAIL.patch | 1.76 KB | wim leers |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !3326
Comments
Comment #2
wim leersAFAICT that'd be an upstream bug in
ckeditor5-image'sImageResizeplugin?https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-ima... indeed seems to have zero handling for
height…Yep, https://github.com/ckeditor/ckeditor5/issues/5154 specifically covers this.
Comment #3
wim leersThis is actually caused by
\Drupal\editor\Plugin\Filter\EditorFileReference::process(). It blindly computesheightandwidthif either one is not set, by inspecting the referenced file. But … it does not take into account that a%value could be set on either one.Even though that is technically not valid in HTML 5 (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Img#attr-width), it was historically allowed so we should cater for that too (https://en.wikipedia.org/wiki/Robustness_principle).
This bug was introduced in #3167034: Leverage the 'loading' html attribute to enable lazy-load by default for images in Drupal core.
Comment #4
wim leersComment #6
frega commentedAssigning this to me, to reroll patch.
Comment #8
frega commentedRerolled as MR.
Adjusted `3336446-4-test-only-FAIL.patch` to reflect removal of `loading="lazy"` attributes in this filter (@see https://git.drupalcode.org/project/drupal/-/commit/a266ba6e).
Comment #9
frega commentedComment #11
frega commentedSetting back to needs review, as the last patch should _fail_ (rerolled test-only patch).
Comment #12
wim leersThanks @frega! (Also: long time no see! Hope you're well! 😊)
Comment #13
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Think looks interesting. Could the issue summary be updated with proposed solution and steps to reproduce please. Also this will keep failing as the test only patch was the last uploaded. Actually surprised to see it survived the review bot.
Comment #14
wim leersWhile we should definitely do this hardening, I think #3348603: CKEditor 5 resizes images with % width instead of px width (the CKEditor 4 default): breaks image captions *and* is a regression would actually solve this even better? All followers of this issue should review that!
Comment #16
wim leers#3348603: CKEditor 5 resizes images with % width instead of px width (the CKEditor 4 default): breaks image captions *and* is a regression landed. Upon re-editing content with CKEditor 5 and re-saving it, CKEditor 5 will convert the % width to pixels instead.
That makes this a fix for a fairly obscure problem. Except that all resized images created with CKEditor 5 so far on all Drupal >=9.5 sites would definitely benefit from this.
So keeping the same priority.
Comment #17
wmcmillian commentedre-rolled #4 for 10.2.x