Fixed: Cannot delete paragraph before and after a widget / Delete space above widget

[bunglegrind]

What is the purpose of this pull request?

fix issue #1572

Does your PR contain necessary tests?

yes

This PR contains

• Unit tests
• Manual tests

Did you follow the CKEditor 4 code style guide?

• PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#1572 ](https://github.com/ckeditor/ckeditor4/issues/1572): Pressing backspace and delete in an empty paragraph between two widgets now deletes the paragraph instead of selecting the next (or previous) widget 

What changes did you make?

Give an overview…

Which issues does your PR resolve?

Closes #1572 .

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[Dumluregn]

Hello again, thank you for the contribution! There are some amendments needed before this PR can be merged - please see the comment below as well as the inline comments.

The fix

The fix works - it is possible to remove any paragraph around the block widget: before, after or between the two of them.

The bad news is that it introduces a pretty nasty regression: user can now create areas unavailable to edit. If the paragraph between the two widgets gets removed, it isn't possible to reintroduce any content between them (the old behaviour kind of prevented such a situation). The same is true for the beginning and end of the document. Please take a look at it as this issue has to be addressed before merging your PR.

Tests

Unit tests are passing (also on older browsers), which is good. However, this test should be placed in the tests/plugins/widget folder and assigned a meaningful name, with ticket references placed inline before each test case - please take a look at the other tests.
Also, the manual tests are a must here - please create at least the one checking the original issue and another one for the case described by me in the previous section (they should be placed in the tests/plugins/widget/manual folder).

The code

See the inline comments and refer to the code guide for the proper solutions.

[bunglegrind]

The fix

The fix works - it is possible to remove any paragraph around the block widget: before, after or between the two of them.

The bad news is that it introduces a pretty nasty regression: user can now create areas unavailable to edit. If the paragraph between the two widgets gets removed, it isn't possible to reintroduce any content between them (the old behaviour kind of prevented such a situation). The same is true for the beginning and end of the document. Please take a look at it as this issue has to be addressed before merging your PR.

Mmm...what about the magic line? You can add again the p tag between two widgets using magic line. That is its purpose, isn't it?

[Dumluregn]

It would be a breaking change for all users that are not using the magic line plugin for now. Fixing one bug for the cost of effectively introducing another one (fixable by adding an extra plugin, but still) isn't worth it. We need to make possible to focus the area between the widgets, at least using keyboard navigation (i.e. when first widget is selected and the right arrow key is pressed, the focus moves between the widgets, not to the second widget right away).

[bunglegrind]

It would be a breaking change for all users that are not using the magic line plugin for now. Fixing one bug for the cost of effectively introducing another one (fixable by adding an extra plugin, but still) isn't worth it. We need to make possible to focus the area between the widgets, at least using keyboard navigation (i.e. when first widget is selected and the right arrow key is pressed, the focus moves between the widgets, not to the second widget right away).

On a second thought, this is not a regression. The bug is not related to this PR: in fact, you can trigger this bug even in the latest version. Just insert two consecutive widgets...and then try to add content between them without resorting to the magic line. You can't.

This is a different issue, I can try to provide a patch, but not in this PR; they're not related.

[bunglegrind]

btw, this is not even a bug - it's the intended behavior (as stated in the function description):

ckeditor4/core/selection.js

Lines 581 to 583 in ece98ca

	// Handle left, right, delete and backspace keystrokes next to non-editable elements
	// by faking selection on them.
	function getOnKeyDownListener( editor ) {

[Dumluregn]

Based on your feedback and internal talks, we decided to create a separate feature request for the paragraph inserting part: #4467. Therefore you can omit this section of the review and focus on the other comments. Thanks again for the commitment to make CKE4 better!

[bunglegrind]

Uh...there was a bug that the tests didn't reveal because it was related to the editable fields of the widgets...but I have used codesnippet which does not have editable fields. I've already fixed, but I need to update the tests.

I've also slightly change, the behaviour of the delete/backspace keys: in addition to delete the paragraph, the next (or previous) widget is (fake) selected, as when up/down/right/left is pressed. Does it sound reasonable?

[Dumluregn]

in addition to delete the paragraph, the next (or previous) widget is (fake) selected, as when up/down/right/left is pressed. Does it sound reasonable?

I think it does - it should make it clearer to understand the selection behaviour 👍

[bunglegrind]

build failed...don't know why. One fail is related to the exportpdf plugin, the other one to pastetools. The former is a known issue, as far as I understood: #4463 (comment)

The latter, well, I run the tests locally and they succeeded. Suggestions?

[f1ames]

build failed...don't know why. One fail is related to the exportpdf plugin, the other one to pastetools. The former is a known issue, as far as I understood: #4463 (comment)

Yes 👍

The latter, well, I run the tests locally and they succeeded. Suggestions?

Looks like some known CI instability we have 🤔 I restarted the failed job and we will see if it passes 🤞

[bunglegrind]

still failing...but a different plugin

[Dumluregn]

still failing...but a different plugin

AFAICS it's exportpdf again 🤔 so I suppose it's ready for review?

[bunglegrind]

still failing...but a different plugin

AFAICS it's exportpdf again 🤔 so I suppose it's ready for review?

yes, it is!

[bunglegrind]

I realized that - maybe - detaching the whole callback whould resolve this issue in a simpler way (a sort of Egg of Columbus). So, I tried on the manual test and indeed, in Firefox (latest version) it succeeded. But in Chrome (latest version), pressing backspace or del didn't change anything.

I'm a bit puzzled about this (huge?) differences between the browsers...since I'm not an expert of frontend development, could you provide hints or suggestions on what is happening behind the curtains? Like: what events are emitted when a key is pressed? How can I debug in realtime this scenario? Or whatever you think is useful.

[bunglegrind]

The reason is that Firefox and Chrome exhibit a different behavior with respect to contentedible when this is true...

a simple snippet:

<!DOCTYPE html>
<html>
    <head>
        <title>My test page</title>
    </head>
    <body contenteditable="true">
      <div contenteditable="false">
          <div contenteditable="true">
              <p>word</p>
          </div>
      </div>
      <p><br></p>
      <div contenteditable="false">
          <div contenteditable="true">
              <p>word</p>
          </div>
      </div>
    </body>
</html>

If I press backspace when the caret is in the p in the middle of the page, firefox deletes the p whereas chrome no...

[Dumluregn]

I'm a bit puzzled about this (huge?) differences between the browsers...

This is the kind of problems we have to deal with every day in CKE 4 😉 Currently we are in the middle of testing phase for the new version release, but I will take a look at your PR at the beginning of the next week and try to push you in the right direction.

[bunglegrind]

ok, thanks!

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[bunglegrind]

Besides the inline comments - one more thing to take care of: in manual test, after you delete the paragraph, undo button isn't enabled.

I think you've just opened a can of worms...

That callback was introduced (7 years ago?) in order to "Handle left, right, delete and backspace keystrokes next to non-editable elements by faking selection on them". The last statement of the callback was (and still is) event.cancel() which actually cancels the keydown event and stop the execution of the other attached callbacks, including the one which enables the undo button.
Since at the time the content wasn't modified by pressing the left/right/del/bs buttons, there were no substancial consequences.
But now bs and del modify the content, and you can observe this weird behaviour.

BTW, the undo snapshot is recorded by the undo plugin within the subsequent keyup event, which is not canceled. And that is the reason you still see the undo step.

I don't know how to solve this particular issue, maybe we should get rid of the whole callback...

Ok, I solved using editor.fire( 'saveSnapshot' ); - This trick is used several times in the editor...

I'm moving to the other points

[bunglegrind]

I think I've done...

Automatic tests are failing on IEx only because IE is rearranging the attributes of the img tag.

[bunglegrind]

Automatic tests are failing on IEx only because IE is rearranging the attributes of the img tag.

Build is failing for the same reason...

[Dumluregn]

@bunglegrind, thank you for the great job you did in this PR! I've just added some final touches. As for the things I'd like to bring to your attention in the future:

1. You had a problem with attributes order in tests. Also, in the latest version you submitted, two tests failed on IE8 because it uses capital letters in tags it creates (so we had <p> vs <P> in test). Fortunately, Bender has a special assertion for such cases: assert.beautified.html(), which solves the problem.
2. I appreciate you wanted to test a number of block elements passing them in an array and keep the testing concise by randomly choosing some of them instead of adding a lot of test cases. There was however one problem with your approach - randomising the test makes it very difficult to debug later (here we didn't even get an info about which block element was selected if it was removed later in the test). So instead of creating a lot of almost identical tests, I've just sticked to three elements that are commonly used in editor contents (<div>, <p> and <h*>) and tested them in one of the scenarios. Now in case of any malfunctioning we'll know exactly which element is problematic.
3. You were merging the master branch into your feature branch. In CKE4 we use git rebase instead to keep all the commits from a branch together - it makes bisecting and looking for potential regressions much easier (although I'm not saying it is always a better choice - just the one we chose in this project). So I've rebased your work onto latest master and you can see they are nicely stacked on top instead of mixed in between other PRs.

I have one final question for you - I see that in the first commit you replaced the condition

if ( ranges.length != 1 || !range.collapsed ) {

with

if ( !sel.isCollapsed() ) {

Did you encounter any case where the old construction was problematic or was that just a simplification?

---

@ckeditor/qa-team - since this PR involves a behavioural change in selection core, I'd like to ask you guys to take a look if it works well from your perspective and if it doesn't break anything. The crucial part is an interaction between selection and widgets. Please note that this change will expose another bug that is already present, but presently a bit hidden in the editor: #4467 (for the justification please check this comment). Let me know If you need any further details.

---

Side note - this PR still needs a changelog entry to be added just before merging.

[bunglegrind]

@bunglegrind, thank you for the great job you did in this PR! I've just added some final touches. As for the things I'd like to bring to your attention in the future:

You're welcome!

1. You had a problem with attributes order in tests. Also, in the latest version you submitted, two tests failed on IE8 because it uses capital letters in tags it creates (so we had `<p>` vs `<P>` in test). Fortunately, Bender has a special assertion for such cases: `assert.beautified.html()`, which solves the problem.

One of the problems I faced while writing this PR was related to the lack of documentation in Bender. Looking into the other tests for "inspiration" was sometimes like to look for a needle in a haystack...I'm still not 100% sure that the assertions I've employed are the most proper ones.

2. I appreciate you wanted to test a number of block elements passing them in an array and keep the testing concise by randomly choosing some of them instead of adding a lot of test cases. There was however one problem with your approach - randomising the test makes it very difficult to debug later (here we didn't even get an info about which block element was selected if it was removed later in the test). So instead of creating a lot of almost identical tests, I've just sticked to three elements that are commonly used in editor contents (`<div>`, `<p>` and `<h*>`) and tested them in one of the scenarios. Now in case of any malfunctioning we'll know exactly which element is problematic.

Ok, fine.

3. You were merging the `master` branch into your feature branch. In CKE4 we use `git rebase` instead to keep all the commits from a branch together - it makes bisecting and looking for potential regressions much easier (although I'm not saying it is always a better choice - just the one we chose in this project). So I've rebased your work onto latest `master` and you can see they are nicely stacked on top instead of mixed in between other PRs.

So, you're squashing the commits. I should have squashed my commits and rebased to the current master branch, if I got the point correctly. If so, I suggest you to improve the related doc page: https://ckeditor.com/docs/ckeditor4/latest/guide/dev_contributing_code.html

I have one final question for you - I see that in the first commit you replaced the condition

if ( ranges.length != 1 || !range.collapsed ) {

with

if ( !sel.isCollapsed() ) {

Did you encounter any case where the old construction was problematic or was that just a simplification?

I think it enhances code readibility and maintainability (in case the selection/range implementation changes in the future, the client code won't be affected). I would not call it a simplification, though.

@ckeditor/qa-team - since this PR involves a behavioural change in selection core, I'd like to ask you guys to take a look if it works well from your perspective and if it doesn't break anything. The crucial part is an interaction between selection and widgets. Please note that this change will expose another bug that is already present, but presently a bit hidden in the editor: #4467 (for the justification please check this comment). Let me know If you need any further details.

btw, I've recently submitted a PR related to this issue, because you were suggesting that these two bugs must be fixed at the same time.

Side note - this PR still needs a changelog entry to be added just before merging.

Are you referring to me or the qa team?

[Dumluregn]

One of the problems I faced while writing this PR was related to the lack of documentation in Bender.

Besides existing test base we usually just check the Bender's source code - you can find some extra comments there (like here). And there is no better docs than source code after all.

So, you're squashing the commits. I should have squashed my commits and rebased to the current master branch, if I got the point correctly.

Not really, I meant that all the commits from one PR are in order, not that they are squashed. No need from you to do anything more.

I think it enhances code readibility and maintainability (in case the selection/range implementation changes in the future, the client code won't be affected). I would not call it a simplification, though.

Ok, thanks.

btw, I've recently submitted a PR related to this issue, because you were suggesting that these two bugs must be fixed at the same time.

I've seen it, we'll handle it soon 👍🏻

Are you referring to me or the qa team?

Myself 🙃

[LukaszGudel]

I've tested this PR and it's looking good 👍 I did not find any weird behaviours, bugs or crashes. Now it's very intuitive to remove empty paragraphs before or after a widget. When #4467 will be ready, then overall it will greatly enhance the user experience when working with widget elements

[Dumluregn]

@LukaszGudel thank you for quick response 🙏🏻

We are done here 👍🏻