Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hello, when using the ckeditor5, I want to add <P><br/></p> after inserting the picture format. I don't know how to do that. #1255

Closed
gongzhiq opened this issue Sep 21, 2018 · 22 comments
Labels
pending:feedback This issue is blocked by necessary feedback. resolution:expired This issue was closed due to lack of feedback. type:question This issue asks a question (how to...).

Comments

@gongzhiq
Copy link

gongzhiq commented Sep 21, 2018

Hello, when using the ckeditor5, I want to add <P><br/></p> after inserting the picture format. I don't know how to do that.

Now, insert the image in the format:

<figure class="image ck-widget">
<img src="">
<figcaption class="ck-editor__editable ck-editor__nested-editable">dsfa</figcaption>
</figure>

The format I want is:

<figure class="image ck-widget">
<img src="">
<figcaption class="ck-editor__editable ck-editor__nested-editable">dsfa</figcaption>
</figure>
<P><br/></p>

How to insert <P><br/></p> after figure

Like this

<image1></image1>
<p><br/></p>
<image2></image2>
<p><br/></p>
<image3></image3>
<p><br/></p>
@ma2ciek ma2ciek self-assigned this Sep 21, 2018
@ma2ciek
Copy link
Contributor

ma2ciek commented Sep 21, 2018

Hi, @gongzhiq!

I've answered this question on SO - https://stackoverflow.com/a/52442482/2043175 - is that yours?

Basically, I was able to listen to the model changes, iterate over last changes and insert an empty paragraph before every image.

const model = editor.model;
const doc = editor.model.document;

doc.on( 'change:data', () => {
    const changes = doc.differ.getChanges();

    model.change( writer => {
        for ( const entry of changes ) {
            if ( entry.type == 'insert' && entry.name == 'image' ) {
                const image = entry.position.nodeAfter;
                const paragraph = writer.createElement( 'paragraph' );

                writer.insert( paragraph, image, 'before' );
            }
        }
    } );
} );

As I mentioned on the SO, the current implementation inserts a paragraph before each image. writer.insert( paragraph, image, 'after' ) inserts all paragraph after the last image - I'm not sure why it happens. Maybe you will now, @scofalik, @Reinmar?

@ma2ciek ma2ciek added status:pending type:question This issue asks a question (how to...). labels Sep 21, 2018
@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2018

You mean that this:

writer.insert( paragraph, image1, 'after' );

/*
<image1></image1>
<image2></image2>
<image3></image3>
*/

Inserts the paragraph after <image3>?

@ma2ciek
Copy link
Contributor

ma2ciek commented Sep 21, 2018

Oh, now I see. Images are inserted one by one. So this is what happens:

  1. The first image is inserted
  2. The paragraph is created after the image
  3. The second image is inserted in place of the paragraph, so the paragraph moves down.
  4. The paragraph is inserted after the image

And so on...

I was surprised that iterating over changes in the other direction doesn't solve the problem and now I know why.

@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2018

You mean – when you drop multiple images at once? Yep, they are inserted one by one. BTW, I think the above code should use a post-fixer. I didn't notice you listen on change:data. The mechanism of postfixers was created when we realised that doing changes as a result of changes is tricky.

@ma2ciek
Copy link
Contributor

ma2ciek commented Sep 21, 2018

This is still tricky and the result is the same as using change:data event. As there is one image in the differ per one post fixer call. So probably it would be better to not use differ in this case and iterate over all root children and add a paragraph between images if they are one after another.

const model = editor.model;
const doc = editor.model.document;
const root = doc.getRoot();

doc.registerPostFixer( writer => {
	// Iterate from the end ato preserve correct positions.
	for ( let i = root.childCount -1 ; i--; i >= 1 ) {
		if ( root.getChild( i ).name === 'image' && root.getChild( i - 1 ).name === 'image' ) {
			const paragraph = writer.createElement( 'paragraph' );

			writer.insert( paragraph, root.getChild( i - 1 ), 'after' );
		}
	}
} );

@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2018

Aaa!!! There's a bug in ImageUpload. It uses separate model.change() blocks for each image. Fix that and we're home. I think this was even reported/commented somewhere in the past.

cc @pjasiun

@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2018

I cced you, @pjasiun, because I think there was some issue with that piece of code in the past:

https://github.com/ckeditor/ckeditor5-image/blob/565ee70bec8095cef75a91c6c5bb7095f2a9d9a1/src/imageupload/imageuploadediting.js#L63-L86

Do you remember whether we used multiple change blocks on purpose?

@jodator
Copy link
Contributor

jodator commented Sep 21, 2018

Aaa!!! There's a bug in ImageUpload. It uses separate model.change() blocks for each image. Fix that and we're home. I think this was even reported/commented somewhere in the past.

It also duplicates the ImageUploadCommand's code AFAICS. The ImageUploadCommand will be able to upload multiple files at once after ckeditor/ckeditor5-image#228 so we can unify editing and command parts (dedup the code) there also.

@pjasiun
Copy link

pjasiun commented Sep 21, 2018

For sure using multiple change blocks is incorrect because it will render after each change what might not be expected. However, I do not remember any specific bug related to this case. I also do not know any reason to implement it this way.

Anyway, as @jodator mentioned we will change it soon, so the upload command will be able to handle multiple images what means that the whole action will be executed in a single change block.

@ma2ciek
Copy link
Contributor

ma2ciek commented Sep 21, 2018

Anyway, as @jodator mentioned we will change it soon, so the upload command will be able to handle multiple images what means that the whole action will be executed in a single change block.

Great to hear it. So once the PR is merged I'll update the snippet for this question.

@jodator
Copy link
Contributor

jodator commented Sep 21, 2018

@pjasiun no. The ImageUploadCommand works this way. The ImageUploadEditing has loop with inner (thus multpile) change blocks and it's code duplicates the code from the command (kinda) on clipboard input.

@ma2ciek
Copy link
Contributor

ma2ciek commented Sep 21, 2018

@gongzhiq, note that to create an empty paragraph manually after the image you can use Enter when the image is selected. Similarly to create a paragraph before the image (if it's the first element in document) you can press Shift + Enter. This is a temporary solution, you can read more in #407.

I'm not sure if you know it, that's something I might overlook.

@pjasiun
Copy link

pjasiun commented Sep 21, 2018

@pjasiun no. The ImageUploadCommand works this way. The ImageUploadEditing has loop with inner (thus multpile) change blocks and it's code duplicates the code from the command (kinda) on clipboard input.

So I believe it should be changed.

@jodator
Copy link
Contributor

jodator commented Sep 21, 2018

So I believe it should be changed.

I also believe in it. More so I think it must be changed :)

@gongzhiq
Copy link
Author

Thank you. Many good suggestions have been given.

@gongzhiq
Copy link
Author

gongzhiq commented Oct 8, 2018

@

You mean that this:

writer.insert( paragraph, image1, 'after' );

/*
<image1></image1>
<image2></image2>
<image3></image3>
*/

Inserts the paragraph after <image3>?

Inserts the paragraph after<image1>``<image2>``<image3>

@gongzhiq
Copy link
Author

gongzhiq commented Oct 8, 2018

Like this

/*
<image1></image1>
<p><br/></p>
<image2></image2>
<p><br/></p>
<image3></image3>
<p><br/></p>
*/

How to do ?

@ma2ciek @Reinmar @jodator @pjasiun

@gongzhiq
Copy link
Author

@gongzhiq
Copy link
Author

gongzhiq commented May 9, 2020

modify imageinsertcommand.js

/**
	 * Executes the command.
	 *
	 * @fires execute
	 * @param {Object} options Options for the executed command.
	 * @param {String|Array.<String>} options.source The image source or an array of image sources to insert.
	 */
	execute( options ) {
		const model = this.editor.model;
		var previousNodeName = "";
		var nextNodeName = "";
		
		if (model.document.selection !== null) {
			// 判断前一个元素是否是附件(media, myWidget, image)				
			if (model.document.selection.getFirstPosition().parent.previousSibling !== null) {
				previousNodeName = model.document.selection.getFirstPosition().parent.previousSibling.name;	
			}
			
			// 判断后一个元素是否是附件(media, myWidget, image)	
			if (model.document.selection.getFirstPosition().parent.nextSibling !== null) {
				nextNodeName = model.document.selection.getFirstPosition().parent.nextSibling.name;	
			}
		}
		
		var vContent = this.editor.getData();
		var AddEmptyParagraphForPrevious = "";
		var AddEmptyParagraphForNext = "";
		
		if (previousNodeName === "media" || previousNodeName === "myWidget" || previousNodeName === "image") {
			AddEmptyParagraphForPrevious = "<p class='att_empty_paragraph'><br/></p>";
		} else {
			if (previousNodeName.length === 0) {
				AddEmptyParagraphForPrevious += "<p><br/></p>";
			}
		}
		
		if (nextNodeName === "media" || nextNodeName === "myWidget" || nextNodeName === "image") {
			AddEmptyParagraphForNext = "<p class='att_empty_paragraph'><br/></p>";
		} else {
			if (nextNodeName.length === 0) {
				AddEmptyParagraphForNext += "<p><br/></p>";
			}
		}
		
		model.change( writer => {
			const sources = Array.isArray( options.source ) ? options.source : [ options.source ];

			for ( const src of sources ) {
				insertImage( writer, this.editor, { src }, AddEmptyParagraphForPrevious, AddEmptyParagraphForNext);
			}
		} );
	}

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past two years. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Jan 15, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending:feedback This issue is blocked by necessary feedback. resolution:expired This issue was closed due to lack of feedback. type:question This issue asks a question (how to...).
Projects
None yet
Development

No branches or pull requests

6 participants