RichText: Ensure that hitting enter at the end of a paragraph creates an empty paragraph

[youknowriad]

Testing instructions

• Hit enter at the end of a paragraph
• The side inserter should show up in the newly created paragraph

[aduth]

I've taken a few passes at refactoring this logic, which I've generalized to filtering out empty nodes when a split occurs, including empty text nodes and inline boundary placeholder nodes.

This resolves a few related bugs present in master:

1. Add a paragraph ending in a link
2. Add a paragraph following the first
3. From the start of the second paragraph, press backspace
4. Press enter

Master: An empty inline boundary fragment is included in the split paragraph

1. Add a paragraph ending in a code block
2. Move caret to the end of the code block
3. Press enter

Master: An empty inline boundary fragment is included in the split paragraph

[aduth]

Related: #1246 (comment)

[aduth]

Aside: tinymce.DOM.isEmpty behaves a bit strangely when it encounters this caret placeholder:

tinymce.DOM.isEmpty( 'Is this empty? ' )
// true

Edit: Upon further testing, this has nothing to do with the caret placeholder, but rather that isEmpty doesn't expect a string value.

[ellatrix]

Isn't the problem when a new RichText instance is created? I adjusted the check in the constructor and that seems to work: https://github.com/WordPress/gutenberg/pull/4956/files#diff-766381e792acf9d4c2505f3aeddcca7eR111

Where the problem is [ '' ] not being considered empty.

[ellatrix]

This is not just on links anymore right? Could happen for bold etc. as well?

[aduth]

Correct, in fact, this fixes the previous issue of only accounting for anchor. We test any kind of node, but from what I've tested and assuming from the link plugin Utils.js is that it uses a different property to test (innerText for links, textContent for all others).

[ellatrix]

Could the same not happen on beforeElement?

[youknowriad]

The issue I'm seeing is that if the empty elements (br) are in the middle of the after content, they are also removed from the newly created block.

[youknowriad]

In theory, this issue could happen in beforeElement when you trigger Enter at the beginning of a paragraph block but in my testings the block is considered Empty properly.

[aduth]

Where the problem is [ '' ] not being considered empty.

Shouldn't we try to prevent this from ever occurring? That's what's being proposed in the latest changes here.

[aduth]

Shouldn't we try to prevent this from ever occurring? That's what's being proposed in the latest changes here.

In fact, with these changes, plus this bit of logic:

gutenberg/blocks/rich-text/index.js

Lines 381 to 382 in 4d835c0

	const isEmpty = tinymce.DOM.isEmpty( this.editor.getBody() );
	this.savedContent = isEmpty ? [] : this.getContent();

...it seems we shouldn't need to manage empty in state at all, and could just rely on:

const isEmpty = this.props.value.length === 0;

?

Would be a nice simplification because we could drop all of the logic surrounding onSelectionChange and onKeyUp.

[aduth]

The issue I'm seeing is that if the empty elements (br) are in the middle of the after content, they are also removed from the newly created block.

In 3476444, I updated isEmptyNode to check exclusively for the empty text node, not element nodes. I included a few unit additional unit tests for real-world scenarios where an empty text node might be present and gave it a few rounds of testing in the browser. These issues seem to be resolved now.

[ellatrix]

@aduth Yes, ideally we would rely on the value for the isEmpty check, but there are many more cases where the content would be empty and not just an empty array. [ <br>, '' ] etc. If we can filter all those out through getContent and keep the content in sync, then great.