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

Making changes directly in the source view drops the made changes on save #10505

Closed
rpkoller opened this issue Sep 9, 2021 · 8 comments · Fixed by #10697
Closed

Making changes directly in the source view drops the made changes on save #10505

rpkoller opened this issue Sep 9, 2021 · 8 comments · Fixed by #10697
Assignees
Labels
package:source-editing squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@rpkoller
Copy link

rpkoller commented Sep 9, 2021

It is an issue i've stumbled across when I was testing the CKEditor 5 contrib module in Drupal 9. I've initially reported it in the according issue tracker ( https://www.drupal.org/project/ckeditor5/issues/3229174 ) but Wim Leers asked me today if I could report it upstream.

📝 Provide detailed reproduction steps (if any)

  1. Enter text Three bold words
  2. Select the three words and hit the bold button
  3. Save
  4. Open the document again
  5. Click the source button
  6. The markup looks like:
<p>
    <strong>Three bold words</strong>
</p>
  1. Change the markup to:
<p>
    <strong>Three bold</strong> words
</p>

or you could also change the string to Three bold words plus two
8. Hit directly save and dont switch back to the regular mode beforehand

✔️ Expected result

I would expect that Three bold is displayed in bold and words not in bold. And in case that I've extended the phrase the plus two would be also shown.

❌ Actual result

All changes are lost if the document is saved directly in the source view. If I switch back to the regular view beforehand and save all changes are properly saved.

📃 Other details

  • Browser: Tested with Firefox 91.0.2 and Safari 13.1.2
  • OS: MacOS 10.13.6
  • First affected CKEditor version: I am not sure at which CKEditor 5 version the Drupal module was at the time I've reported the issue in the modules issue tracker. But I've retested it with the current version today and the issue persists:
  "dependencies": {
    "dllCkeditor5": "https://github.com/ckeditor/ckeditor5.git#v29.2.0"
  }
  • Installed CKEditor plugins: In regards of used CKEditor plugins I don't know ( I don't consider myself a developer more into content strategy but tried to help testing the CKEditor 5 module for Drupal). The listed dependencies in the CKeditor 5 module package.json are:
"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"
  }

If you need more informations please let me know.

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

@rpkoller rpkoller added the type:bug This issue reports a buggy (incorrect) behavior. label Sep 9, 2021
@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Sep 28, 2021
@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2021

Thanks. To rephrase: editor.getData() returns the content of the model, not the source editing area. It should return the latter if we're in a source mode.

@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2021

Question: Should the content of the textarea be passed through the model?

  • Pros: You'll always get a valid HTML, aligned with what's allowed in the editor.
  • Cons: You may be surprised that something that you wrote in source mode wasn't saved (might've been filtered out).

CKEditor 4 returns exactly what's in the textarea, so this is possible:

Is it a feature or a bug?

@wimleers
Copy link

wimleers commented Oct 7, 2021

@Reinmar brought this up in a meeting with @lauriii and I. Points in favor:

  • the user has the context at this time: if the saved result does not render it like the user entered, then they'll notice it right away — assuming they (pre)view the result (who can type flawless HTML markup on the first try without checking the end result? 🧟 💀 😂 )
  • Drupal has been seeing the occasional bug report about this! We did not realize CKEditor 4 didn't do this 😅

So +1!

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2021

Scope:

  • Decorate DataController#get()
  • SourceEditing should listen to DataController#get and when in source mode call editor.data.set() in the same way that _hideSourceEditing() does now.
  • We may need to take care of _dataFromRoots (not sure how it works now).

@niegowski niegowski self-assigned this Oct 14, 2021
@niegowski niegowski added this to the iteration 48 milestone Oct 14, 2021
oleq added a commit that referenced this issue Oct 15, 2021
Fix (source-editing): Calling `editor.getData()` while in the source editing mode should return the data from the source editor passed through the model. Closes #10505.

Feature (engine): The `DataController#get()` is now decorated and fires a `get` event on the method call. See #10505.
@wimleers
Copy link

We're updating to today's https://github.com/ckeditor/ckeditor5/releases/tag/v31.0.0 release in Drupal and unfortunately this bug is not fixed:

10505.not.fixed.in.31.0.0.720p.mov

😞

@niegowski
Copy link
Contributor

We're updating to today's https://github.com/ckeditor/ckeditor5/releases/tag/v31.0.0 release in Drupal and unfortunately this bug is not fixed:

I couldn't reproduce this without editing the data in the source editing mode so that it returns to the original state, but I found an issue with the scenario:

  1. open editor with "abc" text in it
  2. enter source editing mode
  3. add "123" at the end of "abc"
  4. trigger editor.getData() -> <p>abc123</p>
  5. without leaving the source mode remove just added "123"
  6. trigger editor.getData() -> <p>abc123</p>

this is happening only if you modify sth in the editing mode and then rollback to the previous content, only then the editor returns the previous data because the internal storage of the editor data while entering source editing mode was not updated on editor.getData().

Another case that I could not reproduce is the thing that we compare that "previous" content with the data updated on the input event so I wonder if this might be a problem (in case that event got stopped).

I pushed some changes to this branch, it would be great if you could verify if it solves your issues.

@Reinmar
Copy link
Member

Reinmar commented Oct 27, 2021

I tried to reproduce the original bug here (https://ckeditor.com/docs/ckeditor5/latest/features/source-editing.html) and it seems to work. I guess the specific scenario that @niegowski mentioned may be still broken, but it's quite a specific one.

@wimleers
Copy link

wimleers commented Nov 4, 2021

So what could we possibly be doing differently?

I tried to reproduce the original bug here (https://ckeditor.com/docs/ckeditor5/latest/features/source-editing.html) and it seems to work. I guess the specific scenario that @niegowski mentioned may be still broken, but it's quite a specific one.

How can you simulate Hit directly save and dont switch back to the regular mode beforehand from the original bug on that page?

Confirmed that editor.getData() is working, and the <textarea> is updated, but … the POST request still contains the stale data 🤯

For now I'm concluding that this is a problem on the Drupal side, but it sure is mysterious! Thank you and sorry for the false alarm! 🙈

If you want to follow along: https://www.drupal.org/project/ckeditor5/issues/3229174#comment-14280987

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:source-editing squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
5 participants