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

#5901: Use text nodes content directly instead of stringifying using data processor in code-block #7240

Merged
merged 1 commit into from
May 21, 2020

Conversation

tomalec
Copy link
Contributor

@tomalec tomalec commented May 19, 2020

as suggested by @oleq at #5901 (comment).
Ignore HTML elements inside <pre><code>.
Closes: #5901.

Suggested merge commit message (convention)

Fix (code-block): Stop rendering &nbsp; when restoring the state of an empty code block. Closes #5901.


Additional information

There is still an issue with dom to view conversion
https://github.com/ckeditor/ckeditor5/compare/i/5901-code-blocks?expand=1#diff-82a5336a00edfc515fb87b52fc38eafbR892-R901
as you can see, filler element is not cleared on getData

From what I have found, domConverter https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-engine/src/view/domconverter.js#L1027 uses getDataWithoutFiller which strips only INLINE_FILLER. I tried to change _processDataFromDomText to strip the block filler. However, it does not identify &nbsp; inside <code> as block filler, because <code> is an inline element.

I could do more changes, to let it change it anyway, given it already checked it's inside <pre> element which accepts only Phrasing content. But I have a doubt regarding that:

From what I understand domConverter is not aware of code-block feature. Therefore, for it <code> is as inline as any other element like <strong>, even inside <pre>. Shouldn't we use \u200b (zero-width space) in the first place? The pre-formatted behavior of <pre> guarantees the height for us?

//cc @Reinmar

…ocessor,

as suggested by @oleq at #5901 (comment).
Ignore HTML elements inside `<pre><code>`.
Closes: #5901.
@oleq
Copy link
Member

oleq commented May 20, 2020

The problem comes down to &nbsp; being rendered in the code element in editing when the data with &nbsp; is set:

editor.setData( '<pre><code>&nbsp;</code></pre>' );

and the raw editing view looks as follows (and you can move the caret before/after the nbsp):

<pre data-language="Plain text" spellcheck="false"><code class="language-plaintext">&nbsp;</code></pre>

but we'd rather expect the same behavior as with the paragraph:

editor.setData( '<p>&nbsp;</p>' );

when the raw editing view is:

<p><br data-cke-filler="true"></p>

Honestly... after a F2F call with @tomalec I came to the conclusion that we should follow through because from the end-user standpoint the original issue is serious and we can discuss this extra space quirk in a follow-up. IMO it's not a big deal and it could end up in a bigger refactoring around the DomCoverter class.

@tomalec tomalec marked this pull request as ready for review May 20, 2020 16:16
@Reinmar Reinmar merged commit ad22791 into master May 21, 2020
@Reinmar Reinmar deleted the i/5901-code-blocks branch May 21, 2020 10:04
@tomalec
Copy link
Contributor Author

tomalec commented May 21, 2020

Follow-up issue created #7259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code Block displays HTML entities
3 participants