ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.5k stars 3.7k forks source link

Setting image height and width #5154

Closed olup closed 1 year ago

olup commented 6 years ago

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 commented 6 years ago

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.

pjasiun commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 5 years ago

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

mkopinsky commented 5 years ago

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 commented 5 years ago

My team needs this also πŸ‘

Reinmar commented 5 years ago

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.

rhysstubbs commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

Related issue: #8663.

wimleers commented 3 years ago

I do not understand how this is related to #8663. Could you elaborate? 😊 πŸ™

Reinmar commented 3 years ago

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 commented 3 years ago

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.

SazanYa commented 2 years ago

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 commented 2 years ago

pexels-cottonbro-4761788

jc-harbour commented 2 years ago

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

wimleers commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@leevigraham Could you share the code? 😊 πŸ™

leevigraham commented 1 year ago

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

Witoso commented 1 year ago

We are taking a look at this, if anyone wants to check the status, observe the https://github.com/ckeditor/ckeditor5/issues/14146 and its subtasks.

Witoso commented 1 year ago

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

Witoso commented 1 year ago

:tada: 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