Opened 9 years ago

Closed 5 years ago

Last modified 3 years ago

#5547 closed Bug (fixed)

Image width and height should use width and height HTML attributes

Reported by: John Fletcher Owned by: Jakub Ś
Priority: Normal Milestone:
Component: UI : Dialogs Version: 4.4.7
Keywords: Cc: MrM, damian.chojna@…, satya_minnekanti@…, drewbdulgar@…, jrwilson3@…, ckeditor@…

Description

Previously FCKeditor converted what was specified in the image width and height boxes into the html attributes width and height. Now I'm using the latest release of CKEditor and noticed it puts the image width and height into the style attribute. I think we should return to the former behaviour.

Perhaps the change was made as part of this ticket http://dev.fckeditor.net/ticket/4246 - I'm not sure.

Width and height attributes are not deprecated neither in XHTML nor HTML5 when used on images, because it's better for rendering when this data is in the HTML source code. Technically it's equally OK to have it as an attribute or an inline style, however using attributes is better because it's easier to read and parse the code. Additionally you can simplify your CKEditor code - take out the code which has to synchronise the width/height fields with the styles field. By the way, in case of conflict HTML attributes will take precedence over inline styles.

Personally I'm requesting the change because I have to parse the HMTL CKEditor produces and it's easier when they are attributes. Incidentally I find it makes the HTML more logical and easier to read.

Reference: http://www.w3.org/TR/2010/WD-html5-20100304/the-canvas-element.html#attr-dim-width

Attachments (3)

image-width-and-height-attributes.diff (3.3 KB) - added by Pivotal Labs 6 years ago.
Patch to include width and height attributes on images added
image-width-and-height-attributes.patch (4.8 KB) - added by James Wilson 4 years ago.
Patch for CKEditor 4.4.7 source code
image.js (21.0 KB) - added by James Wilson 4 years ago.
Drop-in replacement for compressed 4.4.7 plugins/image/dialog/image.js

Download all attachments as: .zip

Change History (46)

comment:1 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Additionally you can simplify your CKEditor code - take out the code which has to synchronise the width/height fields with the styles field.

Not really. When an image is resized the browser can choose to put those changes as inline styles so the synchronization is always needed, one way or another.

By the way, in case of conflict HTML attributes will take precedence over inline styles.

It's just the other way: inline styles take precedence over any attribute.

In the mean time, you can use this sample to find out how to output those values as attributes: http://dev.fckeditor.net/attachment/ticket/5024/5024_5.patch

comment:2 Changed 9 years ago by John Fletcher

Resolution: wontfix
Status: newclosed

"It's just the other way: inline styles take precedence over any attribute." Ahh you are right - sorry.

Well don't worry about it. I'll just parse the inline styles.

comment:3 Changed 9 years ago by MrM

Cc: MrM added
Resolution: wontfix
Status: closedreopened

Sorry to reopen this bug, but saying "inline styles take precedence over any attribute" does not address the original issue.

I see no reason why you would use css for setting image dimensions in the first place. The width and height attribute of the img tag were never deprecated. In fact, developers are encouraged to use them, according to most (X)HTML guidelines and best practices.

Additionally, using css for dimensions results in the following situation:

When I insert an image, the dimensions get set automatically. However, if I then set a custom style on the image, the dimensions disappear from the source code. This is probably due to the fact that the style attribute gets overwritten with the custom css from the selected style. This problem could easily be avoided by using the width and height attributes instead of css.

comment:4 Changed 9 years ago by Rebecca Macaulay

CKEditor not using the image width and height attributes is a major problem with my inline images. My site is being built with Drupal, and many of the contributed modules use these attributes to function correctly. There's no reason to not have image height and width be in its appropriate place. Others using Drupal have voiced the same concerns (http://drupal.org/node/731068).

comment:5 Changed 7 years ago by Marcus Bointon

Type: New FeatureBug

I have a big problem with this too. I use the editor for editing content that's used in HTML email, and quite a few web mail clients strip inline styles, so images end up with no dimensions at all, usually causing layouts to fall apart.

I agree with MrM: there is no reason to prefer CSS over HTML for this purpose: the image dimensions are not really styling information but additional metadata (i.e. attributes) about the image. If the image was to be displayed at a different size than the real content, then I would class it as styling.

I'd also class this as a bug rather than a feature, and a regression at that since ckeditor used to do this right.

Last edited 7 years ago by Marcus Bointon (previous) (diff)

comment:6 Changed 7 years ago by Marcus Bointon

There's a thread on stackoverflow about this. One of the solutions on there worked for me, though it post-processes the ckeditor output in order to undo the placement of width and height in styles, which just seems horribly wrong, and leaves you with an empty style attribute. Another observation in that thread is that Outlook may refuse to display images at all if they are missing width and height attributes!

The patch linked on this page does much the same, but it's less usable as it's hard coded for a single static instance - mine are generated dynamically and I often have more than one instance.

It looks like this should be patched in this file, around line 660, but there are lots of things going on with width and height in there. Anyone have a better patch?

comment:7 Changed 7 years ago by Alfonso Martínez de Lizarrondo

The solution provided is just based on the output HTML sample: http://nightly.ckeditor.com/latest/ckeditor/_samples/output_html.html

You should take a look at it because it will probably include other interesting info for your usage.

It doesn't really matter too much if the editor generates styles or attributes in the image dialog, resizing the images in the browser will usually generate them as styles, so either you or the editor would still have to run the same code proposed there to be sure that there are no styles.

comment:8 Changed 7 years ago by Marcus Bointon

OK, I've got it working by adapting that code into one of the other suggestions I was using and placing it in my custom JS config file, triggering it from instanceready, which seems to do the trick. This way it works for multiple instances without knowing their names/ids. It still ends up with a style attribute containing a single space, but I can live with that. This is the code I used:

CKEDITOR.on('instanceReady', function (ev) {
	var editor = ev.editor,
		dataProcessor = editor.dataProcessor,
		htmlFilter = dataProcessor && dataProcessor.htmlFilter;

	// Out self closing tags the HTML4 way, like <br>.
	dataProcessor.writer.selfClosingEnd = '>';

	// Make output formatting behave similar to FCKeditor
	var dtd = CKEDITOR.dtd;
	for ( var e in CKEDITOR.tools.extend( {}, dtd.$nonBodyContent, dtd.$block, dtd.$listItem, dtd.$tableContent ) )
	{
		dataProcessor.writer.setRules( e,
			{
				indent : true,
				breakBeforeOpen : true,
				breakAfterOpen : false,
				breakBeforeClose : !dtd[ e ][ '#' ],
				breakAfterClose : true
			});
	}

	// Output properties as attributes, not styles.
	htmlFilter.addRules(
	{
		elements :
		{
			$ : function( element )
			{
				// Output dimensions of images as width and height
				if ( element.name == 'img' )
				{
					var style = element.attributes.style;

					if ( style )
					{
						// Get the width from the style.
						var match = /(?:^|\s)width\s*:\s*(\d+)px/i.exec( style ),
							width = match && match[1];

						// Get the height from the style.
						match = /(?:^|\s)height\s*:\s*(\d+)px/i.exec( style );
						var height = match && match[1];

						if ( width )
						{
							element.attributes.style = element.attributes.style.replace( /(?:^|\s)width\s*:\s*(\d+)px;?/i , '' );
							element.attributes.width = width;
						}

						if ( height )
						{
							element.attributes.style = element.attributes.style.replace( /(?:^|\s)height\s*:\s*(\d+)px;?/i , '' );
							element.attributes.height = height;
						}
					}
				}

				// Output alignment of paragraphs using align
				if ( element.name == 'p' )
				{
					style = element.attributes.style;

					if ( style )
					{
						// Get the align from the style.
						match = /(?:^|\s)text-align\s*:\s*(\w*);/i.exec( style );
						var align = match && match[1];

						if ( align )
						{
							element.attributes.style = element.attributes.style.replace( /(?:^|\s)text-align\s*:\s*(\w*);?/i , '' );
							element.attributes.align = align;
						}
					}
				}

				if ( !element.attributes.style )
					delete element.attributes.style;

				return element;
			}
		},

		attributes :
			{
				style : function( value, element )
				{
					// Return #RGB for background and border colors
					return convertRGBToHex( value );
				}
			}
	} );
});

/**
* Convert a CSS rgb(R, G, B) color back to #RRGGBB format.
* @param Css style string (can include more than one color
* @return Converted css style.
*/
function convertRGBToHex( cssStyle )
{
	return cssStyle.replace( /(?:rgb\(\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\))/gi, function( match, red, green, blue )
		{
			red = parseInt( red, 10 ).toString( 16 );
			green = parseInt( green, 10 ).toString( 16 );
			blue = parseInt( blue, 10 ).toString( 16 );
			var color = [red, green, blue] ;

			// Add padding zeros if the hex value is less than 0x10.
			for ( var i = 0 ; i < color.length ; i++ )
				color[i] = String( '0' + color[i] ).slice( -2 ) ;

			return '#' + color.join( '' ) ;
		 });
}

comment:9 Changed 7 years ago by Jyo

We too use the editor for editing content that's used in HTML email, There is a problem if the width and height are added as the styles,images are not getting displayed as expected for outlook email after modifying the width and height in image properties, We need the clear solution to it to work on ASAP as it is impacting our business.

comment:10 Changed 7 years ago by Marcus Bointon

The code I posted works just fine for me (and I need it for exactly the same reason as you) so I suggest you use that. Remember that this is an open-source project - nobody owes you anything; if you want it fixed, fix it and provide a patch, or sponsor someone to do it, don't just complain.

comment:11 Changed 7 years ago by Jakub Ś

Resolution: wontfix
Status: reopenedclosed

@Jyo - I have read the whole post. There are 3 solutions presented!!!

  1. One by @alfonsoml (can be also found in output for HTML sample in CKEditor)
  2. Second very similar to the first one is presented in http://stackoverflow.com/questions/2051896/ckeditor-prevent-adding-image-dimensions-as-a-css-style by Marco Cortellino and Cedric Dugas
  3. Third by @Synchro

NOTE: @alfonsoml solution doesn't have to be used in HTML page. It can be used in config.js file. That way it will affect all instances of CKEditor.

I'm talkig about @alfonsoml solution in particular because this is the same way as it is handled in output for HTML sample in CKEditor. This is the general way of doing that. You can adjust this solution to your particular needs.

I’m closing the ticket.

comment:12 Changed 7 years ago by Marcus Bointon

The main issue I have with all of the above solutions (including mine) is that I don't think they should be necessary. Image sizes should go in HTML attributes - CKEditor is the only place I've ever seen that only uses CSS for image sizing, and it came as something as a shock to find out that it was doing it in the first place.

comment:13 Changed 7 years ago by Frederico Caldeira Knabben

In fact, it looks like we went too far on #4246, by making everything based on styles. Not to say that it is wrong to use styles for it, but it is not wrong to use attributes as well.

Considering the compatibility impact of it, I think we should do a step back and enforce the usage of attributes for width and height.

Note that we'll be forced to move width and height styles to attributes at that point.

comment:14 Changed 7 years ago by Marcus Bointon

Thanks. One small thing I also needed to do was to make sure that the style tag was omitted altogether if there were no styles. It was sometimes putting in style=" " otherwise.

comment:15 Changed 7 years ago by Jakub Ś

Resolution: wontfix
Status: closedreopened

comment:16 Changed 7 years ago by Jakub Ś

Status: reopenedconfirmed

comment:17 Changed 7 years ago by Jakub Ś

Owner: set to Jakub Ś
Status: confirmedassigned

comment:18 Changed 7 years ago by Damian

Cc: damian.chojna@… added

comment:19 Changed 7 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

comment:20 Changed 7 years ago by Jakub Ś

#8969 was marked as duplicate.

When you add an image to the content and set the image width different from the real size and then use the formatting style pulldown menu eg. image on right, the image looses the widt and height parms. This can be recreated on the demo example.

Last edited 7 years ago by Jakub Ś (previous) (diff)

Changed 6 years ago by Pivotal Labs

Patch to include width and height attributes on images added

comment:21 Changed 6 years ago by Pivotal Labs

We ran into this issue, too. Our app uses CKEditor to compose HTML emails, and many email clients (e.g. Outlook 2010) do not respect the inline style attributes for width and height. The attached patch applied to CKEditor 3.6.4 solved the issue for us.

comment:22 Changed 6 years ago by posabsolute

I would just like to add a +1 for having this issue resolved,

This cause us major issues when we upgrade ckeditor

comment:23 Changed 6 years ago by Patrick Poulain

+1 with posabsolute

comment:24 Changed 6 years ago by Andrew

I'm using the ASP.NET version of CKEditor 3.6.4 and the code in the "image\dialogs\image.js" file looks a lot different than the patch above.

Is there an equivalent fix for the ASP.NET version?

Thanks

comment:25 Changed 6 years ago by Brad

I would like to throw in my vote for this, too. Having width and height attributes set with the "style" attribute breaks some responsive design features. In a CSS file, we can set an image's max-width to 100% and height to auto to allow it to expand and contract. However, the inline style's width and height values override this and breaks the desired effect. Setting width and height with the "width" and "height" properties does not break this, and those properties are not deprecated. So, this should be a big consideration since many sites are aiming to be responsive these days.

comment:26 Changed 6 years ago by Drew

Cc: drewbdulgar@… added
Version: 4.1.2 (GitHub - master)

Hello,

This alteration in behavior with CKEditor has really messed up a lot of our websites. We recently upgraded from CKEditor 3 to CKEditor 4 and in doing so, any attribute that was previously set is removed from the source. So for instance, if we previously had:

<img border="2" width="100" height="500" src="/path/to/image.gif" />

saved as content, it is now rendered within CKEditor 4 as:

<img src="/path/to/image.gif" />

This requires modifications to the source code over hundreds of thousands of pages to convert this to the now inline css style setup. Unless there is a configuration option to handle this which I have not yet seen.

comment:27 Changed 6 years ago by Jakub Ś

This is because of ACF default settings . Please see: http://ckeditor.com/blog/Upgrading-to-CKEditor-4.1
http://ckeditor.com/blog/CKEditor-4.1-RC-Released
http://docs.ckeditor.com/#!/guide/dev_advanced_content_filter
http://docs.ckeditor.com/#!/api/CKEDITOR.filter-method-addTransformations
http://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-allowedContent

You can set ACF to use attributes for width height and border instead of styles.

Please also have a look at ckeditor_git\plugins\htmlwriter\samples\outputhtml.html

comment:28 Changed 5 years ago by Piotrek Koszuliński

Resolution: fixed
Status: assignedclosed

Fixed by the image2 plugin introduction. It uses attributes instead of styles.

comment:29 Changed 4 years ago by Duda

Even image2 plugin automatically adds the width and height. In displayed boxes of height and width. Please tell me any way to not set height and width after image upload. Thanks,

comment:30 Changed 4 years ago by James Wilson

Cc: jrwilson3@… added
Component: GeneralUI : Dialogs
Version: 4.1.24.4.7 (GitHub - master)

This is actually still a problem in master branch -- the image plugin is still there and still widely used -- so I've created pull request #157 on github to properly fix this issue in the image plugin, partially based on the patch file provided by pivotal on 21 above.

If I can figure it out, I'll add a patch file here too, as well as a drop-in version of the compressed image.js file that should work against a CKEditor 4.4.7 build.

Changed 4 years ago by James Wilson

Patch for CKEditor 4.4.7 source code

Changed 4 years ago by James Wilson

Attachment: image.js added

Drop-in replacement for compressed 4.4.7 plugins/image/dialog/image.js

comment:31 Changed 4 years ago by Jakub Ś

@jameswilson but this issue have been fixed in CKEditor 4.x. Please see http://docs.ckeditor.com/#!/guide/dev_acf-section-example%3A-disallow-inline-styles-and-use-attributes-instead.

All you need to do is use ACF.

comment:32 Changed 4 years ago by Piotrek Koszuliński

@j.swiderski - ACF transformations are applied only to the input and output but not while editing. @jameswilson's patch makes the image feature producing attrs or styles depending on the ACF configuration which is a deeper kind of integration.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:33 Changed 4 years ago by Piotrek Koszuliński

I updated my previous comment, because I forgot that transformations are also applied on output data. However, they do not affect the HTML created by the image dialog, so regardless of how editor is configured it still creates styles so while editing we've got styles (what blocks responsive images).

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:34 Changed 4 years ago by BoffinbraiN

Cc: ckeditor@… added

I've just updated my site to refer to the latest CDN version - 4.4.7 - but this bug is still present. It still adds inline styles and no attributes to images.

I have users that aren't experienced with HTML and telling them to manually alter the source for every image they add isn't feasible. The inline styles are breaking our responsive bootstrap CSS.

Please advise?

comment:35 Changed 4 years ago by Jakub Ś

Have you tried perhaps comment:31 ? If you have ACF set and you submit data in traditional way or get it from editor with getData() method then styles will be converted to attributes.

comment:36 Changed 4 years ago by Piotrek Koszuliński

Please advise?

You can use the Enhanced Image plugin which uses attributes only. Or you can use the patch provided by jameswilson in comment:30 which changes behaviour of the old image plugin, but I really recommend using the Enhanced Image plugin.

comment:37 Changed 4 years ago by Piotrek Koszuliński

Have you tried perhaps comment:31 ? If you have ACF set and you submit data in traditional way or get it from editor with getData() method then styles will be converted to attributes.

That's not the best solution as I explained in comment:32. It works with data, but not inside the editor. Only if we accepted the PR #157 it would work, but I don't know when we'll have time to properly review it.

comment:38 Changed 3 years ago by Ned

This is indeed a serious problem as inline styles override external styles - or to put that simply, using this will now ruin 99% of stylesheets out there that sized the images in some way (such as basically every responsive stylesheet).

comment:39 Changed 3 years ago by Piotrek Koszuliński

Did you try using the Enhanced Image plugin? It uses attributes.

comment:40 in reply to:  39 Changed 3 years ago by Alfonso Martínez de Lizarrondo

Replying to Reinmar:

Did you try using the Enhanced Image plugin? It uses attributes.

Yes, it uses attributes, but while editing the content isn't an img, so Styles defined for img elements aren't available in the Styles combo, and also trying to use a rule like

img {
max-width:100%;
height: auto;
}

won't work and the image will be displayed beyond the limits of its container (and I hope that you don't use any rule like .container>img, etc...).

You can drag your feet all that you want, but the fact is that this behavior should have been fixed 5 years ago without requiring the usage of another element that has its clear set of problems.

comment:41 Changed 3 years ago by Piotrek Koszuliński

Yes, it uses attributes, but while editing the content isn't an img, so Styles defined for img elements aren't available in the Styles combo, and also trying to use a rule like

To style image2 you have to use the widget styles – http://docs.ckeditor.com/#!/guide/dev_styles-section-widget-styles They will behave the same way as typical image styles (see http://ckeditor.com/tmp/4.4.0/widget-styles.html).

Plus, if you need to style anything using the stylesheets, styling img will of course work, as it is an image deep down. Many times we put img { max-width:100%; height: auto; } in our samples and there was no problem with that.

However, I admit that there are few issues with widget styles (part of them is shared with styles for normal images). We are working on these issues right now: #13200.

And as for the old image plugin – there was a PR made, but it wasn't finished (comment:30). We'd be super thankful if someone took it over.

comment:42 Changed 3 years ago by Marek Lewandowski

cc

comment:43 in reply to:  41 Changed 3 years ago by Alfonso Martínez de Lizarrondo

Replying to Reinmar:

To style image2 you have to use the widget styles – http://docs.ckeditor.com/#!/guide/dev_styles-section-widget-styles They will behave the same way as typical image styles (see http://ckeditor.com/tmp/4.4.0/widget-styles.html).

Even if all of that works correctly (you admit that there are known issues with widget's styles), the StyleSheetParser doesn't create such styles, so that's another burden to use Image2

Plus, if you need to style anything using the stylesheets, styling img will of course work, as it is an image deep down. Many times we put img { max-width:100%; height: auto; } in our samples and there was no problem with that.

You are either testing with small images or not using image2. Simple demo of how image2 breaks layout http://jsfiddle.net/5tm3L7ex/

And as for the old image plugin – there was a PR made, but it wasn't finished (comment:30). We'd be super thankful if someone took it over.

Since long ago I found much easier to create a plugin to handle all the issues instead of trying to modify the core and then create all the tests for you.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy