Problem/Motivation

The release of Drupal 10.2.5 has introduced a regression in (but not limited to) Site Studio's implementation of CKEditor 5 that results in user input being lost if they type too fast.

  1. A user types into Standard install profile's "Article" node type's "body" field
  2. Uses the Drupal.editors.ckeditor5.onChange event to pull the value of the editor using ckInstance.getData() and store that in a JSON blob.
  3. Since this change https://git.drupalcode.org/project/drupal/-/commit/5540b5f84a6ae7899df01... if a user types multiple characters in quick succession, only the first character they type is registered in the on change event and all subsequent inputs are lost.
  4. I think the issue is due to the use of the immediate variable on the debounce call.

Steps to reproduce

If you paste this snippet into the JS console on a node page with a CKEditor instance and then type into the editor quickly vs slowly, you can see the issue. When typing quickly the onChange event is fired after the first user input but not fired again after the user types their last input.

Drupal.editors.ckeditor5.onChange(document.getElementById('edit-body-0-value'), () => {
 const ckInstance = Drupal.CKEditor5Instances.values().next().value;
  console.log(ckInstance.getData())
});

I've attached a video showing the above steps to test demonstrating the issue.

Proposed resolution

Remove the debounce from the onChange method. In that case, onChange will be called on each change immediately, and if the user/contrib code needs to throttle that, it is their responsibility, and they will be able to decide how much they want to throttle it.

Remaining tasks

Decide if removing debounce is viable. If it is, merge the PR.

User interface changes

None

Introduced terminology

None

API changes

The Drupal.editors.ckeditor5.onChange callback will no longer be throttled by itself. Nothing in core seems to be impacted performance-wise by this. In case the user code needs throttling, they should add it themselves.
For reference, for quite some time before https://www.drupal.org/project/drupal/issues/3396742, due to a mistake in the code, the changes were not throttled anyway, but just delayed by 400ms.

CommentFileSizeAuthor
#20 ezgif-14371c08e1b419.gif619.23 KBjaswinsingh
#11 3443422.patch602 bytes8ballsteve
#5 d11typingfasteditorgif.gif723.33 KBankitv18
#5 d11fasttypingeditorissue.mov3.59 MBankitv18
#3 cke5 event typing speed.gif143.69 KBwim leers
Screen Recording 2024-04-24 at 17.26.28.mov5.05 MBjessebaker

Issue fork drupal-3443422

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jessebaker created an issue. See original summary.

jessebaker’s picture

jessebaker commented
Title: Drupal.editors.ckeditor5.onChange event has outdated data when user types too fast. » Drupal.editors.ckeditor5.onChange event doesn't fire after final input if user types too fast.
Related issues: +#3396742: CKEditor 5 doesn't save updated value if form submitted right after the change
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Drupal.editors.ckeditor5.onChange event doesn't fire after final input if user types too fast. » [regression] Drupal.editors.ckeditor5.onChange event doesn't fire after final input if user types too fast.
Issue summary: View changes
Issue tags: +Contributed project blocker, +JavaScript, +Regression, +data loss, +Needs tests
StatusFileSize
new cke5 event typing speed.gif143.69 KB

This is a regression introduced by #3396742: CKEditor 5 doesn't save updated value if form submitted right after the change 😭

Let's get the (detailed, thank you!) steps to reproduce encoded in the core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php test coverage that #3396742 expanded. We should be able to reproduce the failure in there.

Reproduced easily without Site Studio, just by using a Drupal core text field. Rephrased the issue summary for that. This does not negatively impact Drupal core today, but impacts potentially many contrib modules.

ankitv18 made their first commit to this issue’s fork.

ankitv18’s picture

ankitv18 commented
Issue tags: -JavaScript +JavaScript
StatusFileSize
new d11fasttypingeditorissue.mov3.59 MB
new d11typingfasteditorgif.gif723.33 KB

if debounce method is called with callback and delay only then it works perfectly.
Typing fast fix

rakun’s picture

rakun commented

Hi @ankitv18
Did you provide patch for this since this branch looks empty?

ankitv18 opened merge request !8284

ankitv18’s picture

ankitv18 commented

MR!8284 opened agaisnt 10.2.x branch, if this fix resolve the issue then we can push the commit to the 11.x branch too.

ankitv18’s picture

ankitv18 commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Version: 10.2.x-dev » 11.x-dev
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

11.x was correct target for the MR as a fix will have to land there first. I do see comment #8 but fix will have to pass 11.x too.

Tagging for issue summary as proposed solution appears to be missing. This a UI issue? If so can we add before/after there.

Also appears to be tagged for tests which may still be needed. Though with regressions (especially data loss) I'm a +1 for fixing and opening a follow up for expanding test coverage. But I can't make those calls.

8ballsteve’s picture

8ballsteve commented
StatusFileSize
new 3443422.patch602 bytes

Adding the change from the MR as a patch so we can kick off some tests against this.

ok4p1’s picture

ok4p1 commented

Hello, the patch mitigates the issue but I am still experiencing it

Taking this back, I need to do more testing.

reinfate made their first commit to this issue’s fork.

reinfate opened merge request !10265

reinfate’s picture

reinfate commented
Status: Needs work » Needs review

I wonder if we could just get rid of debounce to fix this. (I pushed MR!10265 with that change)
Before the #3396742: CKEditor 5 doesn't save updated value if form submitted right after the change it worked just like without debounce, but with a 400ms delay for each call as mentioned in the issue summary there.

And IMO it just makes more sense, that onChange should be called on each change immediately, and if the user/contrib code needs to throttle that, it is their responsibility, and they will be able to decide how much they want to throttle it.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Issue summary still appears to need updating. Recommend using standard issue template.

Was previously tagged for tests but not sure what those would look like

reinfate’s picture

reinfate commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

I expanded the \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test::testSave to check the behaviour of the onChange event. The test assumes there will be no debounce/throttling from this method.
Updated the IS
Back to "Needs review".

smustgrave closed merge request !8284

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +Needs Review Queue Initiative 
1) Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test::testSave
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 [
     0 => '<p>c</p>',
-    1 => '<p>ch</p>',
-    2 => '<p>cha</p>',
-    3 => '<p>chan</p>',
-    4 => '<p>chang</p>',
-    5 => '<p>change</p>',
 ]
/builds/issu

Test change looks good and summary is correct so removing those tags.

Following the steps am seeing the issue and MR does appear to addressed.

jaswinsingh’s picture

jaswinsingh commented
StatusFileSize
new ezgif-14371c08e1b419.gif619.23 KB

We are facing a similar issue with the source editing plugin in Site Studio's component. The changes are not being applied to the component.

gif

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

let's go with removing debounce, the dependency needs to be removed as well.

maxdmayhew’s picture

maxdmayhew commented

I also am running into this issue on 10.4.6.

I have reported this to Acquia and honestly the workaround seems to be to go slow and wait 10 seconds or so before saving. The patch did not work for me.

Additionally, this seems more of an acquia specific issue in my testing. I am not experiencing this issue outside of the layout canvas.

jigarius’s picture

jigarius
Location Montréal
commented

I agree with maxdmayhew's comment #22. I think the issue is related to how Site Studio initializes its CKEditor instances. I don't get the issue outside Site Studio. I've created a support ticket with them but it seems they cannot reproduce the issue. If I have some time, I'll try to reproduce the error with a fresh Drupal install with just Site Studio on it.

For now, I've resorted to cheating with a custom event listener that triggers does an editor.setData(editor.getData()) when the user disables source editing mode. Some sources suggest that editor.fire("change:data") should help, however, it didn't work for me. IMHO, the issue probably stems from how Site Studio gets the data from the CKEditor widget.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.