Nested contenteditables + comment markers make page jumps

[ma2ciek]

How to reproduce?

1. Create an image
2. Write some text in its caption
3. Create a comment to the caption
4. Scroll down the page.
5. Select text in the caption
6. Select text in the main content editable element

Current behavior:
Scroll jumps to the top.

Additional info
Reproducible on Chrome, Safari
The same happens with table cells.

I've tried to debug it with @oskarwrobel and here are our thoughts:

Clicking the marker makes the comment's contenteditable active and triggers its rendering. Then Clicking outside of it, makes it inactive, rerenders the comment and rerenders the main content editable element. It triggers two times https://github.com/ckeditor/ckeditor5-engine/blob/5c0a34d6f277fed063d8ed40af5b8ebda4690e14/src/view/renderer.js#L756
which probably doesn't find the selection in the editor for the second time, thus it creates a selection for a while at the top of the editor and scrolls to it.

Possible solutions:

• Switch these two lines - https://github.com/ckeditor/ckeditor5-engine/blob/5c0a34d6f277fed063d8ed40af5b8ebda4690e14/src/view/renderer.js#L231-L232 by @oskarwrobel. Not sure if it doesn't break something else.
• Add setTimeout 0 in comments when they change state between active and inactive.
• Add domElement.focus( { preventScroll: false } ). Unfortunately, it works only in Chrome, doesn't work in Safari and it looks like a "postfixer".

WDYT?
cc @pjasiun @Reinmar

[oskarwrobel]

Clicking the marker makes the comment's contenteditable active and triggers its rendering. Then Clicking outside of it, makes it inactive, rerenders the comment and rerenders the main content editable element.

I think It has nothing to do with comment contenteditable. When clicking the marker inside nested contenteditable then 3 things happen at the same time:

• selection update
• focus update
• marker re-render (from inactive to active)

I guess something is happening (selection or focus update) while marker's span update and the target element is not present in the DOM.

[oskarwrobel]

Switch these two lines by @oskarwrobel. Not sure if it doesn't break something else.

It's just a quick shoot. I haven't debugged it deeply.

[jodator]

Hi guys... I had similar problem while creating a custom selection for tables (block selection). I set selection in model and I neeed to stop selection rendering then to not make selection flicker.

I've added simple switch:

// TODO: Add temporary solution to pausing selection rendering.
if ( this.renderSelection ) {
	this._updateSelection();
	this._updateFocus();
}

and I use it to pause selection rendering during block selection.

Without pausing something (the DOM?) was interferring with model selection and the whole thing doesn't worked well.

[jodator]

Compare:

1. as on mater

2. with "off" switch

ps.: The table feature pauses selection rendering while in block mode.

[pjasiun]

I think that the guilty line is: https://github.com/ckeditor/ckeditor5-engine/blob/5c0a34d6f277fed063d8ed40af5b8ebda4690e14/src/view/renderer.js#L756 (as you mentioned before). It updates focus on the "selection update" part of the rendering in a very naive way (note that we update focus in the "focus update" part in a much more complex way, including checking isFocused and saving the scroll position). If you remove this line, everything works well. But, of course, since it is some FF hack we cannot remove it just like this.

When I started logging renderer.isFocused and domRoot (used in the code linked above) I get interesting results:

When I click on the comment in table editor, lose focus for a while and use global contenteditable a the domRoot and focus it. This is when the editor jump. So the real question here is: why editor lose focus?

[pjasiun]

Also, I think that @jodator case in not related to this issue. But as long as we will not solve both I will not be sure.

[pjasiun]

So, with @Reinmar we were able to debug this problem. The flow is like this:

• user changes the selection in the DOM,
• new selection (in the marker) is converted to the model,
• comments markers plugin test if the selection is in a comment, changes the active marker and ask for rendering this change (note that we downcast changes first and the selection later, so at this point model selection is not yet converted back to the view),
• renderer render change.

At this point:

• DOM selection is already moved to the nested editable,
• model selection is in the comment in the nested editable,
• view selection is still in the main editable.

Then this code is executed: https://github.com/ckeditor/ckeditor5-engine/blob/5c0a34d6f277fed063d8ed40af5b8ebda4690e14/src/view/renderer.js#L739-L756

It takes the selection from view, asks for the corresponding DOM element and tries to focus it. However, because the view has an old selection it returns wrong editable element (main one) in which there is no DOM selection (because the selection is already in the nested editable). Focusing element with no selection cases page jump. TADAM!

[pjasiun]

I pushed the proposal of the fix to t/1528.

If prevent rendering when the user is in the model change block.

On the one hand, this is something I was thinking about for a long time. On the other, there is a reason we delayed this change for that long. It is ugly and makes "big look" logic even more complex.

[Reinmar]

I'm ok with that change. Maybe not the code itself (that flag should be protected (for now), there's a code style issue and fire() is called with unnecessary param), but the idea is good. We need a way to wrap multiple view.change() blocks in one "transaction" to avoid intermediate rendering. Right now we can have a protected API for that and only we'll use it, so we can revert it at any time. But if we'll prove that it's the right direction we can open it.