t/225: The ImageUploadCommand should check whether it can be executed in the selection context.

[oleq]

Suggested merge commit message (convention)

Fix: The ImageUploadCommand should check whether it can be executed in the selection context. Closes ckeditor/ckeditor5#5158. Closes ckeditor/ckeditor5#5160. Closes ckeditor/ckeditor5#5163.

BREAKING CHANGE: The options.file property was renamed to options.files for 'imageUpload' command.
BREAKING CHANGE: The options.insertAt property for 'imageUpload' command was removed. The command will use model's selection.

[jodator]

I'm not 100% sure why there's parent swapping before checking. Maybe some method would be better?

[jodator]

I'm missing a comment why this check and parent swapping.

[jodator]

ps.: Also this doesn't look consistent as it might swap to:

• blockQuote (from p, h1, l1, etc inside blockquote)
• tableRow (from tableCell) - I'm not sure what should be there (table/tableCell - oncoming block content in table cell)
• $root (from p, h1, l1, etc)

Maybe it should lookup for allowed parent or something other in the schema?

[jodator]

ps.: A comment would be sufficient also :)

[oleq]

TBH, I copy&pasted it from the InsertTableCommand implementation without any deeper analysis. The upload command never splits the block, it inserts the new image next to the one the selection is anchored to. So I guess the if is correct here because it comes down to

get the first ancestor of the current block, unless it's the root

[oleq]

BTW, have you seen https://github.com/ckeditor/ckeditor5-image/issues/225#issuecomment-407021363? Do you have any idea how to include a fix for that without breaking the multi-upload?

[jodator]

I'll check that and let you know :)

[jodator]

@oleq Sorry for longer reply. So I've manage to make #227 work by adding multi-file upload support to the FileUploadCommand itself:

// Extend current behavior of uploading one file
const filesToUpload = Array.isArray( options ) ? options : [ options ];

for ( const { file, insertAt } of filesToUpload ) {
	uploadImage( editor, fileRepository, file, insertAt, doc );
}

Where uploadImage() is the method that previously uploaded a file (contents of editor.model.change block.

This way the command can be disabled when an image is selected but it will allow UI to pass multiple files to upload at once - not as it is currently where each file is processed individually by a command. The problem is that after execute() the first image is selected and the command itself was disabled so next files were discarded as the command goes disabled then.

I have no other idea for solving this as probably the expected behavior is to select newly inserted image.

[jodator]

ps.: the ImageUploadUI was modified to:

view.on( 'done', ( evt, files ) => {
	const toUpload = [];

	for ( const file of Array.from( files ) ) {
		const insertAt = findOptimalInsertionPosition( editor.model.document.selection );

		if ( isImageType( file ) ) {
			toUpload.push( { file, insertAt } );
		}
	}

	editor.execute( 'imageUpload', toUpload );
} );

but those two snippets must be checked if there are OK as the files are inserted in revers order (AFAICS).

[coveralls]

Coverage remained the same at 100.0% when pulling 678d6ac on t/225 into 7ce0181 on master.

[jodator]

@pjasiun please review this changes. The goal was to disable the image button in table (works) and also not break multiple images upload. What I'm not sure about is the naming of the ImageUploadCommand param name options.file - I've used file instead of files to be backward-compatible but I think that we should name that files (like in conversion we have classes, styles, etc) and allow either single File or Array.<File>.

I've been assigned to the task.

[pjasiun]

So:

• I am fine with multiple files support in the command,
• I think we can rename file property to files (next release will be breaking anyway, and this is more-less internal API, I do not believe many people use it),
• I do not like parent != parent.root comparison. I have no idea how it works and if it will work in all cases. Instead, I think we should check if this element or any ancestor is a widget (isObject). This way you should solve https://github.com/ckeditor/ckeditor5-image/issues/227 too.

[jodator]

• I do not like parent != parent.root comparison. I have no idea how it works and if it will work in all cases. Instead, I think we should check if this element or any ancestor is a widget (isObject). This way you should solve #227 too.

I'm not sure if I follow this correctly but:

• isObject does note implies widget
• you might be interested to put image in widget (and we will be in a moment as we resolve image in image)

I think that we should check two things:

1. If image is allowed in current selection
2. If selection is not set over an image

[jodator]

@pjasiun: I've updated the PR with your comments and also I've been able to address issues from #227 & #235.

[pjasiun]

I think that we should check two things:

1. If image is allowed in current selection
2. If selection is not set over an image

I am not sure about "2". I think it should be more generic and consider all objects. I see no difference between uploading an image when image or media is selected. One idea is that if the selection is over an object we check in the schema if this object can contain the image. On the other hand, we can assume that an object can not be a parent of the image (it does not seem to make much sense) and disable image upload whenever an object is selected.

[jodator]

One idea is that if the selection is over an object we check in the schema if this object can contain the image. On the other hand, we can assume that an object can not be a parent of the image (it does not seem to make much sense) and disable image upload whenever an object is selected.

I think that the findOptimalInsertionPosition() which command would use for uploading an image doesn't work this way. For instance - when image is selected the uploaded image would go after (before?) it. Thus the explicit check for selected element & check for parent (image+caption case).

[pjasiun]

I think it should be consistent: if you are able to insert image when selection is like this, then the command/button should be available. If the button is disabled it should mean that you are not able to execute the command with such a position (it does nothing). Both ways are fine for me, but I think we should not disable button on the selection which works for executing the command.

[jodator]

@oleq & @pjasiun: could we agree on something?
Should the

1. command (thus button) be disabled when selection is on image (#227)?
2. command (thus button) be disabled when selection is inside image? (yes)
3. should we extend above behavior (1&2) to all object not only image?

[Reinmar]

One "tiny" problem – the command accepts options.insertAt. So when it's enabled in one place you can still pass to it a position where it should not be executable (and would normally be disabled).

That's a big problem that I think I discussed with @oskarwrobel in the past. Honestly, I don't know how to fix this situation. We've been discussing that options.insertAt should be removed and it all should base on the document selection. The longer I think about it the more I think we should do that. But that leads to issues such as – the command needs to use findOptimalInsertionPosition() itself, internally. While that may not sound serious – it goes further – the ImageUploadEditing feature does not use the document selection but the drop position from the clipboardInput. So, the upload feature will need to:

1. move the document selection to the drop position
2. and only then execute the command... but what if it's disabled in this place?

[Reinmar]

I can see you partly agree with that in https://github.com/ckeditor/ckeditor5-image/issues/235. So it's a matter of cleaning up ImageUploadEditing too and completely removing the param after that :D

[pjasiun]

command (thus button) be disabled when selection is on image (#227)?
command (thus button) be disabled when selection is inside image? (yes)

Yes and yes.

should we extend above behavior (1&2) to all object not only image?

I would go with all objects.

[jodator]

@pjasiun Ready to re-review after latest comments.

[pjasiun]

Comment need to be updated.

[pjasiun]

You can close the ticket as soon as you update comments mentioned above.