Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

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

Merged
merged 10 commits into from
Sep 28, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
594b12c
Fix: The ImageUploadCommand should check whether it can be executed i…
oleq Jul 23, 2018
949259c
Merge branch 'master' into t/225
jodator Sep 18, 2018
be5e3a4
Merge branch 'master' into t/225
jodator Sep 18, 2018
12ca8a0
Changed: Support multiple file uploads in ImageUploadCommand.
jodator Sep 19, 2018
0b6be9e
Merge branch 'master' into t/225
jodator Sep 19, 2018
4b3e1cf
Rename options.file to option.files in ImageUploadCommand.
jodator Sep 20, 2018
5ba392d
Changed: The ImageUploadCommand not the ImageUploadUI should calculat…
jodator Sep 21, 2018
b5940b5
Removed the insertAt option from `ImageUploadCommand`. The `ImageUplo…
jodator Sep 25, 2018
f22029d
Disable image upload command when selection is on Object or inside ot…
jodator Sep 25, 2018
678d6ac
Docs: Update private method comments.
jodator Sep 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/imageupload/imageuploadcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,23 @@ import Command from '@ckeditor/ckeditor5-core/src/command';
* @extends module:core/command~Command
*/
export default class ImageUploadCommand extends Command {
/**
* @inheritDoc
*/
refresh() {
const model = this.editor.model;
const selection = model.document.selection;
const schema = model.schema;
const position = selection.getFirstPosition();
let parent = position.parent;

if ( parent != parent.root ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ps.: A comment would be sufficient also :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check that and let you know :)

Copy link
Contributor

@jodator jodator Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

parent = parent.parent;
}

this.isEnabled = schema.checkChild( parent, 'image' );
}

/**
* Executes the command.
*
Expand Down
33 changes: 33 additions & 0 deletions tests/imageupload/imageuploadcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,39 @@ describe( 'ImageUploadCommand', () => {
return editor.destroy();
} );

describe( 'isEnabled', () => {
it( 'should be true when the selection directly in the root', () => {
setModelData( model, '[]' );
expect( command.isEnabled ).to.be.true;
} );

it( 'should be true when the selection directly in a paragraph', () => {
setModelData( model, '<paragraph>foo[]</paragraph>' );
expect( command.isEnabled ).to.be.true;
} );

it( 'should be true when the selection directly in a block', () => {
model.schema.register( 'block', { inheritAllFrom: '$block' } );
model.schema.extend( '$text', { allowIn: 'block' } );
editor.conversion.for( 'downcast' ).add( downcastElementToElement( { model: 'block', view: 'block' } ) );

setModelData( model, '<block>foo[]</block>' );
expect( command.isEnabled ).to.be.true;
} );

it( 'should be false when the selection in a limit element', () => {
model.schema.register( 'block', { inheritAllFrom: '$block' } );
model.schema.register( 'limit', { allowIn: 'block', isLimit: true } );
model.schema.extend( '$text', { allowIn: 'limit' } );

editor.conversion.for( 'downcast' ).add( downcastElementToElement( { model: 'block', view: 'block' } ) );
editor.conversion.for( 'downcast' ).add( downcastElementToElement( { model: 'limit', view: 'limit' } ) );

setModelData( model, '<block><limit>foo[]</limit></block>' );
expect( command.isEnabled ).to.be.false;
} );
} );

describe( 'execute()', () => {
it( 'should insert image at selection position (includes deleting selected content)', () => {
const file = createNativeFileMock();
Expand Down