ckeditor
Fixed DOM selection rendering after missing selectionchange event
Suggested merge commit message (convention)
Fix (engine): Selection change should not be lost even if some selectionchange events won't get fired. Closes #12219.
Additional information
See https://github.com/ckeditor/ckeditor5/issues/12219#issuecomment-1205225533
I just briefly scanned the code out of curiosity how complex was the fix, but I didn't actually review this.
One general remark: Please leave traces to this ticket in the code. I'd add them in the condition that uses this flag and in the test added for this scenario.
@niegowski unfortunately, the original issue is not fixed. Actually, with these changes, I can't place the selection anywhere, as it jumps to the beginning of the document.
Interestingly one user from our community reports that this PR fixes also another issue (#12008). Worth to check it once the PR is ready.
@ckeditor/qa-team could you please retest this PR? Now it upcasts DOM selection just before the selection is completed so the model selection is up to date with the DOM selection before the render is triggered.
This is weird. Currently I'm not able to reproduce the original scenario on docs (older versions and latest) or locally on master. It works fine everywhere (on this branch too). The only thing that changed since I checked it last time is I upgraded Chrome to Version 105.0.5195.102.
Could someone confirm?
@FilipTokarski, I was able to achieve such behaviour (which I suppose is the original issue) on online docs (https://ckeditor.com/docs/ckeditor5/latest/examples/builds/classic-editor.html):
However, CPU throttling is required for achieving reproduction.
I know, I tested it with 6x CPU slowdown. What is your Chrome version?
I've just upgraded to: 105.0.5195.102 (arm64) and still can reproduce the issue.
I've checked both latest docs and docs generated on the PR with x6 CPU throttling. The issue still persists on the latest docs (although in much milder form than it used to - as shown on video below), I can't seem to reproduce it on PR at all.
https://user-images.githubusercontent.com/66953380/188818999-d0683b84-7f73-4295-ab35-aa3c8cbf3fe0.mov
@dufipl did you check if the issue persists for you on the PR?
@Acrophost, on this PR the selection does not jump anymore and the editor is usable. However, on my side, some slowness in its work is possible to see.
This PR is triggering the DOM selection upcast (and flush of DOM mutations) on mouseup DOM event (and keyup/down if isSelecting = true). It's triggered only when there was a selectstart DOM event that did not finish yet. This is introduced to make sure that the model/view selection is matching the DOM selection. The selectionchange DOM event is asynchronous and sometimes is fired too late so we sometimes "corrected" the DOM selection to the last known model selection but that was incorrect because of the asynchronous selectionchange event.
@Reinmar @scofalik do you see any potential problems with this approach?
I've read the discussion in issue and looked at changes. They seem fine and reasonable to me but honestly -- I am so out-of-touch with this part of the engine that my opinion doesn't really bring any value.
To me, the change looks good too. It seems safe to upcast the selection at that point.