Rich Text: Determine emptiness by value

[aduth]

Note: This pull request has try/undo-buffer (#4956) assigned as a base for easier future merging.

This pull request seeks to remove assignment of a this.isEmpty instance property in the RichText component, and in doing so, avoids calculating emptiness on change.

Open questions:

It's unclear when we'd ever expect this.isEmpty to have been assigned as true in onChange where getContent would otherwise not return an empty array. If this is still possible, I'd be inclined to consolidate filtering recently applied for block splitting via filterEmptyNodes (#5034).

Testing instructions:

Repeat testing instructions from #5034

Verify that the RichText placeholder is shown when expected: when the block is not selected and is empty.

[youknowriad]

I believe this change is creating some issues, you can't reproduce with consistency but some times when you split a paragraph and then empty one of these two paragraphs you'll end up with an empty paragraph but not really considered empty since the inserter doesn't show up.

We might want the filtering behavior like you said.

[ellatrix]

Could we maybe still leave the TinyMCE empty check in place, but still look at the value when rendering instead of creating a new property? Should have done that in the undo PR because the property makes little sense.

I think will still have to either check for emptiness or filter out empty nodes, in which case the question becomes which one is more performant?

[aduth]

Could we maybe still leave the TinyMCE empty check in place, but still look at the value when rendering instead of creating a new property?

I think we'll want both changes eventually, but agree it could have been done in separate steps.

I think will still have to either check for emptiness or filter out empty nodes, in which case the question becomes which one is more performant?

Well, we should want to be consistent in either case.

Looking at the implementation of isEmpty, it does a fair bit more, but also things we should probably want. I'll want to compare/contrast with the isEmptyInlineBoundary and isEmptyNode helpers in RichText, as they may be targeting different things.

[ellatrix]

@aduth What's left to do here? Do we still want this? Or do we want if done differently?

[aduth]

@iseulde I think we do want this, as this.isEmpty is an unnecessary cached derived value which can / should be calculated within render where it's used. That said, the previous consideration of filtering empty nodes to ensure that value is empty when the content is empty could be considered a prerequisite. We'll also need to update this with consideration of the string format prop.

[aduth]

Rebased to resolve conflicts, and changed base to master since #4956 has been merged.

In f9f2739, I applied child node filtering during a split to both the before and after fragments. This should address an issue where pressing Enter from the beginning of a block with text would leave the first with a value of [ '' ] (and thus, not considered empty).

I've played around a bit and am not able to find any other cases where this could occur.

[youknowriad]

I also played around this and it seems to work as intended. I believe we can probably add an e2e test to check that the side inserter is still showing using different ways to create empty paragraphs.

[aduth]

I went further in 26be694 after noticing we have many different touch points considering emptiness in different ways. With #4955 keeping the value should in sync, when the editor is empty the value should never be anything other than an empty array. If it is, that's a bug, and should be addressed directly. With getContent using TinyMCE's isEmpty to return an empty array though, I expect this should be mostly fine as-is. It's the splitting functions that were shown to be problematic here.

I also suspect many of the functions changed may be simply remnants of the pre-#4955 state of the component where we couldn't rely on an up-to-date value.

[ellatrix]

If it is, that's a bug, and should be addressed directly.

That sounds good. I'm fine with trying this.

[aduth]

Thanks @iseulde and @youknowriad !