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

Setting image height and width #5154

Closed
olup opened this issue Jun 27, 2018 · 26 comments
Closed

Setting image height and width #5154

olup opened this issue Jun 27, 2018 · 26 comments
Labels
package:image resolution:resolved This issue was already resolved (e.g. by another ticket). squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@olup
Copy link

olup commented Jun 27, 2018

Is this a bug report or feature request? (choose one)

🆕 Feature request

📃 Other details that might be useful

Hi ! Thanks for the very well designed ckeditor 5 and its image plugin. I have two / three questions that can be interpreted as feature requests.

Many front end, including mine, actually uses images parameters sizes to build progressive loading, placeholders, etc... We love our non-jumping image process, and for that it is critical that height and width are hardcoded in the img tag (indeed, only to compute an aspect ratio). From what I can understand in the image plugin code only url and alt and styles are setable on the image bloc object. Would it be possible to consider adding those parameters, or is it something i should extend myself (I am not talking PR but adding a plugin on top of this one) ?

Finally, something that don't really belong here, but i cannot find any equivalent plugin to manage embed (like a youtube video) in the editor.
Is there work allready in this direction or is it uncharted territory ?

Cheers.

@Reinmar
Copy link
Member

Reinmar commented Jun 27, 2018

From what I can understand in the image plugin code only url and alt and styles are setable on the image bloc object.

It also can also set the width, sizes and srcset. E.g. if I drop an image on https://ckeditor5.github.io that's the resulting data:

<figure class="image">
    <img src="https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg" srcset="https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_80 80w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_160 160w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_240 240w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_320 320w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_400 400w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_480 480w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_560 560w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_640 640w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_720 720w, https://33333.cdn.cke-cs.com/rc1DFuFpHqcR3Mah6y0e/images/fc0afb2a3bcfcd2b82139fdbc72eb063f4a8cc8d632a833a_sample.jpg/w_800 800w" sizes="100vw" width="800">
</figure>

However, I don't understand why we don't set the height too... I think that we needed width so it works well with sizes and srcset. But, for some reason, we omit height.

@pjasiun @oleq, do you remember any reasons for not setting the height?

Finally, something that don't really belong here, but i cannot find any equivalent plugin to manage embed (like a youtube video) in the editor.
Is there work allready in this direction or is it uncharted territory ?

Please report it in a separate ticket. Video embedding (from YT, Vimeo, etc) is high on our priority list, but it wasn't reported yet.

@Reinmar Reinmar changed the title image width and height Setting image height and width Jun 27, 2018
@pjasiun
Copy link

pjasiun commented Jun 27, 2018

@pjasiun @oleq, do you remember any reasons for not setting the height?

I believe we wanted to set as few attributes as possible, and we did not see any reason to set height. However, your argument, @olup, is very good and I agree that we should set it.

@the-owl
Copy link

the-owl commented Oct 2, 2018

Is there a way to parse existing width attributes from HTML when using editor.setData?
I tried to extend image support (perhaps not quite correct):

editor.model.schema.extend('image', {
  allowAttributes: 'imageWidth'
});

editor.conversion.for('downcast').add(downcastAttributeToAttribute({
  model: 'imageWidth',
  view: 'width'
}));

editor.conversion.for('upcast').add(upcastAttributeToAttribute({
  model: 'imageWidth',
  view: 'width'
}));

But achieved only this after editor.getData():

<figure class="image" width="100"><img src="..."></figure>

We need to set width on images because we use CKEditor for creating emails, and we can't use stylesheets to apply styling to content.

@jodator
Copy link
Contributor

jodator commented Oct 2, 2018

Related: https://github.com/ckeditor/ckeditor5-table/issues/122#issuecomment-424333590. Probably the same will happen with other widgets and/or elements that are not mapped 1:1.

@jodator
Copy link
Contributor

jodator commented Oct 4, 2018

@the-owl : please take a look at a similar solution for tables: https://github.com/ckeditor/ckeditor5-table/issues/122#issuecomment-426982275

I've re-phrased the downcast writer to match your code - but I didn't test it ;)

editor.conversion.for( 'downcast' ).add( dispatcher => {
	dispatcher.on( 'attribute:imageWidth:image', ( evt, data, conversionApi ) => {
		const image = data.item;

		// The table from the model is mapped to the widget element: <figure>.
		const viewFigure = conversionApi.mapper.toViewElement( image );

		// A <image> is direct child of a <figure> but there might be other view
		// (including UI) elments inside <figure>.
		const viewImage = [ ...viewFigure.getChildren() ]
			.find( element => element.name === 'img' );

		// User view writer to change style of a view table.
		if ( data.attributeNewValue ) {
			conversionApi.writer.setStyle( 'width', data.attributeNewValue, viewImage );
		} else {
			conversionApi.writer.removeStyle( 'width', viewImage );
		}
	} );
} );

@100cm
Copy link

100cm commented Oct 31, 2018

Hi, Should I Need to write plugin for this by myself and build the ckeditor ?

@mkopinsky
Copy link

Will the solution implemented as part of this also include resizing media embeds? Should I open a separate issue in https://github.com/ckeditor/ckeditor5-media-embed ?

@tbredin
Copy link

tbredin commented Aug 26, 2019

My team needs this also 👍

@Reinmar
Copy link
Member

Reinmar commented Aug 26, 2019

We've just released image resizing. You can see it live in nightly docs https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/image.html#resizing-images. Official docs will be updated later on today.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-image Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:image labels Oct 9, 2019
@rhysstubbs
Copy link

rhysstubbs commented Oct 13, 2020

I also need this feature.

Was it implemented? Some of the comments above are talking about image resizing etc. but I don't think that was what the OP was asking about.

Happy to write my own plugin... but it'd be amazing if someone has already done this ;)!?

-- EDIT

I thought I'd go ahead and do this, however I'm slightly confused as the viewToDom function won't return me a DOM node for the image. Since they stored in a WeakMap theres no way for me to iterate and get the correct image.

If I try and create a new image here, that'll obviously happen async and writer seems to fail after.

Is this the correct approach or is there a better way?

this.editor.conversion.for( 'downcast' ).add( dispatcher => {
  dispatcher.on( 'insert:image', ( evt, data, conversionApi ) => {
    const modelElement = data.item;
    const writer = conversionApi.writer;
    const viewFigure = conversionApi.mapper.toViewElement( modelElement );
    if ( !viewFigure || !writer ) {
      return;
    }
    const viewElement = findViewChild( viewFigure, 'img', conversionApi )
    const height = modelElement.getAttribute( 'height' ) || 0;
   if ( height > 0 ) {
      writer.setAttribute( 'height', height, viewElement );
      return;
    }
    const img = this.editor.editing.view.domConverter.viewToDom( viewElement );
     // img is undefined
  } );
}, { priority: 'low' } );

@neongreen
Copy link

We love our non-jumping image process, and for that it is critical that height and width are hardcoded in the img tag

Agreed. Otherwise we see jumps like this, which are very jarring if the image loads slowly:

@Reinmar Reinmar modified the milestones: backlog, nice-to-have Jul 19, 2021
@Reinmar
Copy link
Member

Reinmar commented Jul 19, 2021

Related issue: #8663.

@wimleers
Copy link

I do not understand how this is related to #8663. Could you elaborate? 😊 🙏

@Reinmar
Copy link
Member

Reinmar commented Jul 19, 2021

Currently, CKE5 does support width but AFAICS only if it's paired with sizes or something similar. E.g. the width attribute will be output when an image was pasted into the editor (from file). Additionally, with ImageResize present, the width inline style present on figure is also handled.

#8663 is about backward compat with CKE4 and we'll need to work in it on different notations of similar things. So if CKE4 could've output something, it will need to be handled in CKE5 as well.

@tomitrescak
Copy link

Hello, will the height setting be possible since that functionality has been implemented? This is important for SVG source of images. Currently, when you insert image and set SVG as its source, the height is capped at 150px, beyond which it is not possible to resize the image, making the scalability an issue. I spent already a week on this but cannot find a way out beyond manually setting the height and width of the image.

@Reinmar Reinmar added squad:core Issue to be handled by the Core team. and removed squad:compat labels Sep 27, 2021
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@Mgsy Mgsy added the support:2 An issue reported by a commercially licensed client. label Apr 11, 2022
@SazanYa
Copy link

SazanYa commented Jun 28, 2022

This feature is needed by many people, especially for creating emails. Outlook and Windows 10 Mail rendering engine doesn’t understand the style attribute on <img> elements. So we need to define a width attribute for correct displaying. And it would be nice if ImageResize plugin allowed us to set image sizes using this attributes.

@suracekumar632

This comment was marked as off-topic.

@jc-harbour
Copy link

Bump on this, img tags are getting width and height stripped from the rendered img tag. Any update on this?

@wimleers
Copy link

This has been reported as a bug against Drupal too now: https://www.drupal.org/project/drupal/issues/3336446#comment-14888366.

This bug has been known for a while now — is it really so complicated to fix? 😅

@KlemenDEV
Copy link

It would be really nice to have this fixed, agree. We and our users love CKE5 on our websites, but the CLS score has been hit quite severely on some of our pages due to the arguments missing.

@leevigraham
Copy link

leevigraham commented Feb 12, 2023

I'm working on an image-dimensions plugin. Just have to hook up the observable properties.

image

but this would 100% be better as a first party plugin.

@wimleers
Copy link

wimleers commented May 4, 2023

@leevigraham Could you share the code? 😊 🙏

@leevigraham
Copy link

@wimleers I didn't get much further than the screenshot above.

@Witoso
Copy link
Member

Witoso commented May 22, 2023

We are taking a look at this, if anyone wants to check the status, observe the #14146 and its subtasks.

@Witoso
Copy link
Member

Witoso commented Jul 14, 2023

The implementation is close to completion, but we would love to hear some feedback. The details are gathered in the comment.

@Witoso
Copy link
Member

Witoso commented Sep 21, 2023

🎉 This feature was merged to the master and is available on the nightly releases and nightly docs to test.

In a comment in another issue, you can find more details about the changes.

Gathering interest in the UI setup for those attributes in: #15044

@Witoso Witoso closed this as completed Sep 21, 2023
@Witoso Witoso added resolution:resolved This issue was already resolved (e.g. by another ticket). and removed status:discussion labels Sep 21, 2023
@Witoso Witoso added this to the iteration 67 milestone Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image resolution:resolved This issue was already resolved (e.g. by another ticket). squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests