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

Code/Preformatted/Verse Block: br tag and unintended space character added #58659

Closed
t-hamano opened this issue Feb 5, 2024 · 23 comments · Fixed by #58792
Closed

Code/Preformatted/Verse Block: br tag and unintended space character added #58659

t-hamano opened this issue Feb 5, 2024 · 23 comments · Fixed by #58792
Labels
[Block] Code Affects the Code Block [Block] Preformatted Affects the Preformatted Block - used for showing preformatted text [Block] Verse Affects the Verse block [Package] Rich text /packages/rich-text [Type] Bug An existing feature does not function as intended

Comments

@t-hamano
Copy link
Contributor

t-hamano commented Feb 5, 2024

Description

I think the following issues are also related:

Code, Preformatted, and Verse blocks use the pre tag as wrapper elements. This tag is expected to display the text as is, so line breaks and spaces should be preserved.

However, line breaks are replaced with br tags and spaces are replaced with   entities, which may result in unintended display on the front end.

From what I've researched, I suspect that #43204, which changed the format of RichText values, may be involved.

I'm concerned that this issue may also result in breaking changes to existing plugins that extend code blocks, as reported in #56855.

Step-by-step reproduction instructions

About double tags

Insert a code block and insert the sample code below.

<?php
function custom_the_title( $title, $id = null ) {
    if ( in_category(' test', $id ) ) {
        return '';
    }
    return $title;
}
add_filter( 'the_title', 'custom_the_title', 10, 2 );

When you switch to the code editor, you will see that line breaks have been replaced with br elements, as shown below. At the same time, the PHP opening tag (<) is unintentionally replaced with &lt;.

image

On the front end, it is not displayed correctly due to this double br tag.

image

About incorrect spaces

Enter the text with spaces into the code editor. Another problem is that the space does not appear to appear unless you press the space bar several times, as shown in the screencast below.

cbb823dd441cc346d56c274820916fb3.mp4

In the front end, you should see spaces replaced with &emsp: as shown below.

image

Screenshots, screen recording, code snippet

No response

Environment info

  • Gutenberg version:17.6
  • This problem does not occur with WordPress 6.4.3.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended [Block] Code Affects the Code Block [Package] Rich text /packages/rich-text [Block] Verse Affects the Verse block [Block] Preformatted Affects the Preformatted Block - used for showing preformatted text labels Feb 5, 2024
@t-hamano t-hamano moved this to ❓ Triage in WordPress 6.5 Editor Tasks Feb 5, 2024
@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 5, 2024

This problem does not occur in WP6.4, it occurs for the first time in WP6.5. So I added it to the project board.

@dmsnell
Copy link
Member

dmsnell commented Feb 5, 2024

We should know the context node in which the RichText is found, shouldn't we? so we should be able to signify that newlines should not be replaced here, or that if they are, that we remove the corresponding \n

@t-hamano the &lt; seems like it should be fine. what's the problem there? While not required, that's a legitimate transformation, and unlike TEXTAREA, if we get something inside the PRE that looks like HTML, it will be treated as such, so the escaping on the < is reasonable safety measure against that.

@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 6, 2024

the < seems like it should be fine. what's the problem there?

It seems that this was a misunderstanding on my part. This behavior was also the same in WP6.4.3.

If so, I think the following three problems need to be solved, but is this possible?

  • Avoid replacing \n with double br tags
  • The space does not appear unless you press the space bar several times
  • Spaces are replaced with &emsp;

@dmsnell
Copy link
Member

dmsnell commented Feb 6, 2024

Avoid replacing \n with double br tags

Agreed here that we shouldn't have \n and a <br>, but where is the double br tags? I only see one per line in your example.

The space does not appear unless you press the space bar several times

I'm aware that this is a really inconsistent behavior between browsers, and something imposed on the contentEditable. Are we sure this is a new behavior and wasn't there before?

Spaces are replaced with  

This is a new one I haven't seen before, as they are usually replaced by alternating &nbsp;s and space characters. Can you confirm what browser you're using? Is that in Windows? (taking a guess from the screenshots).

@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 6, 2024

Avoid replacing \n with double br tags

Agreed here that we shouldn't have \n and a <br>, but where is the double br tags? I only see one per line in your example.

Oh, it wasn't a double br tag. Perhaps more accurately, one br element is added in addition to the original \n.

The space does not appear unless you press the space bar several times

I'm aware that this is a really inconsistent behavior between browsers, and something imposed on the contentEditable. Are we sure this is a new behavior and wasn't there before?

This is a behavior that did not exist before. You should be able to reproduce this problem by inserting a code block and pressing the space bar repeatedly.

Spaces are replaced with  

This is a new one I haven't seen before, as they are usually replaced by alternating &nbsp;s and space characters. Can you confirm what browser you're using? Is that in Windows? (taking a guess from the screenshots).

I have confirmed this on Windows OS and Chrome. I reproduce it in the screencast below. I tested it again and it seems that both U+2002 and U+2003 are included.

0de3959503366ef27829d72d621445d0.mp4

@dmsnell
Copy link
Member

dmsnell commented Feb 6, 2024

I have confirmed this on Windows OS and Chrome

Thanks. That looked like Cleartype in the picture… I didn't doubt you saw this behavior, by the way - I only wanted to confirm where it was coming from.

Oh, it wasn't a double br tag.

Awesome, I was hoping and expecting you mean that.

This is a behavior that did not exist before

That suggests to me it was #56855 and not #43204 which introduced this change.

@t-hamano t-hamano changed the title Code/Preformatted/Verse Block: Double br tag and incorrect spaces Code/Preformatted/Verse Block: br tag and unintended space character added Feb 7, 2024
@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 7, 2024

What do you think about temporarily reverting the format to the previous format as shown below to solve this problem? Or should I keep the current format and look for a more ideal solution?

diff --git a/packages/block-library/src/code/block.json b/packages/block-library/src/code/block.json
index 4465c8554f..433ad5d262 100644
--- a/packages/block-library/src/code/block.json
+++ b/packages/block-library/src/code/block.json
@@ -8,8 +8,8 @@
        "textdomain": "default",
        "attributes": {
                "content": {
-                       "type": "rich-text",
-                       "source": "rich-text",
+                       "type": "string",
+                       "source": "html",
                        "selector": "code",
                        "__unstablePreserveWhiteSpace": true
                }

@dmsnell
Copy link
Member

dmsnell commented Feb 7, 2024

What do you think about temporarily reverting the format

What I was trying to say is that this doesn't seem like a result of the format change, but maybe I'm wrong. There was that separate work to alter the whitespace behavior which I suspect is more the culprit. On that note my thoughts about a revert don't matter that much; @ellatrix should have the full context and insight here to advise.

@ellatrix
Copy link
Member

ellatrix commented Feb 7, 2024

Let's revert #56341, it anyway did not accomplish what I wanted.

I'll check about the line breaks.

@ellatrix
Copy link
Member

ellatrix commented Feb 7, 2024

@ellatrix
Copy link
Member

ellatrix commented Feb 7, 2024

  • The space does not appear unless you press the space bar several times
  • Spaces are replaced with &emsp;

The above should fix both these issues.

@ellatrix
Copy link
Member

ellatrix commented Feb 7, 2024

I don't understand the line break issue. How do I reproduce the double line break issue? This is what I'm seeing:

<!-- wp:code -->
<pre class="wp-block-code"><code>&lt;?php<br>function custom_the_title( $title, $id = null ) {<br>    if ( in_category(' test', $id ) ) {<br>        return '';<br>    }<br>    return $title;<br>}<br>add_filter( 'the_title', 'custom_the_title', 10, 2 );</code></pre>
<!-- /wp:code -->

@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 8, 2024

I don't understand the line break issue. How do I reproduce the double line break issue? This is what I'm seeing:

Inserts text containing line breaks (\n) into a Code block. This line breaks are not removed, but a br tag is added. So it looks like there are double line breaks on the front end.

fc3f0f7088a37b76a0398cbf1421a896.mp4

@t-hamano t-hamano moved this from ❓ Triage to 🏗️ In Progress in WordPress 6.5 Editor Tasks Feb 8, 2024
@dmsnell
Copy link
Member

dmsnell commented Feb 8, 2024

could this be an Edge/Windows-specific behavior?

@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 8, 2024

I don't have a Mac computer, so I'd appreciate it if you could test to see if this problem can be reproduced on MacOS.

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2024

Ok, let's try to debug on your end because I don't get the same result. What do you see when you run wp.data.select( 'core/block-editor' ).getBlocks()?

Screenshot 2024-02-08 at 09 12 02

@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 8, 2024

Ok, let's try to debug on your end because I don't get the same result. What do you see when you run wp.data.select( 'core/block-editor' ).getBlocks()?

  • Tested on trunk (commit hash: 5b11266)
  • Insert a Code block
  • Paste the text below into the code block
    one
    two
    three
    
  • Run wp.data.select( 'core/block-editor' ).getBlocks() in browser console

In my environment it looks like \r is added extra. Does this mean CR+LF?

Screencast:

84a77202d379fa49974c45a6057b4809.mp4

Full object data:

image

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2024

So that's what I suspected :) It's \r

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2024

Strange because we should be stripping \r as well.

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2024

Oh, I know what is happening. The preserveWhiteSpace flag makes RichText not strip anything. \n is then internally interpreted as a line break, but \r lingers. But on Windows line breaks are always these two characters together? So I guess even with preserveWhiteSpace we need to make sure these \r characters are stripped.

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2024

This would be one way to fix I think #58805

@ellatrix
Copy link
Member

ellatrix commented Feb 8, 2024

So with these two fixes I think all issues here are addressed?

@t-hamano
Copy link
Contributor Author

t-hamano commented Feb 8, 2024

But on Windows line breaks are always these two characters together?

By default it should be CR+LF(\r\n), but I noticed that \r is not included in the Firefox browser. Therefore, it might depend on the browser type and browser settings. For now, I would like to test #58805.

So with these two fixes I think all issues here are addressed?

Yes, I believe that once #58792 and #58805 are merged, all problems will be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Code Affects the Code Block [Block] Preformatted Affects the Preformatted Block - used for showing preformatted text [Block] Verse Affects the Verse block [Package] Rich text /packages/rich-text [Type] Bug An existing feature does not function as intended
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants