Fake selection container was not appended to the new fake selection editable when fake selection changes #1679
Conversation
…ion editable when fake selection changes.
src/view/renderer.js
Outdated
| @@ -707,7 +707,10 @@ export default class Renderer { | |||
| container.appendChild( domDocument.createTextNode( '\u00A0' ) ); | |||
| } | |||
|
|
|||
| // Add fake container if not already added. | |||
| if ( container.parentElement && container.parentElement != domRoot ) { | |||
There was a problem hiding this comment.
This if (and below if) could simply be
if ( container.parentElement ) {
container.remove();
}
domRoot.appendChild( container );So simpler, but I am not sure if we want to remove and append dom element (container) if there's no need for it. Can it break something? If not, we can go with simpler solution.
There was a problem hiding this comment.
I'd want to avoid moving it around. That may do some strange things to focus. I even considered leaving just one line: domRoot.appendChild( container ); (and no conditions), but I'm afraid what it may do. It depends on how it's implemented internally in the browsers.
However, if you feel braver than I, then you can test it :D
There was a problem hiding this comment.
I may feel brave but I also feel lazy.
There was a problem hiding this comment.
AFAICS:
domRoot.appendChild( container );- It works (checked the manual tests for tables & widgets, including inline widgets in tables)
- Tests pass
- Memory tests for
editor-classicpasses - No increase in detached DOM nodes (I was worrying that it may happen).
Tested on Chrome only though.
ps.: to be clear:
There was a problem hiding this comment.
Tested on Chrome only though.
Which means nothing ;) It has to be tested thoroughly in all browsers. That's why I'd choose the safe path myself.
src/view/renderer.js
Outdated
| @@ -707,7 +707,10 @@ export default class Renderer { | |||
| container.appendChild( domDocument.createTextNode( '\u00A0' ) ); | |||
| } | |||
|
|
|||
| // Add fake container if not already added. | |||
| if ( container.parentElement && container.parentElement != domRoot ) { | |||
There was a problem hiding this comment.
AFAICS:
domRoot.appendChild( container );- It works (checked the manual tests for tables & widgets, including inline widgets in tables)
- Tests pass
- Memory tests for
editor-classicpasses - No increase in detached DOM nodes (I was worrying that it may happen).
Tested on Chrome only though.
ps.: to be clear:
Suggested merge commit message (convention)
Fix: Fake selection container was not appended to the new fake selection editable when the fake selection changes. Closes ckeditor/ckeditor5#1523.