I am running Drupal 9.2.4 and the latest version of ckeditor5. When testing the behavior of another issue i've filed i've noticed that if you click on the source button and remove e.g, wrapping elements u tags and hit directly save for the node the made changes are not applied. if you instead leave the source mode, after making the changes, back to the regular ckeditor wysiwyg view and then hit save the changes are saved and shown in the front-end as expected.

CommentFileSizeAuthor
#15 10505 not fixed in 31.0.0.mov12.09 MBwim leers

Issue fork ckeditor5-3229174

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

rkoller created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Making changes directly in the source view drops the made changes on save » [upstream] Making changes directly in the source view drops the made changes on save
Component: User interface » Code
Category: Task » Bug report
Status: Active » Postponed
Issue tags: +Needs upstream bugfix

Excellent find! Will raise this in the meeting we have with the CKE5 team this Thursday! 😊

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Critical
Issue tags: +data loss

Bumping priority because this is a form of data loss.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

FYI: We ran out of time before we got to this one. Will discuss it next time.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Could you please report this at https://github.com/ckeditor/ckeditor5/issues? 🙏

(If you don't, that's fine, I'll do so when I get back from vacation!)

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

Sure you dont have to deal with that before your vacation. will write up and create issues later tonight or tomorrow (for this issue and the other one you also asked for an upstream report). Just one brief question. Shall i retest with the latest version of the ckeditor5 contrib module upfront if the issue still persists and then reference the git id of the dev version and mention that it was found and tested in drupal9? or what would be the best, helpful and preferred procedure for the upstream devs?

p.s. enjoy your vacation! :)

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

Went ahead since I had a bit of free time to write up both issues you've asked for. Issue could be found over here: https://github.com/ckeditor/ckeditor5/issues/10505

p.s. the only thing i was unsure how to report and write was the `installed CKEditor plugins` part. I've just mentioned the listed dev dependencies in the CKEditor 5 modules package.json: 

"devDependencies": {
    "@ckeditor/ckeditor5-dev-utils": "^25.2.0",
    "raw-loader": "^4.0.2",
    "terser-webpack-plugin": "^5.2.0",
    "webpack": "^5.51.1",
    "webpack-cli": "^4.4.0"
  }

is that correct or completely wrong? :D or are there others?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thank you!!! 🤗🙏

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

It's going to be released today, October 26: https://github.com/ckeditor/ckeditor5/issues/10637. Let's wait for that :)

Wim Leers credited tim.plunkett.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Marked #3245845: Changing code in Source Editing mode is not saved unless switched back out of Source Editing first as a duplicate. Transferring issue credit.

tim.plunkett credited webchick.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Thanks, not sure how I missed this one before opening.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [upstream] Making changes directly in the source view drops the made changes on save » [PP-1] [upstream] Making changes directly in the source view drops the made changes on save

#3245942: Update CKEditor 5 to 31.0.0 will update our CKE5 build to today's release, that should fix this!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] [upstream] Making changes directly in the source view drops the made changes on save » [upstream] Making changes directly in the source view drops the made changes on save
StatusFileSize
new 10505 not fixed in 31.0.0.mov12.09 MB

Turns out that this is not actually fixed yet in CKEditor 5 31.0.0. Even using the exact steps to reproduce that @rkoller posted upstream. Very unfortunate.

Also https://github.com/ckeditor/ckeditor5/issues/10505#issuecomment-952086969

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

@wim-leers hm i've installed ck5 in a sandbox installation of drupal 9.3.x-dev and applied the merge request 141 with Claro as the admin theme. i would say it works half way. it works for entities not created yet but as soon as an entity is saved it doesn't work anymore. (already wanted to intervene and say it works on my end but i did one detail differently which i've noticed afterwards now. perhaps that detail helps debugging)

if you go through the eight steps described in https://github.com/ckeditor/ckeditor5/issues/10505 the bug persists. but create a new article.
then enter the three words and change their styling to bold. but instead of saving and opening the article again you directly switch to the source editor and proceed with changing the markup that only one or two words are styled in bold. you then hit save and you notice that those entries are actually saved. but if you go back to the source editor now and try to change the markup again the new changes arent saved anymore. so the functionality is basically there for ckeditor5 otherwise it wouldnt work for new nodes on the first save?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » lauriii
Status: Postponed » Active

Over at https://github.com/ckeditor/ckeditor5/issues/10505#issuecomment-952211184 a CKEditor 5 developer said that editor.getData() works fine.

I can confirm that this is the case in Drupal too.

Drupal.editors.ckeditor5.detach() does this:

      if (trigger === 'serialize') {
        editor.updateSourceElement();
      } else {

I suspect this is the problem.

I just had a meeting with the CKEditor 5 team, and they were surprised we're even doing that. @zrpnr added that all the way in January, when this module was still getting kickstarted. So no idea what the rationale is.

We were told to remove that editor.updateSourceElement(); (EDIT: maybe for in-place editing? 🤔) and it should work fine … but it does not. 😢

Both with and without that line, I can confirm that:

  1. editor.getData()
  2. $x('//textarea')[1]

both return the correct data. But still either with or without that line, the data does not get saved. I can also confirm that by looking at the POST request: its headers still contain the old data.

Something really weird is going on. @lauriii to the rescue!

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

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: [upstream] Making changes directly in the source view drops the made changes on save » Making changes directly in the source view drops the made changes on save
Assigned: lauriii » Unassigned
Status: Active » Needs review
Issue tags: -Needs upstream bugfix

I did some research on this, and in the end, I was able to figure out the root cause for this.

First, I was trying to figure out whether Drupal.editors.ckeditor5.detach() had something to do with that. Simply removing that didn't fix it, so I had to dig deeper.

I attached an event listener for submit and I was able to confirm that the updated value was in the form field at this point in both, Drupal, and in a minimal setup of CKEditor. After that, I attached another event listener to formdata event. By doing that I was able to point that in Drupal, the field value had been reverted between submit and formdata events. 🤯 However, in the minimal setup of CKEditor, the field value was retained between the events.

I set a breakpoint on the CKEditor 5 detach behavior to find out what was happening. I was quickly able to point out that Drupal.editorDetach was reverting the field value back to it's original state, because data-editor-value-is-changed was still set at false. 😐

I figured that the data-editor-value-is-changed was being set on onChange callback which Drupal editors are required to supply.

In the CKEditor 5 code, that was mapped to change:data on the CKEditor 5 model. At first I thought that there must be a problem on the CKE side and the event wasn't being triggered in source mode. However, that's wasn't the case. The event was triggered correctly and even at the correct time.

It turned out that it actually was a race condition, which was caused by our CKEditor 5 initialization code decorating the onChange callback with debounce. 😅

This means that in the end, it turns out it's completely on our side, we simply have to avoid decorating the callback with debounce when the callback is being called in source editing mode (which indicates that the form must be submitting too, because that's the only condition when the model is being updated when in source editing mode).

Sadly, I didn't manage to write test coverage for this within reasonable amount of time, given all the magic that is happening in the source editing mode. 😢

lauriii opened merge request !155

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

ufff that sounds tricky to debug. I've applied the diff and tested all the scenarios. at the beginning there was just one hickup until i've noticed that i forgot to clear the caches. but afterwards i was able to make changes to the markup in the source view and directly save and in each scenario i've tried the changes got saved and displayed properly in the frontend afterwards. :) (*i've tried with 9.3.0-alpha1)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

A debounced change event conflicting through a race condition only when submitting directly from SourceEditing, just for editor-switching XSS protection … wow 🤯🤣

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
  • Manually tested 👍
  • Deep analysis: #19 👍
  • Code & comments: 👍

🚢

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

  • Wim Leers committed 78888b4 on 1.0.x authored by lauriii 
    Issue #3229174 by lauriii, Wim Leers, rkoller, tim.plunkett, webchick:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.