Deleting BR before widget deletes widget

[nuander]

Steps to reproduce

Hit the delete key with the cursor before a BR element that is before a widget.

Expected result

BR element should be removed

Actual result

The BR element AND the widget after it are removed

Other details

This is similar to the problem as reported in ticket 1572 where the poster reports that an empty P element cannot be deleted before a widget. That issue was fixed in 4.16.1 by modifying selection.js file in the getOnKeyDownListener function.

Fix

I found that by modifying ckeditor 4.17 - selection.js - getOnKeyDownListener function by replacing if ( next && next.type == CKEDITOR.NODE_ELEMENT && next.getAttribute( 'contenteditable' ) == 'false' ) with if ( next && next.type == CKEDITOR.NODE_ELEMENT && next.getAttribute( 'contenteditable' ) != 'true' ) fixed the problem because next.getAttribute( 'contenteditable' ) returns null for the BR element

This had the added benefit of displaying the BR element in the breadcrumbs.

[Comandeer]

I can confirm that br before widget behaves weirdly, but in my test it was not deleted at all after pressing Delete. Can you provide some sample?

[marknpact]

I have retested and found that with a break before an inline widget, if you try to delete the break the widget is deleted but the break is not. For an example go to https://ckeditor.com/docs/ckeditor4/latest/examples/draganddrop.html Switch to Source mode and add a break before the Alice h-card widget. Then switch back and try to delete it.

[github-actions[bot]]

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

[marknpact]

Please do not close this issue. I provided additional info and steps to reproduce. I even provided a possible fix. What else do you need?

[Comandeer]

I'm sorry for delay in responding. Yes, you're right, I was able to reproduce the issue using the scenario you provided. I'm marking the issue as confirmed so our bot won't close it or mark it stale.

[bunglegrind]

I found that by modifying ckeditor 4.17 - selection.js - getOnKeyDownListener function by replacing if ( next && next.type == CKEDITOR.NODE_ELEMENT && next.getAttribute( 'contenteditable' ) == 'false' ) with if ( next && next.type == CKEDITOR.NODE_ELEMENT && next.getAttribute( 'contenteditable' ) != 'true' ) fixed the problem because next.getAttribute( 'contenteditable' ) returns null for the BR element

This had the added benefit of displaying the BR element in the breadcrumbs.

Are you sure? I wrote a manual test and the BR element is removed after hitting two times the del key. The first hit creates a hidden selection element.

[bunglegrind]

btw, the issue is present since 4.3.1. Not related to PR https://github.com/ckeditor/ckeditor4/pull/4433

[marknpact]

@bunglegrind Yes you have to either hit delete twice or hit the right arrow key once so that the BR is listed in the breadcrumbs and then hit delete. that is not perfect but it is still preferable to what it used to do .

[bunglegrind]

My opinion - as a humble contributor - concerning this issue:

1) condition https://github.com/ckeditor/ckeditor4/blob/92173c63f7464b02985888bfeeb4ed80c89b4c94/core/selection.js#L606 must return false when next is a br element (because br is an inline element - you don't need a fake selection for an inline element);

2) the actual bug is the default browser behaviour (tested on firefox and chrome) when you have a br followed by an inline element whose contenteditable attribute is explicitly set to "false". I mean, I can reproduce the very same issue outside of ckeditor