Rich Text: Only merge RichText on collapsed selection

[aduth]

Fixes #8268

This pull request seeks to resolve an issue where pressing backspace while an entire RichText field is selected (e.g. Paragraph block) would cause its content to be merged into the previous block, rather than removed. It's expected that this was an unintended regression of #7877, where previously isHorizontalEdge would include an option to collapseRange. This was moved to the responsibility of the consumer calling isHorizontalEdge to assure, which was not done previously, but is effected here.

Testing instructions:

Repeat steps to reproduce from #8268, ensuring that the paragraph contents are removed, not merged into the previous block.

Ensure the new end-to-end regression test passes:

npm run test-e2e test/e2e/specs/splitting-merging.test.js

[pento]

Here's another case that behaves incorrectly, I added a failing test for it (c64e6e3):

• Create a Paragraph block
• Press Shift+Enter (adding a soft line break to the paragraph)
• Press Backspace

It should delete the soft line break, but currently doesn't.

[aduth]

Thanks @pento . On glance, it sounds like this could be a separate issue. I'll give it a further look and evaluate whether it should be pulled out to its own pull request.

[LZL0]

Thank you guys for looking into it.

[aduth]

I've spent some time this afternoon revisiting the issue. I believe what's been shown is also present on master, not introduced by these changes. There appears to be a flaw in our implementation of isHorizontalEdge which cause inaccurate values to be returned when the content contains newlines. Specifically, its use of textContent and range.toString() will not account for the br tags:

var el = document.createElement( 'div' );
el.appendChild( document.createElement( 'br' ) )
el.textContent
// ""

I have some ideas to explore further a resolution here, potentially creating a new range at the extent of the container and using Range#compareBoundaryPoints to compare the current against the current selection.

That being said, I believe the changes here are still an improvement upon the current state, and should be merged separate from a fix to these other issues.

[aduth]

I rebased this pull request, dropping your commit @pento, in favor of a separate pull request at #8461.

I did find another issue here though, with a failing test and resolution added in d2ba587. This was caused by some too-clever refactoring I'd attempted in simplifying logic of the "handled" event.

I observed that inserting a paragraph block and immediately pressing backspace would be failed to be recognized in the end-to-end tests. On a hunch given some prior... inconveniences, I added a workaround to "wait" for TinyMCE to initialize. I think this is a hacky solution which should be addressed more directly, but I assume it'll take a fair bit more work to do, and I'd not want to include it here. I've left some "TODO" comments to explain a likely implementation.

[LZL0]

Thanks for looking into it. I do believe that this is a pretty important issue to fix.

[aduth]

The underlying bug was also separately addressed in #8651.

I guess this one is now just a refactor of RichText deletion (method extraction) and more end-to-end test stability (protection against my own introduced bug , and the waitForRichTextInitialization)

[aduth]

Between #8651 and this pull request, I noticed different usage of this.editor.selection.getSelection() vs. window.getSelection(). I don't know that it makes any effective difference which one we use, but we should be consistent.

[aduth]

I'm going to close this as being satisfied largely by #8651. If the waitForRichTextInitialization wasn't needed for those tests to pass, ideally we shouldn't introduce it at all. I expect to salvage some pieces here in a resolution for a new bug at #8731