Skip to content

RichText: convert HTML formatting whitespace to spaces #12166

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.

Sign up for GitHub

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

Merged
youknowriad merged 4 commits into from
Nov 30, 2018
Merged

RichText: convert HTML formatting whitespace to spaces #12166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
553c3b1
convert HTML formatting whitespace to spaces
ellatrix Nov 21, 2018
0431aba
Adjust and add e2e tests
ellatrix Nov 21, 2018
4937a21
Visualise consecutive spaces
ellatrix Nov 26, 2018
aacc270
Update e2e tests
ellatrix Nov 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/editor/src/components/rich-text/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@
margin: 0;
position: relative;
line-height: $editor-line-height;
// In HTML, leading and trailing spaces are not visible, and multiple spaces
// elsewhere are visually reduced to one space. This rule prevents spaces
// from collapsing so all space is visible in the editor and can be removed.
// It also prevents some browsers from inserting non-breaking spaces at the
// end of a line to prevent the space from visually disappearing. Sometimes
// these non breaking spaces can linger in the editor causing unwanted non
// breaking spaces in between words. If also prevent Firefox from inserting
// a trailing `br` node to visualise any trailing space, causing the element
// to be saved.
white-space: pre-wrap;
Copy link
Member

@oandregal oandregal Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-wrap will preserve newlines, spaces, and tabs. But in filterStringComplete we're only replacing newlines and tabs with spaces. As a consequence of this change, the editor will now show those consecutive spaces.

Before this change:

peek 2018-11-27 19-21

After this change:

peek 2018-11-27 19-29

Is that a concern?

Copy link
Member

@oandregal oandregal Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness, here's the same action in visual mode. No visual change, although under the hood the HTML doesn't add nbsp.

Before this change:

peek 2018-11-27 19-23

After this change:

peek 2018-11-27 19-28

I've noticed that with this change applied the leading spaces in the first line collapse before the post is reloaded.

Copy link
Member Author

@ellatrix ellatrix Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that’s fine?

Copy link
Member Author

@ellatrix ellatrix Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the rare case that the user want to have those spaces visible on the front end, I think they should insert those non breaking spaces themselves explicitly.


> p:empty {
min-height: $editor-font-size * $editor-line-height;
Expand Down
7 changes: 3 additions & 4 deletions packages/rich-text/src/create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,11 +297,10 @@ function createFromElement( {

const length = element.childNodes.length;

// Remove any line breaks in text nodes. They are not content, but used to
// format the HTML. Line breaks in HTML are stored as BR elements.
// See https://www.w3.org/TR/html5/syntax.html#newlines.
const filterStringComplete = ( string ) => {
string = string.replace( /[\r\n]/g, '' );
// Reduce any whitespace used for HTML formatting to one space
// character, because it will also be displayed as such by the browser.
string = string.replace( /[\n\r\t]+/g, ' ' );
Copy link
Member

@oandregal oandregal Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to replace \r and \n with no spaces as it was before, and any number of tabs with one space?

Copy link
Member Author

@ellatrix ellatrix Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, one of the issues this or fixes is exactly replacing line breaks with a space.


if ( filterString ) {
string = filterString( string );
Expand Down
19 changes: 12 additions & 7 deletions packages/rich-text/src/test/__snapshots__/to-dom.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,13 +317,6 @@ exports[`recordToDom should ignore formats at line separator 1`] = `
</body>
`;

exports[`recordToDom should ignore line breaks to format HTML 1`] = `
<body>

<br />
</body>
`;

exports[`recordToDom should preserve emoji 1`] = `
<body>
🍒
Expand All @@ -339,6 +332,12 @@ exports[`recordToDom should preserve emoji in formatting 1`] = `
</body>
`;

exports[`recordToDom should preserve non breaking space 1`] = `
<body>
test  test
</body>
`;

exports[`recordToDom should remove br with settings 1`] = `
<body>

Expand All @@ -359,6 +358,12 @@ exports[`recordToDom should remove with settings 1`] = `
</body>
`;

exports[`recordToDom should replace characters to format HTML with space 1`] = `
<body>

</body>
`;

exports[`recordToDom should unwrap with settings 1`] = `
<body>
te
Expand Down
30 changes: 24 additions & 6 deletions packages/rich-text/src/test/helpers/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,39 @@ export const spec = [
},
},
{
description: 'should ignore line breaks to format HTML',
html: '\n\n\r\n',
description: 'should replace characters to format HTML with space',
html: '\n\n\r\n\t',
createRange: ( element ) => ( {
startOffset: 0,
startContainer: element,
endOffset: 1,
endContainer: element,
} ),
startPath: [ 0, 0 ],
endPath: [ 0, 0 ],
endPath: [ 0, 1 ],
record: {
start: 0,
end: 0,
formats: [],
text: '',
end: 1,
formats: [ , ],
text: ' ',
},
},
{
description: 'should preserve non breaking space',
html: 'test\u00a0 test',
createRange: ( element ) => ( {
startOffset: 5,
startContainer: element.firstChild,
endOffset: 5,
endContainer: element.firstChild,
} ),
startPath: [ 0, 5 ],
endPath: [ 0, 5 ],
record: {
start: 5,
end: 5,
formats: [ , , , , , , , , , , ],
text: 'test\u00a0 test',
},
},
{
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/__snapshots__/links.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ exports[`Links can be created instantly when a URL is selected 1`] = `

exports[`Links can be created without any text selected 1`] = `
"<!-- wp:paragraph -->
<p>This is Gutenberg: <a href=\\"https://wordpress.org/gutenberg\\">https://wordpress.org/gutenberg</a></p>
<p>This is Gutenberg: <a href=\\"https://wordpress.org/gutenberg\\">https://wordpress.org/gutenberg</a></p>
<!-- /wp:paragraph -->"
`;

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/__snapshots__/rich-text.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`RichText should apply formatting when selection is collapsed 1`] = `
"<!-- wp:paragraph -->
<p>Some <strong>bold</strong>.</p>
<p>Some <strong>bold</strong>.</p>
<!-- /wp:paragraph -->"
`;

Expand Down
4 changes: 2 additions & 2 deletions test/e2e/specs/__snapshots__/undo.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ exports[`undo should undo typing after a pause 2`] = `

exports[`undo should undo typing after non input change 1`] = `
"<!-- wp:paragraph -->
<p>before keyboard <strong>after keyboard</strong></p>
<p>before keyboard <strong>after keyboard</strong></p>
<!-- /wp:paragraph -->"
`;

exports[`undo should undo typing after non input change 2`] = `
"<!-- wp:paragraph -->
<p>before keyboard </p>
<p>before keyboard </p>
<!-- /wp:paragraph -->"
`;
6 changes: 3 additions & 3 deletions test/e2e/specs/__snapshots__/writing-flow.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ exports[`adding blocks should navigate around inline boundaries 1`] = `

exports[`adding blocks should not delete surrounding space when deleting a selected word 1`] = `
"<!-- wp:paragraph -->
<p>alpha  gamma</p>
<p>alpha gamma</p>
<!-- /wp:paragraph -->"
`;

Expand All @@ -136,7 +136,7 @@ exports[`adding blocks should not delete surrounding space when deleting a selec

exports[`adding blocks should not delete surrounding space when deleting a word with Alt+Backspace 1`] = `
"<!-- wp:paragraph -->
<p>alpha  gamma</p>
<p>alpha gamma</p>
<!-- /wp:paragraph -->"
`;

Expand All @@ -148,7 +148,7 @@ exports[`adding blocks should not delete surrounding space when deleting a word

exports[`adding blocks should not delete surrounding space when deleting a word with Backspace 1`] = `
"<!-- wp:paragraph -->
<p>1  3</p>
<p>1 3</p>
<!-- /wp:paragraph -->"
`;

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/adding-inline-tokens.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe( 'adding inline tokens', () => {
await page.click( '.media-modal button.media-button-select' );

// Check the content.
const regex = new RegExp( `<!-- wp:paragraph -->\\s*<p>a\\u00A0<img class="wp-image-\\d+" style="width:\\s*10px;?" src="[^"]+\\/${ filename }\\.png" alt=""\\/?><\\/p>\\s*<!-- \\/wp:paragraph -->` );
const regex = new RegExp( `<!-- wp:paragraph -->\\s*<p>a <img class="wp-image-\\d+" style="width:\\s*10px;?" src="[^"]+\\/${ filename }\\.png" alt=""\\/?><\\/p>\\s*<!-- \\/wp:paragraph -->` );
expect( await getEditedPostContent() ).toMatch( regex );
} );
} );