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

Inserting spaces inside the code element does not work #4169

Closed
pomek opened this issue Aug 31, 2017 · 8 comments
Closed

Inserting spaces inside the code element does not work #4169

pomek opened this issue Aug 31, 2017 · 8 comments
Labels
package:engine resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:bug This issue reports a buggy (incorrect) behavior.

Comments

@pomek
Copy link
Member

pomek commented Aug 31, 2017

All scenarios require the Code feature which will be introduced in ckeditor/ckeditor5-basic-styles#55.

The following scenarios shouldn't crash the editor:

1. Insert a space at the end of </code> element.

Make an editor with the following content:

<p><i>This</i> <code>is an[]</code> <strong>editor</strong> <u>instance</u>.</p>

Then insert a space. The editor data should looks like:

<p><i>This</i> <code>is an []</code> <strong>editor</strong> <u>instance</u>.</p>

Now we can notice the first problem – one of the space is not visible, two spaces are next to each other (in DOM).

image

After inserting another space, the editor will blow up. However, the editor still returns a proper HTML:

<p><i>This</i> <code>is an  []</code> <strong>editor</strong> <u>instance</u>.</p>

Editor crashes on the code related to mutations.

Uncaught TypeError: Cannot read property 'index' of undefined
    at MutationHandler._handleTextNodeInsertion (input.js:298)
    at MutationHandler.handle (input.js:152)
    at Input._handleMutations (input.js:105)
    at Document.listenTo (input.js:50)
    at Document.fire (emittermixin.js:281)
    at MutationObserver._onMutations (mutationobserver.js:248)

const change = getSingleTextNodeChange( mutation ); – this expression for the specific case is undefined. When I added if ( !changes ) { return; }, the error does not occur but another spaces are not inserted.

Temporary solution (thanks to @szymonkups) is replacing this line – https://github.com/ckeditor/ckeditor5-engine/blob/a424116abc84287d49343898d84ac471c4c21932/src/view/renderer.js#L472 with the following code:

let expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, { bind: true } ) );

if ( viewElement.name == 'code' ) {
    expectedDomChildren = expectedDomChildren.map( item => {
        if ( item.nodeType === global.window.Node.TEXT_NODE ) {                
            item.data = item.data.replace( / /g, '\u00A0' );
        }

        return item;
    } );
}

but it is an ugly hack.

I wrote the test which confirms that model and view have proper values – https://gist.github.com/pomek/912a7c9b391bb1442b0d825b13d2b17e.

It looks like something is invalid with the code element.

2. Insert space as a code in empty editor

  1. Clear the editor.
  2. Use code styles.
  3. Press space.

The data stored in the model are correct:

<p><code> []</code></p>

Rendered DOM also is ok but the editor does not look ok. You can compare the editor views' by inserting an empty bolded space instead of the code.

image

image

  1. After inserting another space:
  • Model:
<code> </code><p><code> </code></p>
  • Rendered DOM:
<code> </code><p><code> </code></p>
  1. Inserting another space will blow up the editor:
Uncaught TypeError: Cannot read property 'index' of undefined

After pasting temporary solution in engine/view/renderer~Renderer#_updateChildren(), the error will occur once and the spaces will be inserted.

@scofalik scofalik self-assigned this Aug 31, 2017
@scofalik
Copy link
Contributor

I did changes recently in handling multiple spaces so this may be related with the code I was workin with. I'll look at it. For sure we cannot apply a hack like suggested.

@Reinmar
Copy link
Member

Reinmar commented Aug 31, 2017

I saw recently, when styling the docs, that whitespaces at the boundaries of <code> are not preserved by the browser. So this may be just an issue with this. I had to use a styling like this to preserve them:

	code:not( .hljs ) {
		// It's a darker shade of u-color( 'background-hue' ) so it looks good in info-boxes.
		// See ckeditor/ckeditor5#3575.
		background: rgba( 202, 205, 207, 0.3 );
		padding: u-spacing( -4 ) u-spacing( -3 );
		border-radius: 3px;
	}

	// See ckeditor/ckeditor5#3571.
	code:not( .hljs ):after {
		// To reduce the width of that space to 0px.
		letter-spacing: -1em;
		content: "\00a0";
	}

	// See ckeditor/ckeditor5#3571.
	code:not( .hljs ):before {
		// To reduce the width of that space to 0px.
		letter-spacing: -1em;
		content: "\00a0";
	}

@Reinmar
Copy link
Member

Reinmar commented Aug 31, 2017

If that's the case, then the engine does not need to do anything. I mean – it won't be able to do anything more with this.

@pomek
Copy link
Member Author

pomek commented Aug 31, 2017

I modified the CSS a little:

code:not( .hljs ) {
	// It's a darker shade of u-color( 'background-hue' ) so it looks good in info-boxes.
	// See ckeditor/ckeditor5#3575.
	background: rgba( 202, 205, 207, 0.3 );
	padding: u-spacing( -4 ) u-spacing( -3 );
	border-radius: 3px;

	// See ckeditor/ckeditor5#3571.
	&:before,
	&:after {
		// To reduce the width of that space to 0px.
		letter-spacing: -1em;
		content: "\00a0";
	}

	// Fixes the 2nd scenario.
	& > br[data-cke-filler] {
		display: none;
	}
}

and now the editor does not blow up!

But, we cannot add more than one space before the </code> (but I guess it isn't a big problem – https://github.com/ckeditor/ckeditor5-basic-styles/issues/52#issuecomment-323987258).

@pomek
Copy link
Member Author

pomek commented Aug 31, 2017

I found another problem – you can type two spaces before closing the </code> tag but only one space will be rendered.

<p><i>This</i> <code>is an  []</code> <strong>editor</strong> <u>instance</u>.</p>

image

When you press Backspace, you will get:

<p><i>This</i> <code>is an []</code> <strong>editor</strong> <u>instance</u>.</p>

But the rendered view will not change.

image

If you deselect the editor, then focus once again at the end of the code, then press the Backspace, you will get:

<p><i>This</i> <code>is an[] </code> <strong>editor</strong> <u>instance</u>.</p>

As a summary: the editor will work but the spaces are incorrect rendered.

@Reinmar
Copy link
Member

Reinmar commented Sep 6, 2017

tl;dr here is that after introducing proper styles typing more than one space in a row anywhere in <code> doesn't work.

@Reinmar
Copy link
Member

Reinmar commented Sep 6, 2017

Additionally, an error is thrown in Safari when typing spaces at the end of <code>. Might be caused by the fact that those spaces are not correctly rendered.

@Reinmar
Copy link
Member

Reinmar commented Sep 6, 2017

We'll approach this from the other side: https://github.com/ckeditor/ckeditor5-engine/issues/1126

@Reinmar Reinmar closed this as completed Sep 6, 2017
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

4 participants