Selection optimizations

[engineering-this]

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

What changes did you make?

Selection might cause various buggy behaviours. As a workaround I propose to adjust selection ranges on selectionCheck event. selection.js has 2500 lines of code so I created new module.

Notes

This is an early PR, there are still some things to be done, but first I'd like some feedback on proposed solution.

Right now manual tests are enabled on all envs, however some of cases are reproducible on specific browsers, so please check corresponding issues for more details.

TODO:

• Should we make the method private? It's used in one place.
• API docs.
• Fix failing tests:
  
  ☝️ Some tests need small adjustments - missing method added within this PR in mocked selection instance., others rely on selection which we prevent, so probably these cases can be removed. It doesn't look like anything is broken.

Closes #3175
Closes #3118
Closes #3161
Closes #3256

[engineering-this]

This should be retargetted and rebased to master, but first master needs to be ready for 4.12.1 commits.

[f1ames]

Overall, it looks good, going in the right direction.

Now I started to wonder if we really should have it in a separate file or just add it to the selection.js - as separate function wrapping everything inside like:

CKEDITOR.dom.selection.prototype.optimizeInElementEnds = (function() {
    // local functions
    return function() { ... }
} )();

but still not sure, maybe we will be able to come up with proper naming for this separate file which will convince me.

[engineering-this]

Now I started to wonder if we really should have it in a separate file or just add it to the selection.js - as separate function wrapping everything inside like:

I know that this file doesn't really fit into our codebase, however selection.js has over 2500 lines, my IDE with linters occasionally freezes when editing or browsing such big files. This all leads too poor developer experience.

However when scanning through selection.js I see that some bigger portions could be extracted to separate file:

• CKEDITOR.on( 'instanceCreated', ... )
• CKEDITOR.on( 'instanceReady', ... )
• Adding selection related prototype methods of editor, range, and more.
• Adding constans under CKEDITOR namespace, eg. CKEDITOR.SELECTION_NONE = 1;

So that original file would contain only CKEDITOR.dom.selection class and helper functions used by it.

☝️ Such refactoring might make merging some of existing PRs a bit problematic, probably we should extract it to separate ticket, if we even want to touch this.

[f1ames]

Can you check what's going on with the CI, there are couple of tests failing (restarted it, but it's still red).

The refactoring mentioned in #3213 (comment) would probably make the codebase more readable. Could you extract it to a separate issue?
For now I would be for creating selection directory and putting there optimization.js file with what we have in selectionoprimizer.js now. And then eventually we will proceed with the follow-up, WDYT?

[engineering-this]

I've found one issue introduced by these changes. When changing selection with Shift+Arrow Key optimization reverted change on selection from keystroke resulting in not being able to move selection outside of current element.

I fixed it by adjusting optimization method to consider last keystrokes. This however leads to yet another issue - when you start changing selection with keystroke always only one end of current range changes, however if selection is changed (even if it looks identical) in Firefox, IE and Edge browser doesn't know which end of selection should be moved on next keystroke. This makes selecting things with keyboard a bit problematic, so for this case there won't be optimizations.

[f1ames]

Rebased onto latest major.

[f1ames]

Looking good, few things to polish still.

Tests

Could you add one more generic manual test with elements like list, table, etc. and description how selection which will be optimized can be created (e.g. triple click, shift+arrow keys, selecting with mouse)?

Issue with Shift+Arrow Key selection

Issue

Referring to #3213 (comment):

This however leads to yet another issue - when you start changing selection with keystroke always only one end of current range changes, however if selection is changed (even if it looks identical) in Firefox, IE and Edge browser doesn't know which end of selection should be moved on next keystroke.

I see this is more general issue also reproducible on Chrome. If you press and hold Arrow Key, selection moves nicely between lines and optimization doesn't break anything (as it is not triggered). However, when you press and release (expanding selection step by step) it happens on Chrome too:

Improved solution

We could live with that as it's not that common use case, however I have an idea how to improve it further. What if optimization will not be executed if Shift is pressed? With such approach slowly moving with Arrow Key will work and ignoring for specific browser would not be necessary so code will get simpler.

Edge cases

There are still two cases where it will break:

If someone releases Shift for selection which should be optimized and then presses it again and try to continue changing selection in the same direction - very uncommon edge case IMHO which we can accept (it also happens now).

There might be a case when someone performs to-be-optimized selection with Shift pressed and then uses Delete/Backspace while Shift is still pressed - then optimization will not be applied and deletion would work same as without the fix (see .gif below where I have changed the code so optimization is not applied while Shift is pressed):

So above the Shift is pressed all the time and using Delete moves content inside table cell as reported in #3256. Again that's very uncommon edge case (selecting something with Shift+Arrow Key and not releasing Shift before deleting content - and all happens for to-be-optimized selection). Usually Shift needs to be released (or simply is released as this is the way most people are using keyboards I think) before any other operations are performed (like changing Heading level) so optimization would still be applied correctly.

Code

Since current code already assumes Shift key is pressed and only then saves recent keystrokes very small adjustments will be needed in core/selection/optimization.js to just prevent optimization when editor._lastKeystrokeSelection is set. Or since you don't need to check what arrow keys were pressed it can be further simplified to pass only necessary data.

Ufff, that's complex for such tiny details... 😓

[f1ames]

One more thing, few tests/core/selection/optimization tests fails for build editor on CI, please check what's going on there.

[engineering-this]

Improved solution

We could live with that as it's not that common use case, however I have an idea how to improve it further. What if optimization will not be executed if Shift is pressed? With such approach slowly moving with Arrow Key will work and ignoring for specific browser would not be necessary so code will get simpler.

I haven't thought about this earlier, but that's great idea. Delaying optimizations until Shift allows simplifying code, and it seems to work better! Also plenty tests can be removed.

[f1ames]

Unit tests

• Chrome ✅
• Firefox ✅
• Safari 🚨

• IE8 🚨

• IE11 ✅
• Edge ✅

Manual tests

1. I found one case on Firefox when selection is not optimized. Looks like edge case, and I'm not sure if we should handle it - the image is not selected, however mouse button was released over it so one may expect that selection also cover it 🤔 Please take a look, maybe it can be easily fixed and if not we will just skip it:

☝️ I see it works fine in other browsers so we should check if can be improved on Firefox.

2. Another issue on Firefox (alos happens in IE and Edge), when selection starts at the end of paragraph, applying heading changes both lines for the first time (but then it works fine 🤔):

---

I have noticed that Safari is the only browser which visually shows the difference between such selections - and you can clearly see when the optimization triggers:

Nothing to work on here, just wanted to mention it 🕵

[f1ames]

Rebased onto latest major.

[f1ames]

LGTM 👍

Unit tests

Chrome (CI) - ✅
FF (CI) - ✅
Safari - ✅
IE8 (core only) - ✅
IE11 (few previously failing) - ✅

Manual tests

Chrome - ✅
FF - ✅
Safari - ✅
IE8 - ✅
IE11 - ✅