Rich text: remove preserveWhiteSpace serialisation differences #55999
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
added
[Type] Performance
ellatrix
commented
Nov 9, 2023
| let valueB = create( { | ||
| html: htmlB, | ||
| ...mapRichTextSettings( attributeDefinitionB ), | ||
| } ); |
There was a problem hiding this comment.
Like I said, a pain to work with.
ellatrix
commented
Nov 9, 2023
| * @param {string} string | ||
| */ | ||
| function collapseWhiteSpace( string ) { | ||
| return string.replace( /[\n\r\t]+/g, ' ' ); |
There was a problem hiding this comment.
This is sort of replaced by logic in the paste handler
|
Size Change: -229 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
remove/rich-text-preserve-whitespace
branch
from
2cd0b0f
to
e11bc4c
Compare
ellatrix
requested review from
fluiddot,
dcalhoun,
kevin940726,
Mamaduka,
alexstine and
ajitbohra
as code owners
dmsnell
approved these changes
Nov 9, 2023
There was a problem hiding this comment.
I've been testing this with the Playground and can't find a way to break it.
I explored variations of whitepspace and conversion to and from a paragraph, code block, HTML block.
The only thing that stood out to me was that my spaces were preserved inside the editor for the paragraph but obviously didn't remain in the rendered post. I would have expected successive spaces in the editor (for example, typing this alignment) would produce HTML that preserves those spaces, such as with a non-breaking space. Since this happens in trunk it's not a blocker or even a comment about this change specifically.
While reviewing I did wonder what our reason has been for removing whitespace at all. If we don't preserve the intent or entered text from the editor to the rendered page, why modify it at all? Exploring the DOM inside the browser revealed that the browser doesn't modify textContent to remove extra whitespace, and I couldn't find any way to make it do it, even with calls to .normalize() and .cloneNode(). In other words, if browsers handle the whitespace and the editor has is own rules, what are we trying to accomplish by messing with whitespace?
|
That's because we have white space: pre-wrap turned on for all rich text instances. I'll try to remove it in 587dede. |
|
Flaky tests detected in 2ef1e3a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6826391569
|
ellatrix
force-pushed
the
remove/rich-text-preserve-whitespace
branch
from
587dede
to
0c26493
Compare
ellatrix
changed the title
ellatrix
changed the title
|
FYI: This change apparently broke my Syntax-highlighting Code Block plugin, since the I have a workaround to replace all |
|
@westonruter Worth nothing that the RichText and the code block can contain any HTML formatting. If you create a custom block and don't want formatting, you should ideally use the PlainText component (v2). |
|
@ellatrix Thanks. Good point. However, I'm hoping to retain the ability to do HTML formatting and have syntax highlighting (westonruter/syntax-highlighting-code-block#205), likely facilitated with the HTML Tag Processor. If I fail at that, I'll go with what you suggest. |
@westonruter are you looking to render the contents of the PRE element verbatim as code? if so, I'm not sure that's a reliable method anyway. the PRE element doesn't add any semantic to its contents other than that they should be monospace-rendered. have you looked at storing the contents in a disabled TEXTAREA? or have you tried first decoding the contents on the server? if we interpret the PRE contents appropriately I wonder if the problem would persist? that's now doable with the HTML API in 6.5. does the syntax highlighter have the same struggles with encoded characters? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR adjusts the
preserveWhiteSpaceprop from rich text, which was being used for the Preformatted, Code, and Verse blocks. We don't have to continuously collapse whitespace inside rich text on value changes, and we don't need it for serialisation. Additionally, it's no longer needed for splitting/merging all instances serialise the same way.Why?
multiline, which has recently been removed) is a pain to work with. Currently, to parse and serialised rich text content (withcreateandtoHTMLString), you need to know where this HTML came from (an instance that preserves white space or not). This required us to add a special setting to the block attribute definition, so we can properly handle merging and splitting. Now all rich text values serialise the same way, with line breaks always serialising to the HTML BR tag.How?
As said before, we can rely on the initial HTML parse and the paste handler to remove superfluous white space. The logic in the paste handler was introduces in #17470, much after we added this
preserveWhiteSpaceprop to rich text.Additionally, for line break serialisation, it should be safe to use proper line break elements for rich text instances. It's worth noting that Preformatted, Code, and Verse use rich text components, allowing HTML formatting like bold, links, etc., so line break HTML elements are not odd. If we don't want any HTML elements, we should use the plain text component instead, which outputs line breaks as simple text line breaks.
Testing Instructions
We have lots of e2e test coverage, so we should be good. Mainly check the Preformatted, Code, and Verse blocks.
Testing Instructions for Keyboard
Screenshots or screencast