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.

Sign up for GitHub

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

Jump to bottom

Inserting a space before a block Element in Firefox will replace it with @@ #5651

Closed
zireael-0 opened this issue Oct 23, 2019 · 6 comments
Closed

Inserting a space before a block Element in Firefox will replace it with @@ #5651

zireael-0 opened this issue Oct 23, 2019 · 6 comments
Labels
pending:feedback This issue is blocked by necessary feedback. resolution:resolved This issue was already resolved (e.g. by another ticket). type:bug This issue reports a buggy (incorrect) behavior.

Comments

@zireael-0
Copy link

You need a view-model and DOM that has [Text, Element] anywhere with the elements HTMLElement conforming to display: block.

Moving your caret to the last position within your first Text and appending a space will delete the element and replace it with @@.

As far as I can tell, this issue comes from Firefox inserting a <br /> out of nowhere, breaking the CK InjectTypingMutationsHandling module. In it's handle method the containerChildrenMutated check will somewhat mistakenly succeed and will result in mutating the model incorrectly due to the unexpected new element.

You can observe the differing behavior between Chrome and Firefox with a simple html file and using the inspector of each browser. Placing the caret behind the t of the first text and hitting space will insert a new <br /> element.

<!DOCTYPE html>
<html>
<head></head>
<body>
<div style="border: 1px solid black" contenteditable="true">
text
<div>text in block</div>
</div>
</body>
</html>

I'm not sure if this is expected behavior from Firefox. Maybe it's worth considering to file an issue there, too.

If you'd like to see this fixed sooner, add a 👍 reaction to this post.
@zireael-0 zireael-0 added the type:bug This issue reports a buggy (incorrect) behavior. label Oct 23, 2019
@Mgsy
Copy link
Member

Mgsy commented Nov 4, 2019

Hi! Sorry for the late response. Can you share detailed steps on how to reproduce it in CKEditor 5 with recorded screencast?

@Mgsy Mgsy added the pending:feedback This issue is blocked by necessary feedback. label Nov 4, 2019
@zireael-0
Copy link
Author

Sure thing. I've set up an example editor build here.

If you clone the repository and run

npm install
npm run build

and open the generated out/index.html file in firefox, you should see an editor with a single button.

Try typing in some text and click the button when the caret is positioned after the text. It should insert a <div><img /></div> constellation, showing an example image from wikipedia.

If you move the caret to the end of the text and insert a space, the container is removed and replaced with @@. As far as I can tell this happens due to the <br /> element Firefox unexpectedly inserts and the containerChildrenMutated module not being able to map the DOM correctly anymore. Here is how it looks on my machine:

atat

However (in my example) you can observe a different behavior, when you change the schema in the blockElementPlugin I included. Setting the block element schema to isInline: false and hitting space at the same position will just do nothing instead.

In chrome both cases work just fine.

@Mgsy
Copy link
Member

Mgsy commented Nov 8, 2019

Thanks for a detailed description and live sample! I can confirm this behaviour.

@jodator, can you investigate it?

@jodator
Copy link
Contributor

jodator commented Nov 13, 2019

It's a bit similar to #5613 & #1049. Basically the elements that act as blocks should be blocks (like image) and they shouldn't be nested in other blocks.

If you change your schema to:

this.editor.model.schema.register( MODEL_ELEMENT_NAME, {
	allowIn: '$root',
	isObject: true,
	isBlock: true
} );

and you'd use model.insertContent() in your command:

class InsertBlockElementCommand extends Command {
	execute() {
		this.editor.model.change( writer => {
			const modelElement = writer.createElement( MODEL_ELEMENT_NAME );

			this.editor.model.insertContent( modelElement, this.editor.model.document.selection );
		} );
	}
}

You'd insert your <genericBlock> between paragraphs or you'd split existing ones. Generally the model.insertContent() is your friend here ;)

But this would cause other problems with positions mapping (you could place position inside <div> in the editing area and the whole thing would blow up.

Bonus: use widget in the editing pipeline (remember to include Widget plugin):

this.editor.conversion.for( 'editingDowncast' ).elementToElement( {
	model: MODEL_ELEMENT_NAME,
	view: ( element, writer ) => {
		const containerElement = writer.createContainerElement( 'div' );
		const imgElement = writer.createEmptyElement( 'img' );

		writer.insert( writer.createPositionAt( containerElement, 0 ), imgElement );
		writer.setAttribute( 'class', 'image-container', containerElement );
		writer.setAttribute( 'src', 'https://upload.wikimedia.org/wikipedia/en/a/a9/Example.jpg', imgElement );
		writer.setCustomProperty( 'genericBlockElement', true, containerElement );
		
		return toWidget( containerElement, writer ,{ hasSelectionHandle: true });
	}
} );

Peek 2019-11-13 15-22

@zireael-0
Copy link
Author

Thanks for the response. However, I am not sure if I am satisfied with it:

You're correct that this example yields the same behavior when you do not use nested block elements. However that will no longer be the case if you try to let the image float around the text using CSS.

As far as I can tell, moving it out of the paragraph will make the text float around the image, but it becomes impossible to align the image so that it is not at either the very top or very bottom of the paragraph.

Also it's kinda odd that my version works fine in Chromium-esque browsers - I actually though putting blocks between texts would be an okay thing to do.

@zireael-0
Copy link
Author

Closing this since it's very old and likely doesn't need a fix (anymore)

@zireael-0 zireael-0 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 5, 2023
@Witoso Witoso added the resolution:resolved This issue was already resolved (e.g. by another ticket). label Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
pending:feedback This issue is blocked by necessary feedback. resolution:resolved This issue was already resolved (e.g. by another ticket). type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jodator @Witoso @zireael-0 @Mgsy