Fix character newlines in pre #13799

ellatrix · 2019-02-09T20:11:38Z

Description

Fixes #12109. Alternative of #13606. See #13606 (comment).

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

youknowriad · 2019-02-11T08:03:06Z

packages/block-library/src/preformatted/index.js

@@ -68,7 +68,7 @@ export const settings = {
 		return (
 			<RichText
 				tagName="pre"
-				value={ content }
+				value={ content.replace( /\n/g, '<br>' ) }


Does this mean we should do the opposite once onChange is called?
Are there other tags that needs to behave this way? Should this be a config of RichText?

No, we don't need modify onChange. There are two possibilities:   and \n. In RichText \n is collapsed as it is normally in HTML, so it cannot handle it. I went for using   by default, but we could go for \n too, in which case we would need to onChange. RichText always outputs   for line breaks.

Yes, it could also be part of the RichText config. Something like collapseLineBreaks: false. Not sure if that's needed as it will be more of an exception and it can be resolved outside of RichText, keeping RichText free of more parameters.

Yes, it could also be part of the RichText config. Something like collapseLineBreaks: false.

Does it need to be configured? Or is it enough to infer by tagName === 'pre' ?

Why does RichText collapse HTML anyways? From the comment in filterString, it reads as though it's not strictly necessary, but more to the point of normalizing on an assumption it'd be treated as equivalent by the browser anyways. If so, wouldn't this pull request and corresponding issue contradict that assumption?

gutenberg/packages/rich-text/src/create.js

Lines 235 to 239 in 910b655

function filterString( string ) {

// Reduce any whitespace used for HTML formatting to one space

// character, because it will also be displayed as such by the browser.

return string.replace( /[\n\r\t]+/g, ' ' );

}

Without removing line breaks, the line breaks will be visible in RichText, just like  . So if you add a line break in the HTML editor, then switch back, the line break will remain visible.

If we were to keep textual line breaks, they'd interfere with the rich text value line breaks:

In HTML, \n means nothing, it's just for the formatting of HTML,   means line break.

In a rich text value, \n means line break.

So if we just leave \n, they'll all be serialised as line breaks.

jorgefilipecosta

Works great in my tests and the approach is simple, I think we can use it 👍 I don't expect other blocks to need this kind of replacement so I think it is fine to leave this logic in the block.

Maybe we can add simple memoization to the replacement to avoid executing it on each block rerender even if nothing changed? A function wrapped around memize configured to one in the constructor would probably work great, but the replacement should also be a fast operation so in this case, memoization is not a must.

ellatrix · 2019-02-12T10:04:19Z

@aduth What do you think about the memoization? I'm not sure if a simple replace needs it, but maybe it's slightly faster and worth it.

aduth · 2019-02-15T14:08:59Z

@aduth What do you think about the memoization? I'm not sure if a simple replace needs it, but maybe it's slightly faster and worth it.

Are you asking if it should be introduced? I see none currently.

I'd probably avoid it, or limit it to a maxSize of 1 to just avoid repeatedly calling it when the value doesn't change.

But I also think ideally the value sits at rest in the format we want it to be in, so that we don't need to apply formatting when rendering the value. I see @youknowriad mentioned onChange earlier, but it's not clear by that conversation that we shouldn't just be replacing newlines from content there and not at all as part of the value assignment.

Similar to what we do for the PostTitle component:

gutenberg/packages/editor/src/components/post-title/index.js

Lines 58 to 61 in 858719b

    
           onChange( event ) { 
        
           	const newTitle = event.target.value.replace( REGEXP_NEWLINES, ' ' ); 
        
           	this.props.onUpdate( newTitle ); 
        
           }

ellatrix · 2019-02-15T14:21:09Z

We don't want to change anything in the RichText output. Only to the input. Ideally, the replacing happens on attribute parsing, but there's no way for the block to interfere with it?

And yes, @jorgefilipecosta was thinking it would be good to introduce it. I'm not sure. I'd avoid it as well.

ellatrix · 2019-02-20T13:12:23Z

Merging as it fixes the issue and adds a test. We can iterate on implementation if needed.

gziolo · 2019-02-21T15:32:21Z

The test added in this PR is very unstable on Travis. See #14014.

* Fix character newlines in pre * Add e2e test

EricForgy · 2019-03-26T15:39:20Z

Out of curiosity, I'm a little lost, but did this change break multiline LaTeX equations? Ref #14564

fransflippo · 2020-05-13T05:58:23Z

Would this be included in WordPress 5.4.1? I'm experiencing this exact issue with preformatted blocks in WordPress 5.4.1, but I don't know how to tell which Gutenberg version it includes.

fransflippo · 2020-05-13T06:03:25Z

Scratch that, it's not the exact same issue.

My issue is that adding a Gutenberg preformatted block with newlines cause those newlines to be converted into spaces when rendered to HTML.

Editor:

<pre class="wp-block-preformatted">Line 1
Line 2

Line 4 (previous line was empty)</pre>

HTML:

<pre class="wp-block-preformatted">Line 1 Line 2  Line 4 (previous line was empty)</pre>

Before I log a new issue, is anybody aware of this and can you reproduce it? Thanks.

fransflippo · 2020-05-16T06:00:53Z

If anybody runs into the same problem: newlines in the Tabbed Content block from the Ultimate Blocks plugin will be replaced by whitespace. So don't put preformatted or code blocks inside Tabbed Content blocks. (I'm using Tabby now for tabs which just doesn't have Gutenberg support atm, but the shortcodes work fine and it doesn't screw up my code blocks.)

Fix character newlines in pre

b403327

ellatrix requested review from ajitbohra, gziolo, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners February 9, 2019 20:11

Add e2e test

ce32924

ellatrix requested review from aduth, nerrad and ntwb as code owners February 10, 2019 22:44

gziolo added this to the 5.1 (Gutenberg) milestone Feb 11, 2019

gziolo added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended labels Feb 11, 2019

youknowriad reviewed Feb 11, 2019

View reviewed changes

jorgefilipecosta approved these changes Feb 11, 2019

View reviewed changes

jorgefilipecosta mentioned this pull request Feb 11, 2019

Fix: Converting a <pre> to blocks drops new lines #13606

Closed

noisysocks modified the milestones: 5.1 (Gutenberg), 5.2 (Gutenberg) Feb 18, 2019

ellatrix merged commit 2a37333 into master Feb 20, 2019

ellatrix deleted the fix/pre-newlines branch February 20, 2019 13:12

gziolo mentioned this pull request Feb 21, 2019

Failing test verifying if character newlines are preserved for Preformatted block #14014

Closed

ellatrix mentioned this pull request Mar 2, 2019

Preformatted block does not display preformatted text. #14188

Closed

ellatrix mentioned this pull request Mar 4, 2019

Bump plugin version to 5.2.0-rc.1 #14210

Merged

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Fix character newlines in pre (#13799)

94fc578

* Fix character newlines in pre * Add e2e test

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Fix character newlines in pre (#13799)

dea44b5

* Fix character newlines in pre * Add e2e test

aduth mentioned this pull request Mar 26, 2019

Line breaks break LaTeX #14564

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix character newlines in pre #13799

Fix character newlines in pre #13799

Uh oh!

ellatrix commented Feb 9, 2019 •

edited

Loading

Uh oh!

youknowriad Feb 11, 2019

Uh oh!

ellatrix Feb 11, 2019

Uh oh!

aduth Feb 18, 2019

Uh oh!

aduth Feb 18, 2019

Uh oh!

ellatrix Feb 19, 2019

Uh oh!

jorgefilipecosta left a comment

Uh oh!

ellatrix commented Feb 12, 2019

Uh oh!

aduth commented Feb 15, 2019

Uh oh!

ellatrix commented Feb 15, 2019

Uh oh!

ellatrix commented Feb 20, 2019

Uh oh!

gziolo commented Feb 21, 2019

Uh oh!

EricForgy commented Mar 26, 2019

Uh oh!

fransflippo commented May 13, 2020

Uh oh!

fransflippo commented May 13, 2020 •

edited

Loading

Uh oh!

fransflippo commented May 16, 2020

Uh oh!

Uh oh!

	function filterString( string ) {
	// Reduce any whitespace used for HTML formatting to one space
	// character, because it will also be displayed as such by the browser.
	return string.replace( /[\n\r\t]+/g, ' ' );
	}

Fix character newlines in pre #13799

Fix character newlines in pre #13799

Uh oh!

Conversation

ellatrix commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

youknowriad Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

ellatrix Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

aduth Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

aduth Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

ellatrix Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Feb 12, 2019

Uh oh!

aduth commented Feb 15, 2019

Uh oh!

ellatrix commented Feb 15, 2019

Uh oh!

ellatrix commented Feb 20, 2019

Uh oh!

gziolo commented Feb 21, 2019

Uh oh!

EricForgy commented Mar 26, 2019

Uh oh!

fransflippo commented May 13, 2020

Uh oh!

fransflippo commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fransflippo commented May 16, 2020

Uh oh!

Uh oh!

ellatrix commented Feb 9, 2019 •

edited

Loading

fransflippo commented May 13, 2020 •

edited

Loading