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

[HarryMuc]

Are you reporting a feature request or a bug?

The old issues
"Cannot delete paragraph before and after a widget"
"Delete space above widget"
are still not fixed

Check if the issue is already reported

https://dev.ckeditor.com/ticket/12778
https://dev.ckeditor.com/ticket/13864

Provide detailed reproduction steps (if any)

Just open this page:
https://sdk.ckeditor.com/samples/simplebox.html

Paste this Code:

<h1>&nbsp;</h1>
<div class="simplebox align-center" style="width:750px">
<h2 class="simplebox-title">Title</h2>
<div class="simplebox-content">
<p>Foo <strong>bar</strong>.</p>
<ul>
	<li>Foo!</li>
	<li>Bar!</li>
</ul>
</div>
</div>
<p>&nbsp;</p>
<div class="simplebox align-center" style="width:750px">
<h2 class="simplebox-title">Title</h2>
<div class="simplebox-content">
<p>Foo <strong>bar</strong>.</p>
<ul>
	<li>Foo!</li>
	<li>Bar!</li>
</ul>
</div>
</div>
<p>&nbsp;</p>

And try to delete the paragraphs before, between and after the widgets - not possible!

Expected result

The paragraphs should be deletable

Actual result

Paragraphs are not deleted

Other details

Plase check my workaround in core/selection.js:

	// Handle left, right, delete and backspace keystrokes next to non-editable elements
	// by faking selection on them.
	function getOnKeyDownListener( editor ) {
		var keystrokes = { 37: 1, 39: 1, 8: 1, 46: 1 };

		return function( evt ) {
			var keystroke = evt.data.getKeystroke();

			// Handle only left/right/del/bspace keys.
			if ( !keystrokes[ keystroke ] )
				return;

			var sel = editor.getSelection(),
				ranges = sel.getRanges(),
				range = ranges[ 0 ];

			// Handle only single range and it has to be collapsed.
			if ( ranges.length != 1 || !range.collapsed )
				return;

			var next = range[ keystroke < 38 ? 'getPreviousEditableNode' : 'getNextEditableNode' ]();

			if ( next && next.type == CKEDITOR.NODE_ELEMENT && next.getAttribute( 'contenteditable' ) == 'false' ) {
				/* $custom: added if case to allow removal of empty paragraphs, see incidents
					https://dev.ckeditor.com/ticket/12778
					https://dev.ckeditor.com/ticket/13864 */
				if(sel._ && sel._.cache && sel._.cache.nativeSel && sel._.cache.nativeSel.baseNode && sel._.cache.nativeSel.baseNode.nodeName === 'P'){
					range.endContainer.$.remove();
				} else {
					editor.getSelection().fake( next );
					evt.data.preventDefault();
					evt.cancel();
				}
			}
		};
	}

[jacekbogdanski]

Hello!

I can reproduce the issue and it's related to #749. It would be great if you could push Pull Request with your bug fix. We have some manual how to contribute to CKEDITOR 4, so if you are interested, please check How to contribute article.

[HarryMuc]

#1584

[hissy]

I can still reproduce this issue.

[bunglegrind]

Hey folks, I noticed that HarryMuc provided a fix for this issue, but it was rejected because he didn't follow the contribution guidelines.

I want to resume his contribution, but I am having some difficulty for the setup of the test environment. Where is the right place to ask for support?

[f1ames]

@bunglegrind please refer to our contributing guide.

[bunglegrind]

yes, I've already did it, but there are some discrepances (maybe because I'm using Win10?) .

Specifically:

1. without any change in the master branch, some tests are failing;
2. If I insert invalid lines in the source code, the command grunt does not signal anything

[bunglegrind]

Issue 2) was because grunt checks only the files in stage.

Anyway, I wrote some tests; performed a refactoring on the patch provided by HarryMuc, but I'm still puzzled about the overall test issue (see my point 1 above). Which is the minimum (number/kind/type) of tests a pull request must pass before it can be merged?

[Dumluregn]

Hi @bunglegrind, as for the issue with failing tests - Win10 may be an issue here, could you give total number of failing tests? Basically all tests need to pass on all the browsers we support, but there is not a fixed number of tests that need to be added to the PR - it depends on the issue: sometimes 1 is enough, sometimes 4 is not.

I suggest you push a PR (I think it would be best if you create a new one instead of resurrecting #1584) and we'll see what CI tells about tests.

[bunglegrind]

Sorry, I wasn't referring to the new tests I added, but to the old tests which were failing even before I changed a single line of code.
Anyway, I will follow your suggestion and I will prepare a PR for today. Cheers!

[CKEditorBot]

Closed in #4433

[nuander]

It would be nice if this fix also included the ability to delete break tabs from around widgets