Image dialog - ratio lock is quantised

[Gael]

The ratio lock is quantised, making the lock to unexpectedly unlock.

1. Insert a 4000x3000 px image in CKEditor.
2. Right click on the image and open the image dialiog.
3. Ensure the ratio lock is lock, then change its width to: 330. The height automatically change to 248.
4. Save, re-open the image dialog. The width / height are still 330x248, but the ratio lock is unlock even if the values respect the image ratio.

The calculated ratios are not equals:
originalRatio: 1333 (accurate)
thisRatio: 1330 (quantised)

I think the logic in switchLockRatio method is wrong. Instead of comparing the ratio, where one has been quantised to pixels, it would make more sense to re-calculate the height and check if the image height equals the calculated one.

The following code replacement fix this issue:

File: _source/plugins/image/dialogs/image.js
Lines: 132 to 144

var width = dialog.getValueOf( 'info', 'txtWidth' ),
	height = dialog.getValueOf( 'info', 'txtHeight' ),
	originalRatio = oImageOriginal.$.width / oImageOriginal.$.height;
dialog.lockRatio = false; // Default: unlock ratio

if (!width && !height) {
	dialog.lockRatio = true;
} else {
	if (!isNaN(originalRatio)) {
		var ratioHeight = Math.round(width / originalRatio);
		if (ratioHeight == height) {
			dialog.lockRatio = true;
		}
	}
}

[Jakub Ś]

Problem can be reproduced from CKEditor 3.0 in all browsers.

[Henk-Jan Sleijster]

This problem still exists in version 4.5.5 and is reproducible in the demo.

• ​http://ckeditor.com/demo
• Click image-icon
• Click "Browse server"
• Choose a non-square image, for example: the minions image
• Change the height to 50 pixels
• Click 'OK' and reopen the image dialog by double clicking the image.
• The lock is now unlocked. This is not good.

[Jakub Ś]

First of all don't change version number - it is used to indicate when problem has stared.

Second, thank you for the test case however it can be reproduced from CKEditor version 3 as well so nothing changes here.

Anyway, I will forward this issue to my colleagues.

[Jakub Ś]

#14290 was marked as duplicate.

[Gael]

BTW the patch I provided 3 years ago still fix the problem... I'm not sure why someone added "* 1000" the the ratio calculation. That looks like an unsuccessful attempt to fix the problem.

Should I explain what I'm doing in that patch or provide mathematics evidences as to why it works?

Basically, what I do is quite simple: To know if the lock should be closed or not, I calculate the "Height" that the image should have considering the specified "Width". If the "Height" match the specified "Height", then the ratio is good and the lock should be locked. This works all the time because I'm simply using the same Maths that has been used the calculate the "Height" in the first place. No magic here...

[Jakub Ś]

@gaellafond the issue was simply forgotten which happens when you have over a thousand of other feature requests, bugs reports (and not all of them are valid) and tasks reported. I will forward this ticket to my colleagues.

[Marek Lewandowski]

@gaellafond by the looks of it code looks OK. Can I ask you to send this code with a ​Pull Request, so that you are marked as a contributor of this code? Thanks!

[Gael]

Patched file (not minimised) - can be used as a direct replacement of "plugins/image/dialogs/image.js" for CKEditor version 4.5.6

[Gael]

@j.swiderski, @m.lewandowski Thanks. Sorry, I didn't realised that there is so many issues. I suppose that you have to focus on the one that has the highest activity.

I just submitted a new patched file, one that also take care of images in "Portrait" aspect ratio. I'm currently testing my patch to be sure it fixes the problem in every possible situations, and do not introduce new problems (like the lock closed when the image ratio are not respected).

I will add some comments in my patch and follow the procedure to submit a "Pull Request".

[Marek Lewandowski]

@gaellafond Cool, that sounds great! Thank you for the input so far!

[Gael]

I tried to write a Bender test. I manage to set an image in the dialog window, read the width x height and aspect ratio values.

Then I updated the width and trigger the "keyUp" event to update the height. Then I closed the dialog.

Next step, I need to bring back the image dialog and ensure that the ratio lock remain closed.

Any idea how to do this?

I did this so far, but that doesn't work. It just give me the last value that was in the dialog, not the updated value:

/**
 * #10051
 * Author: Gael Lafond
 * Date: 5th January 2016
 * See: https://dev.ckeditor.com/ticket/10051
 *
 * 1. Open image dialog.
 * 2. Set some proper image url.
 * 3. Set image dimensions, respecting the image ratio and focus out.
 * 4. Ratio lock should be closed.
 */
'test aspect ratio padlock stay closed when aspect ratio respected': function() {
	bender.editorBot.create( {
		name: 'editor_aspect_ratio',
		creator: 'inline'
	},
	function( bot ) {
		bot.dialog( 'image', function( dialog ) {
			dialog.setValueOf( 'info', 'txtUrl', imgs[ 0 ].url );
			downloadImage( imgs[ 0 ].url, onDownload );

			function onDownload() {
				resume( onResume );
			}

			function onResume() {
				var widthInput = dialog.getContentElement( 'info', 'txtWidth' ),
					heightInput = dialog.getContentElement( 'info', 'txtHeight' );

				// Check initial value
				// http://yuilibrary.com/yui/docs/api/classes/Test.Assert.html
				assert.areSame( '163', widthInput.getValue() );
				assert.areSame( '61', heightInput.getValue() );
				assert.isTrue( dialog.lockRatio );

				// Set the width
				widthInput.setValue(50);
				// Trigger the JavaScript event, which calculate the height
				widthInput.onKeyUp();
				// Close the dialog window
				dialog.getButton( 'ok' ).click();

				assert.areSame( '50', widthInput.getValue() );
				assert.areSame( '19', heightInput.getValue() );
				assert.isTrue( dialog.lockRatio );
			}

			wait();
		} );
	} );
},

NOTE: Without the patch, this particular image will show the ratio lock "open" instead of "closed" when the width is set to 50px and the height is proportionally set to 19px. With the patch, the ratio lock stay "closed" as expected.

Thanks