Added handling for delete key

[jeremyVignelles]

Pressing delete at the end of a block merges blocks (fixes #821)
Pressing delete after a rectangular block selection removes the blocks (fixes #1315)

[gohabereg]

The last condition is not clear for me. I suppose there should be currentBlock.lastInput instead
But I believe such behaviour is not really intuitive, let me explain why.
For example you have list tool, you create couple of lines (but don't type any text there) and press delete key on the last line. You wouldn't expect this will delete the list block

[jeremyVignelles]

Indeed, looks like a bad copy/paste of that line:

editor.js/src/components/modules/blockEvents.ts

Line 288 in 3b20c4a

if (currentBlock.selected || (currentBlock.isEmpty && currentBlock.currentInput === currentBlock.firstInput)) {

For example you have list tool, you create couple of lines (but don't type any text there) and press delete key on the last line. You wouldn't expect this will delete the list block

I just copied the behavior of the backspace key. Why would it make sense when pressing backspace and not when pressing delete ? Both have the same semantics for me, only the direction changes.

[gohabereg]

Well, it makes sense for paragraph tool, but for more complex tools it looks unintuitive for me. When I press delete I expect next entity to be deleted (or merged with the current one), but not current entity deletion. I might be wrong, so could you please apply changes and I'll take a look in action

[jeremyVignelles]

Changes are already made and rebased.

Being able to delete what has been selected with a rectangular selection is exactly the point of #1315 , and it is working as I would expect.

During my testing, I found a bug on firefox with the current version (on editorjs.io), that somehow disappeared with my version. When pressing delete on an empty list item, the item is removed and the carret is placed at the end of the previous item. Pressing enter does not create a new item anymore.

[gohabereg]

See comment above, I don't think delete key should remove current block in any case

[jeremyVignelles]

As a windows user, I'm using the delete key more often for such tasks than the backspace key.
If you are selecting a block, pressing delete should delete it ( isn't that the point of the delete key, after all ? 😄 )

I have to admit though, that I copied that comment without understanding what it means. I don't see how it could be empty.

[jeremyVignelles]

Thanks for the review. Should I rebase on the next branch to integrate the new commits ?

[gohabereg]

@jeremyVignelles yes please

[gohabereg]

For some reason I can't merge anything except two paragraphs using delete key and also have noticed few more uncovered cases. Will investigate it on the weekend and get back to you

[jeremyVignelles]

Do the other "merges" work with the backspace key?

[gohabereg]

You are right, they shouldn't, but there are some of my discovers:

Pre-requirements	Steps	Actual	Expected
Two not-empty different blocks one by one	Set cursor to the end of the first block and press delete	Nothing is happen	If the same happens for backspace, caret is set to the previous block. So for delete I'd expect caret to set to the next block
Not-empty block and then an empty block	- "" -	- "" -	I'd expect empty Block to be deleted
- "" -	Set cursor to the empty block and press delete	Block is deleted, caret is set to previous block	I'd expect caret to be set to the next block
Not-empty block and then block without inputs (eg Delimiter)	Set cursor to the end of the first block and press delete	Nothing is happen	I'd expect Delimiter to be deleted

[jeremyVignelles]

Two not-empty different blocks one by one

I'm not sure, but I'd consider it a bug of the backspace key. I'm not sure that's something I would expect to be normal : If I pressed backspace instead of the left arrow key, that's because I wanted to destroy something, which obviously didn't work but had a "side effect" of moving the caret.

EDIT : This seems to be explicitely wanted :

editor.js/src/components/modules/blockEvents.ts

Line 357 in 32ac52a

* other case will handle as usual ARROW LEFT behaviour

. I'm still not sure that's a desired behavior, but I can match that the other way around

Other points

Seems like the mergeBlocks methods assume that targetBlock is just before blockToMerge, but it doesn't handle well the cases you mentionned. I'm digging further and I'll get back to you

[jeremyVignelles]

So I fixed the 3 last points (in 3 different commits to make it easier to review). Thanks for testing and pointing that out !

As for the first point, I'm not sure how this should be handled:

editor.js/src/components/modules/blockEvents.ts

Lines 370 to 372 in 32ac52a

	if (Caret.navigatePrevious()) {
	Toolbar.close();
	}

In this block, I dont know in which direction the merge occurred. Sure, I could detect it by the position of the caret, or by passing an additional parameter. Is that really something useful though ?

For example, I'm on the image caption control :

I made an error, and I press backspace to the beginning of the input, as I am not patient, I'm holding down the backspace key, until everything is removed. If I don't release the key on time, the caret is moved to the previous block, and starts removing code above.

[gohabereg]

@jeremyVignelles agree the first point is arguable, I'm ok with the current behaviour, but it conflicts with backspace behaviour. Let's listen to @neSpecc and @khaydarov opinion.

I've added some test cases, hope you don't mind

[jeremyVignelles]

Great, thanks for the test case, I never used cypress, I'll see how it works then 🙂

[jeremyVignelles]

Anything else is needed here ? Can it be reviewed now ?

[gohabereg]

@neSpecc @khaydarov take a look please

[hyun-park]

Any updates on this?

[jeremyVignelles]

Just merged the latest "next" and tried to resolve the conflicts that I had.
Ready again for review

[mbagsik00]

Hi, may I know what's the update on this? Thanks

[NeoKms]

Hello! When?

[julianpeterson1]

Hi there, we're also waiting for this solution, thanks!

[kimchi-hent]

Any progress on this?

[cicelle]

Any news on that front ?

[neSpecc]

Resolved by #2402
Available starting 2.28.0-rc.1
2.28 will be released this week.