Introduce selection post-fixer

[jodator]

Suggested merge commit message (convention)

Feature: Introduced a selection post-fixer. Its role is to ensure that after all changes are applied the selection is placed in a correct position. Closes ckeditor/ckeditor5#4191. Closes ckeditor/ckeditor5#4204. Closes ckeditor/ckeditor5#4208. Closes ckeditor/ckeditor5#3167. Closes ckeditor/ckeditor5#3168. Closes ckeditor/ckeditor5#3171. Closes ckeditor/ckeditor5#562. Closes ckeditor/ckeditor5#611.

---

Additional information:

• This PR is required by tables pr: Changed: Prevent partial table selection by setting isLimit=true in s… ckeditor5-table#39.
• Probably fixes also: [FF] The caret appears at the top of the editor after right-click on the not focused editable ckeditor5#562, Problems with backwards selection around images ckeditor5#611 (to check).
• I'm not sure where to place this post fixer. Right now it is engine/controllers/ as the EditingController adds this post fixer.
• Selection might sometimes jump - but I did not experienced that in a while.

I've asked @Mgsy to check if mentioned bugs are fixed. For me it looks like this post fixer will also fix them.

[coveralls]

Coverage remained the same at 100.0% when pulling 0c10532 on t/ckeditor5-table/12 into 457afde on master.

[Mgsy]

Firefox

There is still a problem with the selection. If you try to select <paragraph><image><paragraph>, the first <paragraph> won't be selected. When you try to delete selected content and restore it, only the caption will be restored. Tested on Windows and macOS.

[Mgsy]

Firefox

The backwards selection makes a part of the selection blinks.

In other browsers there is no possibility to make the backwards selection.

[Mgsy]

Probably fixes also: ckeditor/ckeditor5#562, ckeditor/ckeditor5#611 (to check).

It seems that this one fixes ckeditor/ckeditor5#562 but unfortunately, ckeditor/ckeditor5#611 is still reproducible.

Besides that, there are still problems with selecting widgets (see previous comments), especially it concerns the backwards selection (all browsers). ckeditor/ckeditor5#611 is fixed.

[Reinmar]

It's a function so its name should start with a verb. You can find similar logic in the view – e.g. there's view/filler~injectQuirksHandling.

I'd propose to move this code to model/utils/selection-postfixer and expose a function called injectSelectionPostfixer. I'd also say that it can be used in Model#constructor() because:

• it will be symmetrical to View#constructor() injecting some behaviours,
• it's related to schema and basic schema is defined in Model.

[jodator]

K, thanks for pointing out proper placement for the logic. I'll update the PR with this change.

[Reinmar]

Actually, I see that this function doesn't add a postfixer itself. Check out how view/filler.js is structured – this way of exposing filler's functionality encapsulates what it needs to do – how many listeners it has to register, with what params, etc.

In this case, we're talking about just one post fixer and registering it in a straightforward way via doc.registerPostFixer(). Therefore, on one hand, exporting the callback like you did seems sufficient. OTOH, it's hard to tell now whether this is all that we'll ever need. Hence... IDK :P I leave it up to you.

[Reinmar]

This PR blows up 59 tests ;) Some of these tests check some commands execution on selection spanning limit elements. But some are weird – mainly, in the lists where I think we shouldn't have any problems.

Now, should we remove these tests or fix them? Theoretically, those tests check whether commands work fine for incorrect selections, but incorrect selections are automatically fixed, so these tests (and code which they test) are unnecessary. But, we post-fix the selection so it's possible to (programmatically) set incorrect selection and execute the command immediately after that. Hence, commands need to work for incorrect selections too.

[Reinmar]

Wow, this change works better than I expected. It doesn't, at least on Chrome, get in a way of doing various selections. I found only minor non-blocking issues. And, my biggest surprise is that Ctrl+A kinda started working when widget is at the beginning/end of the content :D

[jodator]

I've pushed the branch t/ckeditor5-table/12 with proper branches set in mgit.json in ckeditor5 repo.

[jodator]

Side note: While test are ready to be reviewed I still need to refactor the post fixer as in comments.

[jodator]

@Reinmar OK so the tests are fixed. The PR in List feature has master merged as some list tests were broken.

[Reinmar]

The documentation of all the new functions only mention limit elements. While, since you also use getNearestSelectionRange() we also make sure that selection doesn't end up in a place where text isn't allowed. You wrote the documentation (esp. the public one) too oriented on the limit element aspect. I'll change this a bit because the role of this tool is to fix ALL selections. Right now it means two rules, but it may be more in the future. The public docs should be focused on that generic use case for this feature.

[Reinmar]

I found a series of bugs with shift+left/right around images (when starting from a collapsed selection outside of an image). For some reason these cases we working fine with tables.

I realised that we focused on limit elements, but the same rules need to be applied to object elements. You also can't have such a selection:

<paragraph>xx[x</paragraph>
<image>]<caption>xxx</caption></image>

In the image feature only the caption is marked as a limit. Image isn't because theoretically this was a different semantics. It's an object element. But perhaps all objects are limits too?

We may, perhaps change this in the schema, but I'm not sure yet. I checked though, that after doing (isLimit(node) || isObject(node)) checks in the selection post-fixer most bugs disappeared. In fact, only one set of them remained:

<p>xxx</p>
<image><caption>yyy</caption></image>
<p>x[]xx</p>

1. Press shift+left a couple of times (e.g. 3)
2. Press shift+right the same number of times

You should be in the same place. But, you get stuck on the image. Why? Because shift+right makes such a selection:

<p>xxx</p>
<image><caption>[yyy</caption></image>
<p>x]xx</p>

And we extend it to:

<p>xxx</p>
[<image><caption>yyy</caption></image>
<p>x]xx</p>

Reverting what the user did by pressing shift+right. Unfortunately, to solve this we need to distinguish between left and right keys. This can only be done in the ckeditor5-widget package and needs to be a followup of this ticket.

[Reinmar]

We may, perhaps change this in the schema, but I'm not sure yet. I checked though, that after doing (isLimit(node) || isObject(node)) checks in the selection post-fixer most bugs disappeared. In fact, only one set of them remained:

OK, I decided to make schema.isLimit() return true for isObject elements. This makes quite a lot of sense – a self-contained non-dividable element limits the selection and other operations too. You can never select or delete a part of e.g. an image. This allowed simplifying a couple of existing conditions which checked for isObject || isLimit.

[Reinmar]

I spent a couple more hours on this because I realised that this PR doesn't introduce a complete set of rules. It also oversimplified some scenarios (crossing limit boundaries), which would hit us as soon as we'd use any block content inside table cells or image captions.

I extracted these scenarios to https://github.com/ckeditor/ckeditor5-engine/issues/1436 to not block this ticket and I'm running now all the tests again. Hopefully, this PR can be merged as-is at this stage.

@Mgsy, could you do last checks how this PR behaves? You'll need to do that through a setup of branches in ckeditor5@t/ckeditor5-table/12.

[Mgsy]

LGTM.

[Reinmar]

This is from master (I'm testing a different PR):

How does it behave on this PR?

[Mgsy]

How does it behave on this PR?

It selects the widget:

[Reinmar]

Pity. But I'm afraid that this all we can do for now.

[Reinmar]

Pity. But I'm afraid that this all we can do for now.

I think that we may fix this when working on row/col selection. It'd be tricky, but we can:

1. record that someone pressed shift+up,
2. using an early post-fixer (provided by the table feature) check when the selection went,
3. if it's spanning two cells (the starting one and the one above) create a selection of two cells.

If the table's custom selection post-fixer will be executed before the post-fixer that we're adding right now kicks in, then we'll have all the information that we need to make the right decisions.

[Reinmar]

It seems that this one fixes ckeditor/ckeditor5#562 but unfortunately, ckeditor/ckeditor5#611 is still reproducible.

After my latest changes this issue is fixed too :)

[Reinmar]

OK, the biggest issue I see is the one I mentioned in #1431 (comment). I think we'll need to work on it soon. But it's not a regression, cause on master this is bad too, so we're fine to move forward.

[Reinmar]

I reported two tickets for the mentioned shift+arrows thing: https://github.com/ckeditor/ckeditor5-widget/issues/42 and https://github.com/ckeditor/ckeditor5-table/issues/64.