[drupalImage] <img width> upcast assumes HTML5: px unit, but HTML4 allowed % unit

@lauriii responded in the #ckeditor5 Drupal Slack channel, where you asked aobut this. He thinks it's it's related to https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/cked... — credited him for that.

This would be consistent with the steps to reproduce you posted: it only happens if you start editing already existing content.

I understand this blocks #3224652: [drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing, or at least your use of it in #3247974: [drupalImage] Image resize, Image with text wrapped shown correctly on front end and not correctly shown in backend. So tagging blocker.

This will need tests.

Since this could be considered data loss, marking critical. I hope to start working on this tomorrow (today is already a very full day). So, self-assigning, but if somebody starts working on this before 09:00 CEST Nov 18 2021, feel free to assign to yourself.

That code was introduced in #3222847: <img width height>.

you need to fix .es6 files as https://www.drupal.org/about/core/policies/core-change-policies/frontend... and .js files will be genareated

Applied changes by vlyalko onto a new 9.4.x MR

Very nice of you to respect the commit credit in the merge request, @hooroomoo! 🤩👏❤️

Looks like I reviewed the wrong MR 😅 🙈

None of the review comments between #16 and #17 are relevant. Reviewed the correct MR now!

@Wim Leers @lauriii How do you resize an image without the plugin? Is it something with HTML tags? I originally thought to change the style attribute directly inside of my test but realized later that's probably not the way to do it.

This should work:

1. click the "Source" button
2. modify <img width> to for example width="33%".
3. click "Source" again

Your width of 33% should persist..

removed needs test tag

The test was passing for me, then I removed the two image dependencies that I had originally added and saw now that the test failed.

I added back the image.imageResize dependency because the test seems to fail without it. The element doesn't get the width attribute that is added inside the Source so the size that shows up in the editor is wrong.

Not sure if that makes this blocked until the dependency is added in #3224652: [drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing.

Hm … I'd like to understand why it fails. What did your digging unveil? Where/how does the width attribute value you manually specified get lost? Is it in CKEditor 5's code or is it in Drupal's DrupalImage CKEditor 5 plugin (in core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/*)? 🤔

We may need to block this on #3224652: [drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing. But even then we'd want to understand why we need that additional plugin for this to work correctly.

So either way, the investigation/digging that I mentioned at the beginning of this comment is necessary IMHO.

1. So I tried changing the width through source editing on CKE5's demo page and confirmed that it doesn't resize the image there either so I think it's safe to assume it's not because of Drupal's DrupalImage.

2. Then I found these lines of code 116-118 in CKE5's resize plugin that looks it is what's setting the width to the figure from the image.

	if (data.attributeNewValue !== null ) {
			viewWriter.setStyle( 'width', data.attributeNewValue, figure);
		         viewWriter.addClass( 'image_resized', figure ); 

3. Also I am including this comment I found from someone who works on CKEditor 5 which makes it sound like the ImageResize plugin is necessary for the figure style to also be updated. (Also Wim I was pleasantly surprised to see your avatar right above)

So this is what I was able to find in my investigation. I am not 100% confident but my guess is that the width attribute manually specified doesn't bubble up to the figure element without the resize plugin that sets the styling for width.

Also just found https://github.com/ckeditor/ckeditor5/issues/10838, which seems to further confirm this is a limitation of CKE5's Image plugin.

This is something that we tried to work around in our code but I'm not sure we have test coverage for that. That's what the width/height upcast and downcast are supposed to solve because I think earlier we decided that we should preserve width/height attributes even if the resize plugin isn't enabled. So I think we should come up with a bug fix and test coverage regardless of what happens with #3224652: [drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing.

I think @lauriii's comment confirms that this is already Drupal-owned/specific, which means the fix for this can also be Drupal-specific. So this is not blocked on #3224652: [drupalImage] Add ckeditor5-image's imageresize plugin to allow image resizing.

@lauriii, could you maybe leave some pointers for @hooroomoo to dive into core/modules/ckeditor5/js/ckeditor5_plugins/drupalImage/*?

Per #26 and #27.

Data is upcasted properly now with the expected units but the image itself does not resize. Created another issue #3260479: [drupalImage] <img width> not visible in editing view (because only for data downcast) since it is out of scope for this specific issue.

Reviewed. This is definitely on the right track, but still has a problem — which is super easy to spot now thanks to the test coverage @hooroomoo wrote! 👍😄

Thanks also for opening #3260479: [drupalImage] <img width> not visible in editing view (because only for data downcast) :)

Finally … in my review on the merge request I pointed to https://html.spec.whatwg.org/multipage/embedded-content-other.html#attr-... … but curiously, that did not mention relative units. Turns out that the HTML spec does not allow specifying units in there. Technically <img width="33%"> is therefore invalid HTML. But, of course, HTML being HTML … there's an exception: in HTML 4.01 this used to be valid: https://www.w3.org/TR/html401/struct/objects.html#h-13.7.1

Since we don't want to break pre-existing content, we do have to support this scenario. Retitling for clarity

It seems like CKEditor is outputting the px unit when the resize unit is set to px. I guess that's something that we should report upstream, and is the likely reason why we implemented this the way it is.

I checked what CKEditor 5 expects for the module attribute, and it expects that the unit is always set. So what we want to do is ensure on upcast that width and height with px unit, include px when set to the model attribute. However, that px unit should be removed when downcasting the data.

#32:

I guess that's something that we should report upstream, and is the likely reason why we implemented this the way it is.

And I bet it's because they use style … and not even on <img>, but on the wrapping <figure> 😅. That's also what https://ckeditor.com/docs/ckeditor5/latest/features/images/images-resizi... shows.

Worth bringing up in tomorrow's meeting with CKEditor 5 folks?

But definitely out of scope here! Tagging Needs followup for that new issue, not yet creating it so we can include their upstream commentary/guidance right from the start 👍

Based on #33, I don't think there's problem with the upstream behavior. I think it's fine for them to output the value in style attribute on the <figure> element. Removing the needs followup tag based on that.

I pushed 8320cf55, 0c085d8f, b4861511 and 8d74730f to fix nits.

The only actual change I made is 31d1db8c, which just simplifies the test.

All of the actual work here was done by @hooroomoo. Hence I feel comfortable RTBC'ing this :)

Oohhhh … really good point, @lauriii! I totally forgot about that.

After you do that, @hooroomoo, this'll be RTBC again :)

🚢

Committed 27af96d and pushed to 10.0.x. Also committed the MR to 9.4.x Thanks!

Leaving open for 9.3.x backport once the code freeze has ended.

Cherry-picked to 9.3.x.