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.

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

Jump to bottom

Raw handling: preserve empty paragraphs #59476

Merged
merged 5 commits into from Mar 28, 2024
Merged

Raw handling: preserve empty paragraphs #59476

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b14e145
Raw handling: preserve empty paragraphs
ellatrix Feb 29, 2024
a74da92
adjust shortcode tests
ellatrix Feb 29, 2024
6f39d39
Fix for nextpage too
ellatrix Mar 28, 2024
1ba1858
Update comment
ellatrix Mar 28, 2024
b65754d
Restore the functions
ellatrix Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/blocks/src/api/raw-handling/blockquote-normaliser.js
View file
Open in desktop
Expand Up @@ -3,10 +3,12 @@
*/
import normaliseBlocks from './normalise-blocks';

export default function blockquoteNormaliser( node ) {
if ( node.nodeName !== 'BLOCKQUOTE' ) {
return;
}
export default function blockquoteNormaliser( options ) {
return ( node ) => {
if ( node.nodeName !== 'BLOCKQUOTE' ) {
return;
}

node.innerHTML = normaliseBlocks( node.innerHTML );
node.innerHTML = normaliseBlocks( node.innerHTML, options );
};
}
4 changes: 2 additions & 2 deletions packages/blocks/src/api/raw-handling/index.js
View file
Open in desktop
Expand Up @@ -65,11 +65,11 @@ export function rawHandler( { HTML = '' } ) {
figureContentReducer,
// Needed to create the quote block, which cannot handle text
// without wrapper paragraphs.
blockquoteNormaliser,
blockquoteNormaliser( { raw: true } ),
];

piece = deepFilterHTML( piece, filters, blockContentSchema );
piece = normaliseBlocks( piece );
piece = normaliseBlocks( piece, { raw: true } );

return htmlToBlocks( piece, rawHandler );
} )
Expand Down
4 changes: 2 additions & 2 deletions packages/blocks/src/api/raw-handling/normalise-blocks.js
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@
*/
import { isEmpty, isPhrasingContent } from '@wordpress/dom';

export default function normaliseBlocks( HTML ) {
export default function normaliseBlocks( HTML, options = {} ) {
const decuDoc = document.implementation.createHTMLDocument( '' );
const accuDoc = document.implementation.createHTMLDocument( '' );

Expand Down Expand Up @@ -47,7 +47,7 @@ export default function normaliseBlocks( HTML ) {
}
} else if ( node.nodeName === 'P' ) {
// Only append non-empty paragraph nodes.
if ( isEmpty( node ) ) {
if ( isEmpty( node ) && ! options.raw ) {
decu.removeChild( node );
} else {
accu.appendChild( node );
Expand Down
2 changes: 1 addition & 1 deletion packages/blocks/src/api/raw-handling/paste-handler.js
View file
Open in desktop
Expand Up @@ -191,7 +191,7 @@ export function pasteHandler( {
commentRemover,
iframeRemover,
figureContentReducer,
blockquoteNormaliser,
blockquoteNormaliser(),
divNormaliser,
];

Expand Down
15 changes: 11 additions & 4 deletions packages/blocks/src/api/raw-handling/shortcode-converter.js
View file
Open in desktop
Expand Up @@ -14,6 +14,9 @@ import { applyBuiltInValidationFixes } from '../parser/apply-built-in-validation
const castArray = ( maybeArray ) =>
Array.isArray( maybeArray ) ? maybeArray : [ maybeArray ];

const beforeLineRegexp = /(\n|<p>)\s*$/;
const afterLineRegexp = /^\s*(\n|<\/p>)/;

function segmentHTMLToShortcodeBlock(
HTML,
lastIndex = 0,
Expand Down Expand Up @@ -56,8 +59,8 @@ function segmentHTMLToShortcodeBlock(
if (
! match.shortcode.content?.includes( '<' ) &&
! (
/(\n|<p>)\s*$/.test( beforeHTML ) &&
/^\s*(\n|<\/p>)/.test( afterHTML )
beforeLineRegexp.test( beforeHTML ) &&
afterLineRegexp.test( afterHTML )
)
) {
return segmentHTMLToShortcodeBlock( HTML, lastIndex );
Expand Down Expand Up @@ -143,9 +146,13 @@ function segmentHTMLToShortcodeBlock(
}

return [
...segmentHTMLToShortcodeBlock( beforeHTML ),
...segmentHTMLToShortcodeBlock(
beforeHTML.replace( beforeLineRegexp, '' )
),
...blocks,
...segmentHTMLToShortcodeBlock( afterHTML ),
...segmentHTMLToShortcodeBlock(
afterHTML.replace( afterLineRegexp, '' )
),
];
}

Expand Down
91 changes: 37 additions & 54 deletions packages/blocks/src/api/raw-handling/special-comment-converter.js
View file
Open in desktop
Expand Up @@ -23,58 +23,20 @@ export default function specialCommentConverter( node, doc ) {
return;
}

if ( node.nodeValue === 'nextpage' ) {
replace( node, createNextpage( doc ) );
if (
node.nodeValue !== 'nextpage' &&
node.nodeValue.indexOf( 'more' ) !== 0
) {
return;
}

if ( node.nodeValue.indexOf( 'more' ) === 0 ) {
moreCommentConverter( node, doc );
}
}

/**
* Convert `<!--more-->` as well as the `<!--more Some text-->` variant
* and its `<!--noteaser-->` companion into the custom element
* described in `specialCommentConverter()`.
*
* @param {Node} node The node to be processed.
* @param {Document} doc The document of the node.
* @return {void}
*/
function moreCommentConverter( node, doc ) {
// Grab any custom text in the comment.
const customText = node.nodeValue.slice( 4 ).trim();

/*
* When a `<!--more-->` comment is found, we need to look for any
* `<!--noteaser-->` sibling, but it may not be a direct sibling
* (whitespace typically lies in between)
*/
let sibling = node;
let noTeaser = false;
while ( ( sibling = sibling.nextSibling ) ) {
if (
sibling.nodeType === sibling.COMMENT_NODE &&
sibling.nodeValue === 'noteaser'
) {
noTeaser = true;
remove( sibling );
break;
}
}

const moreBlock = createMore( customText, noTeaser, doc );
const block = createBlock( node, doc );

// If our `<!--more-->` comment is in the middle of a paragraph, we should
// split the paragraph in two and insert the more block in between. If not,
// the more block will eventually end up being inserted after the paragraph.
if (
! node.parentNode ||
node.parentNode.nodeName !== 'P' ||
node.parentNode.childNodes.length === 1
) {
replace( node, moreBlock );
if ( ! node.parentNode || node.parentNode.nodeName !== 'P' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ellatrix: can you explain this change? The code no longer matches the comment. If you can confirm my intuition here:

  • Now the else branch will be run whenever the comment node's parent is a paragraph, even if it only has one child (the comment node).
  • This forces all paragraphs through the paragraphBuilder walk, ensuring that paragraphs that only contained the comment (and would become <p></p>) are destroyed.

If the above is right, some follow-up questions:

  • Is it necessary to the current PR, or an adjacent change?
  • Looking at the tests in packages/blocks/src/api/raw-handling/test/special-comment-converter.js, there is now some inconsistency between the handling of <!--more--> and <!--nextpage--> — with the latter, tests still expect empty paragraph nodes to be produced. Should we fix that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not adjacent, it was required to update this because some of the integration tests started failing with extra empty paragraphs that the original change was no longer removing.

I didn't think the comment no longer matched, but I reworded it.

I also fixed it for next page.

replace( node, block );
} else {
const childNodes = Array.from( node.parentNode.childNodes );
const nodeIndex = childNodes.indexOf( node );
Expand All @@ -93,7 +55,7 @@ function moreCommentConverter( node, doc ) {
// Split the original parent node and insert our more block
[
childNodes.slice( 0, nodeIndex ).reduce( paragraphBuilder, null ),
moreBlock,
block,
childNodes.slice( nodeIndex + 1 ).reduce( paragraphBuilder, null ),
].forEach(
( element ) =>
Expand All @@ -105,7 +67,35 @@ function moreCommentConverter( node, doc ) {
}
}

function createMore( customText, noTeaser, doc ) {
function createBlock( commentNode, doc ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the convenience of folding everything into a single function. However, the resulting function is more difficult to understand for someone who doesn't have our context. Can we add a comment explaining that this function handles both more and nextpage comments, for ease of maintenance?

If we wanted to go the extra mile, we could even make that explicit in code, either with a switch:

switch ( commentNode.nodeValue.match(/\w+/)?.[0] ) {
  case 'nextpage':
    // do stuff
    break;
  case 'more':
    // do stuff
    break;
}

Or simply via function delegation:

function createBlock( commentNode, doc ) {
  if ( commentNode.nodeValue === 'nextpage' ) {
    return createNextPage( doc );
  }
  return createMore( commentNode, doc );
}

Copy link
Member Author

@ellatrix ellatrix Mar 28, 2024 •
edited

Choose a reason for hiding this comment

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

Restored the original functions :)

if ( commentNode.nodeValue === 'nextpage' ) {
const node = doc.createElement( 'wp-block' );
node.dataset.block = 'core/nextpage';

return node;
}

// Grab any custom text in the comment.
const customText = commentNode.nodeValue.slice( 4 ).trim();

/*
* When a `<!--more-->` comment is found, we need to look for any
* `<!--noteaser-->` sibling, but it may not be a direct sibling
* (whitespace typically lies in between)
*/
let sibling = commentNode;
let noTeaser = false;
while ( ( sibling = sibling.nextSibling ) ) {
if (
sibling.nodeType === sibling.COMMENT_NODE &&
sibling.nodeValue === 'noteaser'
) {
noTeaser = true;
remove( sibling );
break;
}
}

const node = doc.createElement( 'wp-block' );
node.dataset.block = 'core/more';
if ( customText ) {
Expand All @@ -117,10 +107,3 @@ function createMore( customText, noTeaser, doc ) {
}
return node;
}

function createNextpage( doc ) {
const node = doc.createElement( 'wp-block' );
node.dataset.block = 'core/nextpage';

return node;
}
2 changes: 1 addition & 1 deletion packages/blocks/src/api/raw-handling/test/blockquote-normaliser.js
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ describe( 'blockquoteNormaliser', () => {
it( 'should normalise blockquote', () => {
const input = '<blockquote>test</blockquote>';
const output = '<blockquote><p>test</p></blockquote>';
expect( deepFilterHTML( input, [ blockquoteNormaliser ] ) ).toEqual(
expect( deepFilterHTML( input, [ blockquoteNormaliser() ] ) ).toEqual(
output
);
} );
Expand Down
16 changes: 8 additions & 8 deletions packages/blocks/src/api/raw-handling/test/special-comment-converter.js
View file
Open in desktop
Expand Up @@ -8,22 +8,22 @@ describe( 'specialCommentConverter', () => {
it( 'should convert a single "more" comment into a basic block', () => {
expect(
deepFilterHTML( '<p><!--more--></p>', [ specialCommentConverter ] )
).toEqual( '<p></p><wp-block data-block="core/more"></wp-block>' );
).toEqual( '<wp-block data-block="core/more"></wp-block>' );
} );
it( 'should convert a single "nextpage" comment into a basic block', () => {
expect(
deepFilterHTML( '<p><!--nextpage--></p>', [
specialCommentConverter,
] )
).toEqual( '<p></p><wp-block data-block="core/nextpage"></wp-block>' );
).toEqual( '<wp-block data-block="core/nextpage"></wp-block>' );
} );
it( 'should convert two comments into a block', () => {
expect(
deepFilterHTML( '<p><!--more--><!--noteaser--></p>', [
specialCommentConverter,
] )
).toEqual(
'<p></p><wp-block data-block="core/more" data-no-teaser=""></wp-block>'
'<wp-block data-block="core/more" data-no-teaser=""></wp-block>'
);
} );
it( 'should pass custom text to the block', () => {
Expand All @@ -33,7 +33,7 @@ describe( 'specialCommentConverter', () => {
[ specialCommentConverter ]
)
).toEqual(
'<p></p><wp-block data-block="core/more" data-custom-text="Read all about it!" data-no-teaser=""></wp-block>'
'<wp-block data-block="core/more" data-custom-text="Read all about it!" data-no-teaser=""></wp-block>'
);
} );
it( 'should not break content order', () => {
Expand Down Expand Up @@ -95,7 +95,7 @@ describe( 'specialCommentConverter', () => {
[ specialCommentConverter ]
);
expect( output ).toEqual(
'<p></p><wp-block data-block="core/more" data-no-teaser=""></wp-block>'
'<wp-block data-block="core/more" data-no-teaser=""></wp-block>'
);
} );
it( 'should not break content order', () => {
Expand All @@ -108,7 +108,7 @@ describe( 'specialCommentConverter', () => {
);
expect( output ).toEqual(
`<p>First paragraph.</p>
<p></p><wp-block data-block=\"core/more\"></wp-block>
<wp-block data-block=\"core/more\"></wp-block>
<p>Second paragraph</p>
<p>Third paragraph</p>`
);
Expand All @@ -124,9 +124,9 @@ describe( 'specialCommentConverter', () => {
);
expect( output ).toEqual(
`<p>First page.</p>
<p></p><wp-block data-block=\"core/nextpage\"></wp-block>
<wp-block data-block=\"core/nextpage\"></wp-block>
<p>Second page</p>
<p></p><wp-block data-block=\"core/nextpage\"></wp-block>
<wp-block data-block=\"core/nextpage\"></wp-block>
<p>Third page</p>`
);
} );
Expand Down
18 changes: 18 additions & 0 deletions test/integration/__snapshots__/blocks-raw-handling.test.js.snap
View file
Open in desktop
Expand Up @@ -198,3 +198,21 @@ exports[`rawHandler should preserve alignment 1`] = `
<p class="has-text-align-center">center</p>
<!-- /wp:paragraph -->"
`;

exports[`rawHandler should preserve all paragraphs 1`] = `
"<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>&nbsp;&nbsp;</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->"
`;
8 changes: 8 additions & 0 deletions test/integration/blocks-raw-handling.test.js
View file
Open in desktop
Expand Up @@ -560,4 +560,12 @@ describe( 'rawHandler', () => {
const HTML = '<p style="text-align:center">center</p>';
expect( serialize( rawHandler( { HTML } ) ) ).toMatchSnapshot();
} );

it( 'should preserve all paragraphs', () => {
const HTML = `<p></p>
<p>&nbsp;&nbsp;</p>
<p class="p"></p>
<p style="border: 1px solid tomato;"></p>`;
expect( serialize( rawHandler( { HTML } ) ) ).toMatchSnapshot();
} );
} );