The blinking caret after embeding a media inside a table

[pomek]

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

Master

📋 Steps to reproduce

1. Open docs page: /features/table.html,
2. Put the caret inside a cell,
3. Shift+Enter,
4. Paste: https://www.youtube.com/watch?v=H08tGjXNHO4

✅ Expected result

I am not sure because other browsers produce different results:

• Firefox: Media is selected, the caret isn't displayed.
• Safari: Media isn't selected, the caret is still in the second line.

❎ Actual result

The caret is blinking and the media is selected.

📃 Other details that might be useful

MacOS / Chrome 72.0.3626.109

[Reinmar]

If this can be reproduced outside tables and if this is a regression, we need to add it to it22. Otherwise, move it to it23.

[jodator]

Only in tables. Dunno if regression though.

[jodator]

The same happens with using button:

[Reinmar]

Only in tables. Dunno if regression though.

This happens only in tables so it cannot be a regression since before this release we did not support nested widgets at all.

[Reinmar]

As I commented in ckeditor/ckeditor5-table#176 (comment) I think the bug is somewhere in the engine/widget logic. It's weird that we've got a native selection + widget selection. If they are looking like this:

model:
<paragraph>Foo</paragraph><table><tableRow><tableCell><paragraph>Bar<softBreak></softBreak></paragraph>[<media url="https://www.youtube.com/watch?v=H08tGjXNHO4"></media>]</tableCell></tableRow></table>

view:
<p>Foo</p><figure class="ck-widget ck-widget_with-selection-handler table" contenteditable="false"><div class="ck ck-widget__selection-handler"></div><table><tbody><tr><td class="ck-editor__editable ck-editor__nested-editable ck-editor__nested-editable_focused" contenteditable="true"><p>Bar<br></br></p>[<figure class="ck-widget ck-widget_selected media" contenteditable="false"><div class="ck-media__wrapper" data-oembed-url="https://www.youtube.com/watch?v=H08tGjXNHO4"></div></figure>]</td></tr></tbody></table></figure>

Then we should see a fake selection being rendered. But that's not true on master now. There's no fake selection so it means that something strange happens when this specific view is being rendered.

[jodator]

@Reinmar are you sure that the fake selection is not in the DOM in this fix?

It is in the table cell (other bug):

[Reinmar]

I was testing the situation before this fix. I update my comment to make it clearer.

[jodator]

I was testing the situation before this fix. I update my comment to make it clearer.

But the View table post-fixer was setting normal selection on the element which was causing the bug. The fake selection was set and after that the view table cell post fixer was setting view selection based on model selection so overriding the fake selection previously set by the widget.

In the fix I restricted the view table cell post fixer from changing the selection if it is already OK. It should only fix a selection when it renamed <span> to <p> and the selection was still on the renamed element (happened with Delete AFAIR).

I'm pretty confident that the issue was view table cell post fixer messing with view selection (not respecting isFake on it and doing too much then it should).

[Reinmar]

I talked with @jodator yesterday and we explained each other what's happening here.

So, @jodator's observation was that this post-fixer selects a widget, but it was losing the information that this selection should be made with a fake selection.

My observation was that while getViewData() shows a widget selection, the render renders a non-fake selection (because we see a blinking caret). What worried me was that via getViewData() everything seemed fine, so something must be seriously bad if it doesn't render well. Also, I was worried that a code which should not be able to break this (a post-fixer) was causing such problems

It turned out that:

• As a plugin author you can break how the selection is rendered for other features. You may do that because you need to know about the "fake" state of a selection.
• Our tools don't help us in tracking these issues so we may have automated tests which won't secure us from such issues.

Both things are quite serious, IMO. The first issue indicates IMO that the responsibility how the selection is rendered should be on the view element, not the converter. The second one shows that it would be safe if we'd improve getViewData() a bit.

cc @pjasiun

[jodator]

I've lost track how this should be fixed. After inspecting the PR it fixes the issue but there's no decision on what to do with the reported issues by @Reinmar.

[Reinmar]

IIRC the PR was good (for now). And there's a followup to make getData() dev util output the fake selection in a visible way to avoid such miscommunication and unprecise tests. Finally, there's #1574 to rethink this in the future, because we have an architectural issue that the post fixer needs to take care of this.