Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't change the selection while calling Editor#getData() in intervals #12219

Closed
Mgsy opened this issue Aug 3, 2022 · 5 comments · Fixed by #12223
Closed

Can't change the selection while calling Editor#getData() in intervals #12219

Mgsy opened this issue Aug 3, 2022 · 5 comments · Fixed by #12223
Assignees
Labels
squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Mgsy
Copy link
Member

Mgsy commented Aug 3, 2022

📝 Provide detailed reproduction steps (if any)

  1. Go to https://ckeditor.com/docs/ckeditor5/latest/examples/builds/classic-editor.html.
  2. Set the content with the table: https://gist.github.com/Mgsy/e72dd372ca99025249228af6ee55c548 
  3. Downgrade the CPU performance 6x (Dev tools -> Performance).
  4. Run setInterval( () => console.log( editor.getData() ), 200 ).
  5. Click on some cells to change the selection.

❌ Actual result

It's impossible to change the selection or the selection jumps.

📃 Other details

It's not reproducible in v30.0.0 https://ckeditor.com/docs/ckeditor5/30.0.0/examples/builds/classic-editor.html.


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@Mgsy Mgsy added type:bug This issue reports a buggy (incorrect) behavior. support:2 An issue reported by a commercially licensed client. squad:core Issue to be handled by the Core team. labels Aug 3, 2022
@niegowski
Copy link
Contributor

Results of my debugging. 

Normal flow:

  1. The user clicks on the text
  2. selectstart event
    1. view.document.isSelecting = true (selection rendering is disabled while the isSelecting flag is set)
  3. selectionchange event
    1. model selection is updated to the current position of DOM selection
    2. it's down-casted to the view selection
  4. since the view selection changed, the renderer tries to update the DOM selection
    1. but it's ignored because the isSelecting flag is still set
  5. mouseup event
    1. view.document.isSelecting = false
  6.  the Renderer notices that the isSelecting flag is no longer set so triggers render to update the DOM selection
    1. selection up to date (the view selection matches the DOM selection)

The invalid flow:

  1. The user clicks on the text
  2. selectstart event
    1. view.document.isSelecting = true (selection rendering is disabled while the isSelecting flag is set)
  3. User clicks in some other text position
  4. mouseup event
    1. view.document.isSelecting = false
  5.  the Renderer notices that the isSelecting flag is no longer set so triggers render to update the DOM selection
    1. since there was no selectionchange event the model and view still see selection in the previous position
    2. the renderer updates DOM selection to the position known by the view (not the one user clicked)
  6. selectionchange event (with the latest DOM selection updated by the renderer in the previous step)
    1. model selection is updated to the current position of DOM selection
    2. it's down-casted to the view selection
  7. since the view selection changed, the renderer tries to update the DOM selection
    1. DOM selection is set back to the previous position (not the one clicked by the user)

So the problem was that sometimes the selectionchange event does not trigger soon enough and because we use the current DOM selection while handling that event, we do not even know that there were multiple changes along the way. Also, by triggering render after the selection is over, the DOM selection gets updated to the previously known position and that makes the following selection change events report the previous position.

@niegowski niegowski self-assigned this Aug 4, 2022
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Aug 4, 2022
@Reinmar
Copy link
Member

Reinmar commented Aug 5, 2022

I suppose the delay with which selectionchange is being fired is related heavily to performance of the entire browser. So it was enough to do getData() on a short content on a throttled CPU to trigger this?

Very interesting case. Thanks for a detailed writeup.

@Reinmar
Copy link
Member

Reinmar commented Aug 17, 2022

BTW, we realized that this scenario to make getData() in 200ms makes no sense.

It only makes sense to call getData() when something's changed in the model data and this can be done by listening to editor.model.document#change:data and debouncing it the same way Autosave does

Thanks to that getData() will not be called when the user is actually making a selection because making a selection does not change the model data. Hence, the issue should not exist.

@ckeditor/qa-team, could you check on master that replacing the interval with Autosave set to 200ms prevents the issue?


We also discussed #12223 and another idea appear. What if we delayed the render() (e.g. by delaying the entire change:isSelecting) that caused this issue? Would that allow the buffered native selectionchange event to fire, update our model&view selection and thus also prevent the issue?

@niegowski Could you verify this and if it works, add this improvement to the PR?

@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. labels Aug 17, 2022
@FilipTokarski
Copy link
Member

I checked it and as you described - with autosave there is no problem with selecting the cells.

@CKEditorBot CKEditorBot removed the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Aug 22, 2022
@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Aug 29, 2022
@niegowski
Copy link
Contributor

We also discussed #12223 and another idea appear. What if we delayed the render() (e.g. by delaying the entire change:isSelecting) that caused this issue? Would that allow the buffered native selectionchange event to fire, update our model&view selection and thus also prevent the issue?

@niegowski Could you verify this and if it works, add this improvement to the PR?

I was trying to debounce the isSelecting = false but as a result it's impossible to select multiple table cells, probably because multi cell selection is also using mouseup events. 

Another thing is that the delay seems to give inconsistent results. Sometimes it manages to handle the selection change and sometimes it does not. I'm wondering about another solution. Since the whole thing is about outdated model/view selection then maybe we could force model selection update (flush) just before the renderer is about to kick in after selecting is done.

@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Sep 12, 2022
@CKEditorBot CKEditorBot added this to the iteration 57 milestone Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants