ckeditor
Fix: selection optimization in list disallow full list removal
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
All patches that 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
- [X] Unit tests
- [X] Manual tests
Did you follow the CKEditor 4 code style guide?
Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.
- [X] PR is consistent with the code style guide
What is the proposed changelog entry for this pull request?
* [#4931](https://github.com/ckeditor/ckeditor4/issues/4931): Fix: selection optimization skips list elements and disallow list removal.
What changes did you make?
Give an overview…
Besides all details I put in the issue comments: I found that we have additional selection optimization which is responsible for shrinking selection under some circumstances. I added an additional check if the end selection container is a li element and prevents optimization if so.
Which issues does your PR resolve?
Closes #4931 .
Overall fix working very well but unfortunately, it's not working on Firefox. It's looking like after selecting the whole content by using CTRL+A range.startContainter and range.endContainer are the same and they contain the whole body instead of the first and last element from the list(line in the other browsers). 🤔 Could you at it?
If you take a look at those containers during select all action on FF - they are always indicating body element. So it looks like some other bug with FF, but this time on 'delete' action. I changed the content in the manual test. It still proves this issue (it is even closer to the content from the report). But it won't fail in FF. With the new content selection in FF also remains set on body - but this time the entire list is removed. Probably we should extract another issue for that - WDYT? In other case - that require a new 'debug' session to find out what cause this behaviour on FF.
I think that we missed some unit tests to cover this issue. If I remember correctly we currently have an existing tests with removing lists so we can add it here.
There are cases of 'removing' lists - but with 'deselecting' list feature - not with del/backspace key. Also, this issue is more about 'selection'. Because removing lists remains the same - the problem was that the selection was improper. So I would stick just to tests related to selection optimization.
Due to the fact that the issue in Firefox can also be reproduced in Chrome after using the selectall plugin, I would be for investigating it a little (even if only to check if the selection in these two cases is the same).
Select all plugin in div editor in both browsers (FF & Chrome) makes selection range.startContainer and endContainer set on div with editor content:
So the selection optimization early returns with false:
https://github.com/ckeditor/ckeditor4/blob/a7b0d54376a794f65bce64086bd5f6bb51cb02d9/core/selection/optimization.js#L116-L117
Not removing a single list element is not a problem with selection optimization.
In Safari the fix works correctly but something odd happens to the caret – it disappears after the deletion of the list.
The selection is set on the body after content deletion:
However, that happens only in this manual test 🤔 not happening in our dev samples:
So the selection remains the same, but the carret is not visible 🤔
@Comandeer - ready for another review.
- I'm going to create a follow-ups
- Please see the above comments as a response to your recent review.
- Added automatic test
- Refactor code according to the suggestion
- Just in case (random manual test performing) added select all plugin to the manual test
@Comandeer - as we spoke - I adjusted the manual test and reverted ctrl+A binding. Could you take a look at this PR again?
@Comandeer - I fixed the code for the IE8. It appears that the endContainer was not a li as in other browsers, but the text node inside the li. Because of that, the used method is does not exists on such node.
Also, adjusted the manual test. So the PR is ready for another review.